[v3,00/19] Improve loader environment variable handling

  The recent CVE-2023-4911 fix [1] and tunable change to SXID_ERASE
discussion [2] brought some issues with the current environment handling
by the loader. Besides the bugs in tuning parsing, some other questions
are:

  * What should be the security boundaries for tunable and other tuning
    environment variables?

  * Should tunables be filtered out or be disabled altogether in setuid
    binaries [3]?

  * How should ld.so handle security-sensitive tunable (like malloc
    options)?

  * How to handle ill-formatted tunable definition [4]?

  * Is tunable copy/parsing (through tunable_strdup) required [5]?

  * Which other environment variables the loader should ignore and/or
    filter in security-sensitive context.

On this patchset, I followed the idea laid out in the discussion on
whether to apply SXID_ERASE to all tunables [6]:

  1. ignore any tunable on AT_SECURE binaries (as some Linux distributions
      do already [7]);

  2. Add malloc tunables along with GLIBC_TUNABLES to unsecvars;

  3. Do not parse ill-formatted GLIBC_TUNABLES strings;

  4. Remove the requirement of duplicating the GLIBC_TUNABLES string for
     parsing.

  5. Ignore most of the environment variables on security-sensitive
     mode (AT_SECURE/setuid/setgid).

Patch #1 removes '/etc/suid-debug', which has not been working since
malloc debugging supported moved to libc_malloc_debug.so. It is one
thing less that might change AT_SECURE binaries' behavior
due to environment configurations.

Patch #2 removed tunables parsing and applying for setuid/setgid
binaries (similar to Alt Linux patch).

Patch #3 and #4 add all malloc tunable and GLIBC_TUNABLES to unsecvars
and improve tst-env-setuid.c to test all possible environment variables.

Patch #5 and #6 improved the GLIBC_TUNABLES handling to avoid handling
ill-formatted inputs.

Patch #7 makes _dl_debug_vdprintf usable before self-relocation so patch
#8 can add a loader warning that ill-formatted GLIBC_TUNABLES inputs are
ignored (it also fixes the issue where the GLIBC_TUNABLE allocation
failure will trigger a SEGFAULT on some architecture for PIE).

Patch #9, #10, and #11 remove the tunable_strdup and make the
GLIBC_TUNABLE parsing in place (no more possible allocation failure).
The parsing now tracks the tunable start and its size. The
dl-tunable-parse.h adds helper functions to help to parse, like an
strcmp that also checks for size and an iterator for suboptions that are
comma-separated (used on hwcap parsing by x86, powerpc, and s390x).

Patch #12, #13, #14, #16, and #18 make loader ignore all but just
LD_PRELOAD and LD_AUDIT for setuid binaries.  For both options, loader
ensures that pathnames containing slashes are ignored and shared
libraries are loaded only from the standard search directories and only
if they have set-user-ID mode bit enabled.

[1] https://sourceware.org/pipermail/libc-alpha/2023-October/151921.html
[2] https://sourceware.org/pipermail/libc-alpha/2023-October/151936.html
[3] https://www.openwall.com/lists/oss-security/2023/10/03/3
[4] https://sourceware.org/pipermail/libc-alpha/2023-October/151927.html
[5] https://sourceware.org/pipermail/libc-alpha/2023-October/151959.html
[6] https://sourceware.org/pipermail/libc-alpha/2023-October/152011.html
[7] https://git.altlinux.org/gears/g/glibc.git?p=glibc.git;a=commitdiff;h=5d1686416ab766f3dd0780ab730650c4c0f76ca9

Changes from v2:
* Extend tst-tunables with tunables aliases tests.
* Use warning instead of an error to indicate invalid tunables.
* Fixed tunable_initialize for string aliases.
Changes from v1:
* Ignore most of the environment variables on security-sensitive mode.
* Extend tests.

Adhemerval Zanella (19):
  elf: Remove /etc/suid-debug support
  elf: Add GLIBC_TUNABLES to unsecvars
  elf: Ignore GLIBC_TUNABLES for setuid/setgid binaries
  elf: Add all malloc tunable to unsecvars
  elf: Do not process invalid tunable format
  elf: Do not parse ill-formatted strings
  elf: Fix _dl_debug_vdprintf to work before self-relocation
  elf: Emit warning if tunable is ill-formatted
  x86: Use dl-symbol-redir-ifunc.h on cpu-tunables
  s390: Use dl-symbol-redir-ifunc.h on cpu-tunables
  elf: Do not duplicate the GLIBC_TUNABLES string
  elf: Ignore LD_PROFILE for setuid binaries
  elf: Remove LD_PROFILE for static binaries
  elf: Ignore loader debug env vars for setuid
  elf: Remove any_debug from dl_main_state
  elf: Ignore LD_LIBRARY_PATH and debug env var for setuid for static
  elf: Add comments on how LD_AUDIT and LD_PRELOAD handle
    __libc_enable_secure
  elf: Ignore LD_BIND_NOW and LD_BIND_NOT for setuid binaries
  elf: Refactor process_envvars

 elf/Makefile                                  |  26 +-
 elf/dl-load.c                                 |  10 +-
 elf/dl-main.h                                 |   3 -
 elf/dl-printf.c                               |  16 +-
 elf/dl-runtime.c                              |  12 +-
 elf/dl-support.c                              |  41 +--
 elf/dl-tunable-types.h                        |  10 -
 elf/dl-tunables.c                             | 219 ++++--------
 elf/dl-tunables.h                             |   6 +-
 elf/dl-tunables.list                          |  17 -
 elf/{dl-profstub.c => libc-dl-profstub.c}     |   0
 elf/rtld.c                                    | 122 +++++--
 elf/tst-env-setuid-static.c                   |   1 +
 elf/tst-env-setuid-tunables.c                 |  59 ++--
 elf/tst-env-setuid.c                          | 140 +++++---
 elf/tst-tunables.c                            | 321 ++++++++++++++++++
 include/dlfcn.h                               |   5 +
 manual/README.tunables                        |   9 -
 manual/memory.texi                            |   4 +-
 manual/tunables.texi                          |   4 +-
 scripts/gen-tunables.awk                      |  18 +-
 stdio-common/Makefile                         |   5 +
 stdio-common/_itoa.c                          |   5 +
 sysdeps/aarch64/dl-machine.h                  |   4 +-
 sysdeps/aarch64/dl-trampoline.S               |   2 +-
 sysdeps/alpha/dl-machine.h                    |   6 +-
 sysdeps/alpha/dl-trampoline.S                 |   4 +
 sysdeps/arm/dl-machine.h                      |   4 +-
 sysdeps/arm/dl-trampoline.S                   |   2 +-
 sysdeps/generic/dl-tunables-parse.h           | 129 +++++++
 sysdeps/generic/unsecvars.h                   |  10 +
 sysdeps/hppa/dl-machine.h                     |  36 +-
 sysdeps/hppa/dl-trampoline.S                  |   2 +
 sysdeps/i386/dl-machine.h                     |   2 +
 sysdeps/i386/dl-trampoline.S                  |   2 +-
 .../i686/multiarch/dl-symbol-redir-ifunc.h    |   5 +
 sysdeps/ia64/dl-machine.h                     |  10 +-
 sysdeps/ia64/dl-trampoline.S                  |   2 +-
 sysdeps/loongarch/dl-machine.h                |   6 +-
 sysdeps/loongarch/dl-trampoline.h             |   2 +
 sysdeps/m68k/dl-machine.h                     |   4 +-
 sysdeps/m68k/dl-trampoline.S                  |   2 +
 sysdeps/powerpc/powerpc32/dl-machine.c        |   2 +-
 sysdeps/powerpc/powerpc32/dl-machine.h        |  10 +-
 sysdeps/powerpc/powerpc32/dl-trampoline.S     |   2 +-
 sysdeps/powerpc/powerpc64/dl-machine.h        |  20 +-
 sysdeps/powerpc/powerpc64/dl-trampoline.S     |   2 +-
 sysdeps/s390/cpu-features.c                   | 169 ++++-----
 .../s390/multiarch/dl-symbol-redir-ifunc.h    |   2 +
 sysdeps/s390/s390-32/dl-machine.h             |   8 +-
 sysdeps/s390/s390-32/dl-trampoline.h          |   2 +-
 sysdeps/s390/s390-64/dl-machine.h             |   8 +-
 sysdeps/s390/s390-64/dl-trampoline.h          |   2 +-
 sysdeps/sh/dl-machine.h                       |   2 +
 sysdeps/sh/dl-trampoline.S                    |   2 +
 sysdeps/sparc/sparc32/dl-machine.h            |   4 +-
 sysdeps/sparc/sparc32/dl-trampoline.S         |   2 +
 sysdeps/sparc/sparc64/dl-machine.h            |   4 +-
 sysdeps/sparc/sparc64/dl-trampoline.S         |   2 +
 .../unix/sysv/linux/aarch64/cpu-features.c    |  38 ++-
 .../sysv/linux/i386/dl-writev.h}              |  18 +-
 .../unix/sysv/linux/powerpc/cpu-features.c    |  45 +--
 .../sysv/linux/powerpc/tst-hwcap-tunables.c   |   6 +-
 sysdeps/x86/Makefile                          |   4 +-
 sysdeps/x86/cpu-tunables.c                    | 135 +++-----
 sysdeps/x86/tst-hwcap-tunables.c              | 148 ++++++++
 sysdeps/x86_64/64/dl-tunables.list            |   1 -
 sysdeps/x86_64/dl-machine.h                   |   2 +
 sysdeps/x86_64/dl-trampoline.S                |  64 ++--
 .../x86_64/multiarch/dl-symbol-redir-ifunc.h  |  15 +
 70 files changed, 1281 insertions(+), 725 deletions(-)
 rename elf/{dl-profstub.c => libc-dl-profstub.c} (100%)
 create mode 100644 elf/tst-env-setuid-static.c
 create mode 100644 elf/tst-tunables.c
 create mode 100644 sysdeps/generic/dl-tunables-parse.h
 rename sysdeps/{x86_64/memcmp-isa-default-impl.h => unix/sysv/linux/i386/dl-writev.h} (62%)
 create mode 100644 sysdeps/x86/tst-hwcap-tunables.c
  

On 2023-11-06 15:25, Adhemerval Zanella wrote:
> The recent CVE-2023-4911 fix [1] and tunable change to SXID_ERASE
> discussion [2] brought some issues with the current environment handling
> by the loader. Besides the bugs in tuning parsing, some other questions
> are:

Overall I think 1-10 are ready to be committed since they handle a full 
block of work, i.e. tunables validation.  I know 11 kinda belongs in 
that block, but it would be nice to reduce this set to 10 from 19 for 
the next version :)

Also maybe push the independent patches in 11-19 that have R-b already.

Thanks,
Sid

> 
>    * What should be the security boundaries for tunable and other tuning
>      environment variables?
> 
>    * Should tunables be filtered out or be disabled altogether in setuid
>      binaries [3]?
> 
>    * How should ld.so handle security-sensitive tunable (like malloc
>      options)?
> 
>    * How to handle ill-formatted tunable definition [4]?
> 
>    * Is tunable copy/parsing (through tunable_strdup) required [5]?
> 
>    * Which other environment variables the loader should ignore and/or
>      filter in security-sensitive context.
> 
> On this patchset, I followed the idea laid out in the discussion on
> whether to apply SXID_ERASE to all tunables [6]:
> 
>    1. ignore any tunable on AT_SECURE binaries (as some Linux distributions
>        do already [7]);
> 
>    2. Add malloc tunables along with GLIBC_TUNABLES to unsecvars;
> 
>    3. Do not parse ill-formatted GLIBC_TUNABLES strings;
> 
>    4. Remove the requirement of duplicating the GLIBC_TUNABLES string for
>       parsing.
> 
>    5. Ignore most of the environment variables on security-sensitive
>       mode (AT_SECURE/setuid/setgid).
> 
> Patch #1 removes '/etc/suid-debug', which has not been working since
> malloc debugging supported moved to libc_malloc_debug.so. It is one
> thing less that might change AT_SECURE binaries' behavior
> due to environment configurations.
> 
> Patch #2 removed tunables parsing and applying for setuid/setgid
> binaries (similar to Alt Linux patch).
> 
> Patch #3 and #4 add all malloc tunable and GLIBC_TUNABLES to unsecvars
> and improve tst-env-setuid.c to test all possible environment variables.
> 
> Patch #5 and #6 improved the GLIBC_TUNABLES handling to avoid handling
> ill-formatted inputs.
> 
> Patch #7 makes _dl_debug_vdprintf usable before self-relocation so patch
> #8 can add a loader warning that ill-formatted GLIBC_TUNABLES inputs are
> ignored (it also fixes the issue where the GLIBC_TUNABLE allocation
> failure will trigger a SEGFAULT on some architecture for PIE).
> 
> Patch #9, #10, and #11 remove the tunable_strdup and make the
> GLIBC_TUNABLE parsing in place (no more possible allocation failure).
> The parsing now tracks the tunable start and its size. The
> dl-tunable-parse.h adds helper functions to help to parse, like an
> strcmp that also checks for size and an iterator for suboptions that are
> comma-separated (used on hwcap parsing by x86, powerpc, and s390x).
> 
> Patch #12, #13, #14, #16, and #18 make loader ignore all but just
> LD_PRELOAD and LD_AUDIT for setuid binaries.  For both options, loader
> ensures that pathnames containing slashes are ignored and shared
> libraries are loaded only from the standard search directories and only
> if they have set-user-ID mode bit enabled.
> 
> [1] https://sourceware.org/pipermail/libc-alpha/2023-October/151921.html
> [2] https://sourceware.org/pipermail/libc-alpha/2023-October/151936.html
> [3] https://www.openwall.com/lists/oss-security/2023/10/03/3
> [4] https://sourceware.org/pipermail/libc-alpha/2023-October/151927.html
> [5] https://sourceware.org/pipermail/libc-alpha/2023-October/151959.html
> [6] https://sourceware.org/pipermail/libc-alpha/2023-October/152011.html
> [7] https://git.altlinux.org/gears/g/glibc.git?p=glibc.git;a=commitdiff;h=5d1686416ab766f3dd0780ab730650c4c0f76ca9
> 
> Changes from v2:
> * Extend tst-tunables with tunables aliases tests.
> * Use warning instead of an error to indicate invalid tunables.
> * Fixed tunable_initialize for string aliases.
> Changes from v1:
> * Ignore most of the environment variables on security-sensitive mode.
> * Extend tests.
> 
> Adhemerval Zanella (19):
>    elf: Remove /etc/suid-debug support
>    elf: Add GLIBC_TUNABLES to unsecvars
>    elf: Ignore GLIBC_TUNABLES for setuid/setgid binaries
>    elf: Add all malloc tunable to unsecvars
>    elf: Do not process invalid tunable format
>    elf: Do not parse ill-formatted strings
>    elf: Fix _dl_debug_vdprintf to work before self-relocation
>    elf: Emit warning if tunable is ill-formatted
>    x86: Use dl-symbol-redir-ifunc.h on cpu-tunables
>    s390: Use dl-symbol-redir-ifunc.h on cpu-tunables
>    elf: Do not duplicate the GLIBC_TUNABLES string
>    elf: Ignore LD_PROFILE for setuid binaries
>    elf: Remove LD_PROFILE for static binaries
>    elf: Ignore loader debug env vars for setuid
>    elf: Remove any_debug from dl_main_state
>    elf: Ignore LD_LIBRARY_PATH and debug env var for setuid for static
>    elf: Add comments on how LD_AUDIT and LD_PRELOAD handle
>      __libc_enable_secure
>    elf: Ignore LD_BIND_NOW and LD_BIND_NOT for setuid binaries
>    elf: Refactor process_envvars
> 
>   elf/Makefile                                  |  26 +-
>   elf/dl-load.c                                 |  10 +-
>   elf/dl-main.h                                 |   3 -
>   elf/dl-printf.c                               |  16 +-
>   elf/dl-runtime.c                              |  12 +-
>   elf/dl-support.c                              |  41 +--
>   elf/dl-tunable-types.h                        |  10 -
>   elf/dl-tunables.c                             | 219 ++++--------
>   elf/dl-tunables.h                             |   6 +-
>   elf/dl-tunables.list                          |  17 -
>   elf/{dl-profstub.c => libc-dl-profstub.c}     |   0
>   elf/rtld.c                                    | 122 +++++--
>   elf/tst-env-setuid-static.c                   |   1 +
>   elf/tst-env-setuid-tunables.c                 |  59 ++--
>   elf/tst-env-setuid.c                          | 140 +++++---
>   elf/tst-tunables.c                            | 321 ++++++++++++++++++
>   include/dlfcn.h                               |   5 +
>   manual/README.tunables                        |   9 -
>   manual/memory.texi                            |   4 +-
>   manual/tunables.texi                          |   4 +-
>   scripts/gen-tunables.awk                      |  18 +-
>   stdio-common/Makefile                         |   5 +
>   stdio-common/_itoa.c                          |   5 +
>   sysdeps/aarch64/dl-machine.h                  |   4 +-
>   sysdeps/aarch64/dl-trampoline.S               |   2 +-
>   sysdeps/alpha/dl-machine.h                    |   6 +-
>   sysdeps/alpha/dl-trampoline.S                 |   4 +
>   sysdeps/arm/dl-machine.h                      |   4 +-
>   sysdeps/arm/dl-trampoline.S                   |   2 +-
>   sysdeps/generic/dl-tunables-parse.h           | 129 +++++++
>   sysdeps/generic/unsecvars.h                   |  10 +
>   sysdeps/hppa/dl-machine.h                     |  36 +-
>   sysdeps/hppa/dl-trampoline.S                  |   2 +
>   sysdeps/i386/dl-machine.h                     |   2 +
>   sysdeps/i386/dl-trampoline.S                  |   2 +-
>   .../i686/multiarch/dl-symbol-redir-ifunc.h    |   5 +
>   sysdeps/ia64/dl-machine.h                     |  10 +-
>   sysdeps/ia64/dl-trampoline.S                  |   2 +-
>   sysdeps/loongarch/dl-machine.h                |   6 +-
>   sysdeps/loongarch/dl-trampoline.h             |   2 +
>   sysdeps/m68k/dl-machine.h                     |   4 +-
>   sysdeps/m68k/dl-trampoline.S                  |   2 +
>   sysdeps/powerpc/powerpc32/dl-machine.c        |   2 +-
>   sysdeps/powerpc/powerpc32/dl-machine.h        |  10 +-
>   sysdeps/powerpc/powerpc32/dl-trampoline.S     |   2 +-
>   sysdeps/powerpc/powerpc64/dl-machine.h        |  20 +-
>   sysdeps/powerpc/powerpc64/dl-trampoline.S     |   2 +-
>   sysdeps/s390/cpu-features.c                   | 169 ++++-----
>   .../s390/multiarch/dl-symbol-redir-ifunc.h    |   2 +
>   sysdeps/s390/s390-32/dl-machine.h             |   8 +-
>   sysdeps/s390/s390-32/dl-trampoline.h          |   2 +-
>   sysdeps/s390/s390-64/dl-machine.h             |   8 +-
>   sysdeps/s390/s390-64/dl-trampoline.h          |   2 +-
>   sysdeps/sh/dl-machine.h                       |   2 +
>   sysdeps/sh/dl-trampoline.S                    |   2 +
>   sysdeps/sparc/sparc32/dl-machine.h            |   4 +-
>   sysdeps/sparc/sparc32/dl-trampoline.S         |   2 +
>   sysdeps/sparc/sparc64/dl-machine.h            |   4 +-
>   sysdeps/sparc/sparc64/dl-trampoline.S         |   2 +
>   .../unix/sysv/linux/aarch64/cpu-features.c    |  38 ++-
>   .../sysv/linux/i386/dl-writev.h}              |  18 +-
>   .../unix/sysv/linux/powerpc/cpu-features.c    |  45 +--
>   .../sysv/linux/powerpc/tst-hwcap-tunables.c   |   6 +-
>   sysdeps/x86/Makefile                          |   4 +-
>   sysdeps/x86/cpu-tunables.c                    | 135 +++-----
>   sysdeps/x86/tst-hwcap-tunables.c              | 148 ++++++++
>   sysdeps/x86_64/64/dl-tunables.list            |   1 -
>   sysdeps/x86_64/dl-machine.h                   |   2 +
>   sysdeps/x86_64/dl-trampoline.S                |  64 ++--
>   .../x86_64/multiarch/dl-symbol-redir-ifunc.h  |  15 +
>   70 files changed, 1281 insertions(+), 725 deletions(-)
>   rename elf/{dl-profstub.c => libc-dl-profstub.c} (100%)
>   create mode 100644 elf/tst-env-setuid-static.c
>   create mode 100644 elf/tst-tunables.c
>   create mode 100644 sysdeps/generic/dl-tunables-parse.h
>   rename sysdeps/{x86_64/memcmp-isa-default-impl.h => unix/sysv/linux/i386/dl-writev.h} (62%)
>   create mode 100644 sysdeps/x86/tst-hwcap-tunables.c
>
  

On 20/11/23 20:12, Siddhesh Poyarekar wrote:
> 
> 
> On 2023-11-06 15:25, Adhemerval Zanella wrote:
>> The recent CVE-2023-4911 fix [1] and tunable change to SXID_ERASE
>> discussion [2] brought some issues with the current environment handling
>> by the loader. Besides the bugs in tuning parsing, some other questions
>> are:
> 
> Overall I think 1-10 are ready to be committed since they handle a full block of work, i.e. tunables validation.  I know 11 kinda belongs in that block, but it would be nice to reduce this set to 10 from 19 for the next version :)
> 
> Also maybe push the independent patches in 11-19 that have R-b already.

Alright, I will install the approved patches and send a newer version with
the changes requested.