Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1191404 > unrolled thread

Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional Kees Cook <keescook@chromium.org> - 2015-07-24 01:40 +0200
    Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional Andy Lutomirski <luto@amacapital.net> - 2015-07-24 01:50 +0200
      Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional Willy Tarreau <w@1wt.eu> - 2015-07-24 02:00 +0200
        Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional Kees Cook <keescook@chromium.org> - 2015-07-24 02:10 +0200
          Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional Willy Tarreau <w@1wt.eu> - 2015-07-24 09:30 +0200
            Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional Willy Tarreau <w@1wt.eu> - 2015-07-24 09:50 +0200

#1191404 — Re: [PATCH v3 2/3] x86/ldt: Make modify_ldt optional

On Thu, Jul 23, 2015 at 3:24 AM, Willy Tarreau <w@1wt.eu> wrote:
> Hi Andy,
>
> On Wed, Jul 22, 2015 at 12:23:47PM -0700, Andy Lutomirski wrote:
>> The modify_ldt syscall exposes a large attack surface and is
>> unnecessary for modern userspace.  Make it optional.
>
> Wouldn't you prefer something like this which makes it possible to re-enable
> it at runtime so that we can hope distros ship with it disabled by default ?
>
> It's pretty efficient on your ldtgdt testcase :
>
> # echo 1 > /proc/sys/kernel/modify_ldt
> # ./a.out
> [OK]    LDT entry 0 has AR 0x0040FA00 and limit 0x0000000A
> [OK]    LDT entry 0 has AR 0x00C0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 1 is invalid
> [OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 1 is invalid
> [OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07A00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00907A00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07200 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07000 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07400 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00507600 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507E00 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507C00 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507A00 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
> [RUN]   Test fork
> [OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
> [OK]    LDT entry 1 is invalid
> [OK]    LDT entry 0 has AR 0x0040FA00 and limit 0x0000000A
> [OK]    LDT entry 0 has AR 0x00C0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 1 is invalid
> [OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 1 is invalid
> [OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D0FA00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07A00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00907A00 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07200 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07000 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00D07400 and limit 0x0000AFFF
> [OK]    LDT entry 2 has AR 0x00507600 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507E00 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507C00 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507A00 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
> [OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
> [RUN]   Test fork
> [OK]    Child succeeded
> [OK]    modify_ldt failure 22
> [OK]    LDT entry 0 has AR 0x0000F200 and limit 0x00000000
> [OK]    LDT entry 0 has AR 0x00007200 and limit 0x00000000
> [OK]    LDT entry 0 has AR 0x0000F000 and limit 0x00000000
> [OK]    LDT entry 0 has AR 0x00007200 and limit 0x00000000
> [OK]    LDT entry 0 has AR 0x00007000 and limit 0x00000001
> [OK]    LDT entry 0 has AR 0x00007000 and limit 0x00000000
> [OK]    LDT entry 0 is invalid
> [OK]    LDT entry 0 has AR 0x0040F200 and limit 0x00000000
> [OK]    LDT entry 0 is invalid
> [SKIP]  Cannot set affinity to CPU 1
>
>
> # echo 0 > /proc/sys/kernel/modify_ldt
> # ./a.out
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    LDT entry 1 is invalid
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    LDT entry 1 is invalid
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [SKIP]  Skipping fork test because have no LDT
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [OK]    modify_ldt is returned -ENOSYS
> [SKIP]  Cannot set affinity to CPU 1
>
> The patch is quite small (I stole your comment for the config option).
>
> Willy
>
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 226d569..b926f65 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1012,6 +1012,23 @@ config X86_16BIT
>           this option saves about 300 bytes on i386, or around 6K text
>           plus 16K runtime memory on x86-64,
>
> +config DEFAULT_MODIFY_LDT_SYSCALL
> +       bool "Allow userspace to modify the LDT (local descriptor table)"
> +       default y
> +       ---help---
> +         Linux can allow user programs to install a per-process x86
> +         Local Descriptor Table (LDT) using the modify_ldt(2) system
> +         call.  This is required to run 16-bit or segmented code such as
> +         DOSEMU or some Wine programs.  It is also used by some very old
> +         threading libraries.
> +
> +         Enabling this feature increases the low-level kernel attack
> +         surface. Disabling it disables the modify_ldt(2) system call by
> +         default. Note that even when disabled it remains possible to
> +         enable it at runtime by setting the sys.kernel.modify_ldt sysctl.
> +
> +         Say 'N' here if you don't expect to use DOSEMU or Wine often.
> +
>  config X86_ESPFIX32
>         def_bool y
>         depends on X86_16BIT && X86_32
> diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
> index c37886d..2f10b6c 100644
> --- a/arch/x86/kernel/ldt.c
> +++ b/arch/x86/kernel/ldt.c
> @@ -20,6 +20,12 @@
>  #include <asm/mmu_context.h>
>  #include <asm/syscalls.h>
>
> +#ifdef CONFIG_DEFAULT_MODIFY_LDT_SYSCALL
> +int sysctl_modify_ldt __read_mostly = 1;
> +#else
> +int sysctl_modify_ldt __read_mostly = 0;
> +#endif
> +
>  #ifdef CONFIG_SMP
>  static void flush_ldt(void *current_mm)
>  {
> @@ -254,6 +260,9 @@ asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>  {
>         int ret = -ENOSYS;
>
> +       if (!sysctl_modify_ldt)
> +               return ret;
> +
>         switch (func) {
>         case 0:
>                 ret = read_ldt(ptr, bytecount);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 2082b1a..60270c6 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -111,6 +111,9 @@ extern int sysctl_nr_open_min, sysctl_nr_open_max;
>  #ifndef CONFIG_MMU
>  extern int sysctl_nr_trim_pages;
>  #endif
> +#ifdef CONFIG_X86
> +extern int sysctl_modify_ldt;
> +#endif
>
>  /* Constants used for minimum and  maximum */
>  #ifdef CONFIG_LOCKUP_DETECTOR
> @@ -962,6 +965,13 @@ static struct ctl_table kern_table[] = {
>                 .mode           = 0644,
>                 .proc_handler   = proc_dointvec,
>         },
> +       {
> +               .procname       = "modify_ldt",
> +               .data           = &sysctl_modify_ldt,
> +               .maxlen         = sizeof(int),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dointvec,
> +       },
>  #endif
>  #if defined(CONFIG_MMU)
>         {

I've been pondering something like this that is even MORE generic, for
any syscall. Something like a "syscalls" directory under
/proc/sys/kernel, with 1 entry per syscall. "0" is "available", "1" is
disabled, and "-1" disabled until next boot.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191406

On Thu, Jul 23, 2015 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Jul 23, 2015 at 3:24 AM, Willy Tarreau <w@1wt.eu> wrote:
>>  #ifdef CONFIG_SMP
>>  static void flush_ldt(void *current_mm)
>>  {
>> @@ -254,6 +260,9 @@ asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>>  {
>>         int ret = -ENOSYS;
>>
>> +       if (!sysctl_modify_ldt)
>> +               return ret;
>> +
>>         switch (func) {
>>         case 0:
>>                 ret = read_ldt(ptr, bytecount);
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 2082b1a..60270c6 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -111,6 +111,9 @@ extern int sysctl_nr_open_min, sysctl_nr_open_max;
>>  #ifndef CONFIG_MMU
>>  extern int sysctl_nr_trim_pages;
>>  #endif
>> +#ifdef CONFIG_X86
>> +extern int sysctl_modify_ldt;
>> +#endif
>>
>>  /* Constants used for minimum and  maximum */
>>  #ifdef CONFIG_LOCKUP_DETECTOR
>> @@ -962,6 +965,13 @@ static struct ctl_table kern_table[] = {
>>                 .mode           = 0644,
>>                 .proc_handler   = proc_dointvec,
>>         },
>> +       {
>> +               .procname       = "modify_ldt",
>> +               .data           = &sysctl_modify_ldt,
>> +               .maxlen         = sizeof(int),
>> +               .mode           = 0644,
>> +               .proc_handler   = proc_dointvec,
>> +       },
>>  #endif
>>  #if defined(CONFIG_MMU)
>>         {
>
> I've been pondering something like this that is even MORE generic, for
> any syscall. Something like a "syscalls" directory under
> /proc/sys/kernel, with 1 entry per syscall. "0" is "available", "1" is
> disabled, and "-1" disabled until next boot.
>

It might want to be /proc/sys/kernel/syscalls/[abi]/[name], possibly
with more than just those options.  We might want "disabled, returns
ENOSYS", "disabled, returns EPERM", and a lock bit.

On x86 at least, the implementation's easy -- we can just poke the
syscall table.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191407

On Thu, Jul 23, 2015 at 04:40:14PM -0700, Andy Lutomirski wrote:
> On Thu, Jul 23, 2015 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
> > I've been pondering something like this that is even MORE generic, for
> > any syscall. Something like a "syscalls" directory under
> > /proc/sys/kernel, with 1 entry per syscall. "0" is "available", "1" is
> > disabled, and "-1" disabled until next boot.
> >
> 
> It might want to be /proc/sys/kernel/syscalls/[abi]/[name], possibly
> with more than just those options.  We might want "disabled, returns
> ENOSYS", "disabled, returns EPERM", and a lock bit.
> 
> On x86 at least, the implementation's easy -- we can just poke the
> syscall table.

I wouldn't do it these days. Around 2000-2001, with a friend we designed
a module with its userland counterpart which was called "overloader". The
principle was to intercept syscalls in order to enforce some form of
policies, log values, or remap paths, etc. The first use was to log all
file creations during a "make install" to more easily build packages. It
was at the era where it was easy to modify the syscall table from a module,
in kernel 2.2.

We quickly found that beyond logging/rewriting syscall arguments, it had
limited use cases when used as a "syscall firewall" because many syscalls
are still too coarse to decide whether you want to enable/disable them.
I remember that socketcall() and ioctl() were among the annoying ones.
Either you totally enable or totally disable. In the end, the only valid
use cases we found for enabling/disabling a syscall were limited to a very
small set for debugging purposes, in order to force some application code
to detect a missing implementation and switch to an alternative (eg: these
days if you suspect a bug in epoll you could disable it and force the app
to use poll instead). It was still useful to disable module loading and
FS mounting but that was about all by then.

All this to say that probably only a handful of tricky syscalls would
need an on/off switch but clearly not all of them at all, so I'd rather
add a few entries just for the relevant ones, mainly to fix compatibility
issues and nothing more. Eg: what's the point of disabling exit(), wait(),
kill(), fork() or getpid()... It would only increase the difficulty to
sort out bug reports.

Just my opinion,
Willy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191408

On Thu, Jul 23, 2015 at 4:58 PM, Willy Tarreau <w@1wt.eu> wrote:
> On Thu, Jul 23, 2015 at 04:40:14PM -0700, Andy Lutomirski wrote:
>> On Thu, Jul 23, 2015 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
>> > I've been pondering something like this that is even MORE generic, for
>> > any syscall. Something like a "syscalls" directory under
>> > /proc/sys/kernel, with 1 entry per syscall. "0" is "available", "1" is
>> > disabled, and "-1" disabled until next boot.
>> >
>>
>> It might want to be /proc/sys/kernel/syscalls/[abi]/[name], possibly
>> with more than just those options.  We might want "disabled, returns
>> ENOSYS", "disabled, returns EPERM", and a lock bit.
>>
>> On x86 at least, the implementation's easy -- we can just poke the
>> syscall table.
>
> I wouldn't do it these days. Around 2000-2001, with a friend we designed
> a module with its userland counterpart which was called "overloader". The
> principle was to intercept syscalls in order to enforce some form of
> policies, log values, or remap paths, etc. The first use was to log all
> file creations during a "make install" to more easily build packages. It
> was at the era where it was easy to modify the syscall table from a module,
> in kernel 2.2.
>
> We quickly found that beyond logging/rewriting syscall arguments, it had
> limited use cases when used as a "syscall firewall" because many syscalls
> are still too coarse to decide whether you want to enable/disable them.
> I remember that socketcall() and ioctl() were among the annoying ones.
> Either you totally enable or totally disable. In the end, the only valid
> use cases we found for enabling/disabling a syscall were limited to a very
> small set for debugging purposes, in order to force some application code
> to detect a missing implementation and switch to an alternative (eg: these
> days if you suspect a bug in epoll you could disable it and force the app
> to use poll instead). It was still useful to disable module loading and
> FS mounting but that was about all by then.
>
> All this to say that probably only a handful of tricky syscalls would
> need an on/off switch but clearly not all of them at all, so I'd rather
> add a few entries just for the relevant ones, mainly to fix compatibility
> issues and nothing more. Eg: what's the point of disabling exit(), wait(),
> kill(), fork() or getpid()... It would only increase the difficulty to
> sort out bug reports.
>
> Just my opinion,

Well, I would really like to have something like this around so that I
can trivially globally disable syscalls when they have security risks.
My hack[1] to disable kexec_load, for example, was terrible while I
waited for a kernel that supported the disable_kexec_load sysctl.

-Kees

[1] https://outflux.net/blog/archives/2013/12/10/live-patching-the-kernel/

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191572

On Thu, Jul 23, 2015 at 05:09:21PM -0700, Kees Cook wrote:
> > All this to say that probably only a handful of tricky syscalls would
> > need an on/off switch but clearly not all of them at all, so I'd rather
> > add a few entries just for the relevant ones, mainly to fix compatibility
> > issues and nothing more. Eg: what's the point of disabling exit(), wait(),
> > kill(), fork() or getpid()... It would only increase the difficulty to
> > sort out bug reports.
> >
> > Just my opinion,
> 
> Well, I would really like to have something like this around so that I
> can trivially globally disable syscalls when they have security risks.

I understand, but while maybe it could make sense to have the option on
any linux-specific syscall, having it on the standard, portable ones
will be useless as disabling them will break most applications.

> My hack[1] to disable kexec_load, for example, was terrible while I
> waited for a kernel that supported the disable_kexec_load sysctl.

This typically is one linux-specific syscall which no regular application
would rely on and which can come with side effects. I think there are not
*that* many, none of them is performance-critical, and they'd rather be
dealt with one at a time.

> [1] https://outflux.net/blog/archives/2013/12/10/live-patching-the-kernel/

Thanks, that (and the linked articles) was an interesting read.

Willy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191580

On Fri, Jul 24, 2015 at 09:24:51AM +0200, Willy Tarreau wrote:
> On Thu, Jul 23, 2015 at 05:09:21PM -0700, Kees Cook wrote:
> > > All this to say that probably only a handful of tricky syscalls would
> > > need an on/off switch but clearly not all of them at all, so I'd rather
> > > add a few entries just for the relevant ones, mainly to fix compatibility
> > > issues and nothing more. Eg: what's the point of disabling exit(), wait(),
> > > kill(), fork() or getpid()... It would only increase the difficulty to
> > > sort out bug reports.
> > >
> > > Just my opinion,
> > 
> > Well, I would really like to have something like this around so that I
> > can trivially globally disable syscalls when they have security risks.
> 
> I understand, but while maybe it could make sense to have the option on
> any linux-specific syscall, having it on the standard, portable ones
> will be useless as disabling them will break most applications.
> 
> > My hack[1] to disable kexec_load, for example, was terrible while I
> > waited for a kernel that supported the disable_kexec_load sysctl.
> 
> This typically is one linux-specific syscall which no regular application
> would rely on and which can come with side effects. I think there are not
> *that* many, none of them is performance-critical, and they'd rather be
> dealt with one at a time.

Looking at syscall_64.tbl, I'm seeing that the first ~133 syscalls
have no reason for being disabled if we don't want to break portable
applications, with the exception of ptrace I guess. Past this, things
like uselib, personality, sysfs, prctl etc... could be disabled. There
are still some exceptions in this area but I don't see them as critical
if someone would accidently disable them (eg: getpriority, mlock, ...).
Others like chroot, setrlimit, adjtimex, settimeofday, mount, umount,
time need to be kept. And a few ones like sync or sethostname would be
nice to have optional in order to lock down a system at boot. Many of
the other ones are ns-specific versions of the first ones (*at) and
would rather not being made optional either. I think maybe we can find
between 10 and 30 that would make sense to optionally disable.

Willy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web