Add cheap defense mechanisms #1171

stoeckmann · 2025-01-06T16:35:06Z

Before you read any further: No reachable issues exist in current code base (as far as I can tell)

But these defense mechanisms would have made reviewing easier because I could have skipped a few searches:

Use _exit instead of exit after execution errors

After a fork and a failing exec, calling exit is not the best choice. Use _exit instead because tools like groupmod etc. use add_cleanup to clean up when exit is called. If the child, which inherits these instructions, calls exit by itself, these cleanup routines could release passwd/group locks too early. Again: Does not happen, because the code launches sub processes only after these cleanup routines have been already released again. Yet, better be safe than sorry.

Set close on exec on critical file descriptors

Even though fopen is used in lib/gshadow.c and lib/shadow.c, we can add the "e" mode to trigger O_CLOEXEC for systems which support this glibc extension. The file descriptor which keeps /etc/gshadow or /etc/shadow open is then guaranteed to not be passed down to a sub process. Again: Does not happen. The code carefully closes these file descriptors through dedicated function calls before sub processes are launched. Yet, better be safe than sorry again.

alejandro-colomar · 2025-01-06T22:42:50Z

It would be interesting to patch the fork(2) manual page to do the same thing in the EXAMPLES section. Would you mind sending a patch to linux-man@ too?

alejandro-colomar

LGTM. Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

Calling exit might trigger cleanup functions registered through atexit. Since some programs use this mechanism, be extra cautious to never release passwd/group locks too early. Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>

With glibc we can use "e" in mode argument to set O_CLOEXEC on opened files. The /etc/shadow and /etc/gshadow file handles should be protected to make sure that they are never passed to child processes by accident. Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>

alejandro-colomar · 2025-01-10T09:23:52Z

Since nobody commented, I'll merge now. Thanks!

stoeckmann · 2025-01-12T15:37:09Z

It would be interesting to patch the fork(2) manual page to do the same thing in the EXAMPLES section. Would you mind sending a patch to linux-man@ too?

I've looked at the EXAMPLES section now, but I'm not sure if it really helps there. No exec call is performed and the whole main-function is printed without any atexit calls. Leaving the forked process with exit in this case is no problem and introducing _exit might make the code less readable or more confusing imho.

alejandro-colomar · 2025-01-12T20:46:25Z

It would be interesting to patch the fork(2) manual page to do the same thing in the EXAMPLES section. Would you mind sending a patch to linux-man@ too?

I've looked at the EXAMPLES section now, but I'm not sure if it really helps there. No exec call is performed and the whole main-function is printed without any atexit calls. Leaving the forked process with exit in this case is no problem and introducing _exit might make the code less readable or more confusing imho.

I guess _exit(2) is never wrong there? Or is it sometimes wrong? If _exit(2) is always better (or at least equivalent), maybe it would be good to show it in the example, to promote it. And in the commit message we could add a detailed explanation.

What do you think?

stoeckmann · 2025-01-12T21:34:04Z

I guess _exit(2) is never wrong there? Or is it sometimes wrong? If _exit(2) is always better (or at least equivalent), maybe it would be good to show it in the example, to promote it. And in the commit message we could add a detailed explanation.

What do you think?

I'll prepare a patch. Promoting that _exit exists might help people out there to at least read its manual page. We can discuss this further on man-pages mailing list if needed.

alejandro-colomar · 2025-01-12T22:41:08Z

I guess _exit(2) is never wrong there? Or is it sometimes wrong? If _exit(2) is always better (or at least equivalent), maybe it would be good to show it in the example, to promote it. And in the commit message we could add a detailed explanation.
What do you think?

I'll prepare a patch. Promoting that _exit exists might help people out there to at least read its manual page. We can discuss this further on man-pages mailing list if needed.

Thanks!

Indeed, I knew it existed, but had only seen it sporadically, and never knew what it was really good for, until you sent this PR. Using it in the manual page will hopefully help people know about it, and give it more visibility.

Karlson2k · 2025-02-17T02:14:57Z

Doesn't use of glibc extensions break functioning with non-glibc, like musl?

Isn't it safer to use constructs like

shadow = fopen (SGROUP_FILE, "re");
if (NULL == shadow )
  shadow = fopen (SGROUP_FILE, "r");

?

Or, alternatively, detect extension in configure?

alejandro-colomar · 2025-02-17T09:41:32Z

Hi,

On Sun, Feb 16, 2025 at 06:15:18PM -0800, Karlson2k wrote: Karlson2k left a comment (shadow-maint/shadow#1171) Doesn't use of glibc extensions break functioning with non-glibc, like musl?

Hmmm, I didn't know musl doesn't support this. It would be interesting to get them to support it. I've CCd several interested parties in this email.

Isn't it safe to use constructs like ``` C shadow = fopen (SGROUP_FILE, "re"); if (NULL == shadow ) shadow = fopen (SGROUP_FILE, "r"); ``` ?

Is 'e' only available in glibc? Do other libraries consciously not support O_CLOEXEC in fopen(3)? I see that POSIX.1-2024 added the 'e' mode string character, so we're using standard features (yeah, very modern ones, but still standard). Is there any reason to not implement them, or is it just a matter of time and contributors? <https://pubs.opengroup.org/onlinepubs/9799919799/functions/fopen.html>

Or, alternatively, detect extension in `configure`?

If we have to... Have a lovely day! Alex

…

-- <https://www.alejandro-colomar.es/>

alejandro-colomar · 2025-02-17T13:46:03Z

Hi,

On Mon, Feb 17, 2025 at 10:42:11AM +0100, Alejandro Colomar wrote:

On Sun, Feb 16, 2025 at 06:15:18PM -0800, Karlson2k wrote:

Karlson2k left a comment (#1171)

Doesn't use of glibc extensions break functioning with non-glibc, like musl?

After reading musl's source code, it does support 'e'. And it does so
since 2012. Which libc are you using? And which version?

        alx@devuan:~/src/musl/libc/master$ grepc fopen .
        ./include/stdio.h:FILE *fopen(const char *__restrict, const char *__restrict);
        ./src/stdio/fopen.c:FILE *fopen(const char *restrict filename, const char *restrict mode)
        {
                FILE *f;
                int fd;
                int flags;

                /* Check for valid initial mode character */
                if (!strchr("rwa", *mode)) {
                        errno = EINVAL;
                        return 0;
                }

                /* Compute the flags to pass to open() */
                flags = __fmodeflags(mode);

                fd = sys_open(filename, flags, 0666);
                if (fd < 0) return 0;
                if (flags & O_CLOEXEC)
                        __syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC);

                f = __fdopen(fd, mode);
                if (f) return f;

                __syscall(SYS_close, fd);
                return 0;
        }

        alx@devuan:~/src/musl/libc/master$ grepc __fmodeflags .
        ./src/stdio/__fmodeflags.c:int __fmodeflags(const char *mode)
        {
                int flags;
                if (strchr(mode, '+')) flags = O_RDWR;
                else if (*mode == 'r') flags = O_RDONLY;
                else flags = O_WRONLY;
                if (strchr(mode, 'x')) flags |= O_EXCL;
                if (strchr(mode, 'e')) flags |= O_CLOEXEC;
                if (*mode != 'r') flags |= O_CREAT;
                if (*mode == 'w') flags |= O_TRUNC;
                if (*mode == 'a') flags |= O_APPEND;
                return flags;
        }
        ./src/internal/stdio_impl.h:hidden int __fmodeflags(const char *);

        alx@devuan:~/src/musl/libc/master$ git blame -- src/stdio/__fmodeflags.c | grep CLOEXEC
        892cafff6 (Rich Felker 2012-10-24 21:16:06 -0400 11)    if (strchr(mode, 'e')) flags |= O_CLOEXEC;

        alx@devuan:~/src/musl/libc/master$ git show 892cafff6 | grep -e CLOEXEC -e ^diff | head -n5
        diff --git a/src/internal/stdio_impl.h b/src/internal/stdio_impl.h
        diff --git a/src/stdio/__fmodeflags.c b/src/stdio/__fmodeflags.c
        +       if (strchr(mode, 'e')) flags |= O_CLOEXEC;
        diff --git a/src/stdio/fopen.c b/src/stdio/fopen.c
        -       if (strchr(mode, 'e')) flags |= O_CLOEXEC;

        alx@devuan:~/src/musl/libc/master$ git blame 892cafff6^ -- src/stdio/fopen.c | grep CLOEXEC
        8582a6e9f (Rich Felker 2012-09-29 18:09:34 -0400 20)    if (strchr(mode, 'e')) flags |= O_CLOEXEC;

        alx@devuan:~/src/musl/libc/master$ git log -1 8582a6e9f
        commit 8582a6e9f25dd7b87d72961f58008052a4cac473
        Author: Rich Felker <dalias@aerifal.cx>
        Date:   Sat Sep 29 18:09:34 2012 -0400

            add 'e' modifier (close-on-exec) to fopen and fdopen

            this feature will be in the next version of POSIX, and can be used
            internally immediately. there are many internal uses of fopen where
            close-on-exec is needed to fix bugs.

Cheers,
Alex

Karlson2k · 2025-02-17T14:47:46Z

On GNU/Linux I'm using glibc mainly, but my comment is not related to my specific use.

The generic approach to writing reliable portable software:

do not use private extension if possible (they are unportable by their nature and could be even removed in new releases)
if private extension is important, test for extension availability before use
do not blindly use standard features if the standard is new, check for feature availability instead
use fallback paths if possible

Talking about this flag. It is supported by glibc, musl and uclibc-ng. Not supported by dietlibc.

alejandro-colomar · 2025-02-17T15:12:48Z

On GNU/Linux I'm using glibc mainly, but my comment is not related to my specific use.

The generic approach to writing reliable portable software:

* do not use private extension if possible (they are unportable by their nature and could be even removed in new releases)

* if private extension is important, test for extension availability before use

* do not blindly use standard features if the standard is new, check for feature availability instead

* use fallback paths if possible

We make compromises between portability and security/safety. For portability reasons, one could argue using the ISO C89 dialect (we even had some fallback code for K&R C compilers until a few weeks ago), but that would result in code that is more unsafe. Fallback code also results in many untested paths, so we avoid it unless its really important.

Since this extension is old (available in glibc since 2007, and in musl since 2012), and it provides important security features, and it has been standardized, I think it's okay to use it.

Even back in 2012, it had already been accepted into the next POSIX standard (which would eventually become POSIX.1-2024), as the commit message from musl documents:

        commit 8582a6e9f25dd7b87d72961f58008052a4cac473
        Author: Rich Felker <dalias@aerifal.cx>
        Date:   Sat Sep 29 18:09:34 2012 -0400

            add 'e' modifier (close-on-exec) to fopen and fdopen

            this feature will be in the next version of POSIX, and can be used
            internally immediately. there are many internal uses of fopen where
            close-on-exec is needed to fix bugs.

Talking about this flag. It is supported by glibc, musl and uclibc-ng. Not supported by dietlibc.

Good; it has wide support. dietlibc should implement it.

Karlson2k · 2025-02-17T17:22:50Z

Sure, it is up to maintainers whether to improve code portability or wait for others to adapt. 😉
On the other hand, the code already has a number of uses of unportable (or not fully portable) features, so this particular part is not a problem as the whole code may work only in specific environments, where these extensions are automatically available.

alejandro-colomar · 2025-02-17T17:42:04Z

As a sanity check, the major BSDs (Free,Net,Open) also support e. MirBSD doesn't document it. Let's CC its maintainer: @mirabilos.

Thorsten, is it an undocumented feature? Or does MirBSD not implement O_CLOEXEC in fopen(3)?

Anyone feel free to contact the dietlibc maintainer(s) to ask them to support this POSIX feature.

mirabilos · 2025-02-17T21:48:12Z

(Not that there’s any need for the shadow package on MirBSD, but…)

On Mon, 17 Feb 2025, Alejandro Colomar wrote: Thorsten, is it an undocumented feature? Or does MirBSD not implement O_CLOEXEC in fopen(3)?

MirBSD didn’t even have O_CLOEXEC in open until recently (2016), and I don’t expect it to provide latest POSIX features any time soon other than on a need basis for now. I’m stuck fixing some locale-related issues at the moment… fopen(3) and friends currently support /^[rwa]b?\+?/ with [rwa] and [+] actually doing something (src/lib/libc/stdio/flags.c). In general, MirBSD doesn’t really target POSIX, that’s pretty much insurmountable (especially the threads part) and in parts (leap seconds) even against a law in my country, but I try to bring mksh as close to POSIX as possible/sensible (all current shells ignore the part where you’re supposed to assign a path for builtins). Expect porting to MirBSD to be similar to porting to a classic BSD or other Unix, not to a modern one. And, given its slow progress at the moment it’s sensible to not consider it much when porting software (not calling it abandoned, but I’ve yet to get quite some working on it done before I can even use it as daily box myself again). Still thanks for thinking about it, //mirabilos

The _exit(2) function is a better choice for exiting a child in many cases. Most prominently it avoids calls of functions registered with atexit(3) by the parent. There are valid reasons to call exit(3) and the example is actually one of them: flush FILE-based output. Since atexit(3) is never called, we could just stay with exit(3). Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> Message-ID: <tngwcffbrzbfkj6vrxgxpekrp3bzuftdy2mzow56xyfkrcna2w@nbgr2ourerxo> Link: <shadow-maint/shadow#1171> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar approved these changes Jan 6, 2025

View reviewed changes

stoeckmann added 2 commits January 8, 2025 17:22

stoeckmann force-pushed the defense branch from 681c453 to 9af6dce Compare January 8, 2025 16:22

alejandro-colomar merged commit aebc4dd into shadow-maint:master Jan 10, 2025
9 checks passed

Add cheap defense mechanisms #1171

Add cheap defense mechanisms #1171

Uh oh!

Conversation

stoeckmann commented Jan 6, 2025

Uh oh!

alejandro-colomar commented Jan 6, 2025

Uh oh!

alejandro-colomar left a comment

Choose a reason for hiding this comment

Uh oh!

alejandro-colomar commented Jan 10, 2025

Uh oh!

Uh oh!

stoeckmann commented Jan 12, 2025

Uh oh!

alejandro-colomar commented Jan 12, 2025

Uh oh!

stoeckmann commented Jan 12, 2025

Uh oh!

alejandro-colomar commented Jan 12, 2025

Uh oh!

Karlson2k commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejandro-colomar commented Feb 17, 2025 via email

Uh oh!

alejandro-colomar commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karlson2k commented Feb 17, 2025

Uh oh!

alejandro-colomar commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karlson2k commented Feb 17, 2025

Uh oh!

alejandro-colomar commented Feb 17, 2025

Uh oh!

mirabilos commented Feb 17, 2025 via email

Uh oh!

Uh oh!

Karlson2k commented Feb 17, 2025 •

edited

Loading

alejandro-colomar commented Feb 17, 2025 •

edited

Loading

alejandro-colomar commented Feb 17, 2025 •

edited

Loading