[musl] Re: [shadow-maint/shadow] Add cheap defense mechanisms (PR #1171)

[musl] Re: [shadow-maint/shadow] Add cheap defense mechanisms (PR #1171)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[musl] Re: [shadow-maint/shadow] Add cheap defense mechanisms (PR #1171)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

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 (shadow-maint/shadow#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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [musl] Re: [shadow-maint/shadow] Add cheap defense mechanisms (PR #1171)

[Rich Felker]

On Mon, Feb 17, 2025 at 10:42:06AM +0100, Alejandro Colomar wrote:
> 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.

It's in the latest POSIX and we have supported it for a long time as
POSIX-future (since 2012/release 0.9.7).

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

Unfortunately this doesn't work because it's UB to pass any modes but
the standards-specified ones.

In any case use of fopen is just gratuitously bad for software that
targets POSIX. The right way to do things is a two-step open+fdopen.
This avoids needing to depend on new features to open and lets you use
all the modern open flags, openat if needed, etc.

Rich


P.S. Had to omit shadow-utils <~hallyn/shadow@lists.sr.ht> from CC
because my mail software rejects / in an address... gotta fix that.
Apologies.

Re: [musl] Re: [shadow-maint/shadow] Add cheap defense mechanisms (PR #1171)

[Evgeny Grin]

[-- Attachment #1.1.1: Type: text/plain, Size: 1241 bytes --]

Hi,

17-Feb-2025 15:44 (UTC+0100), Rich Felker wrote:
> On Mon, Feb 17, 2025 at 10:42:06AM +0100, Alejandro Colomar wrote:
> 
>>> Isn't it safe to use constructs like
>>> ``` C
>>> shadow = fopen (SGROUP_FILE, "re");
>>> if (NULL == shadow )
>>>    shadow = fopen (SGROUP_FILE, "r");
>>> ```
>>> ?
> 
> Unfortunately this doesn't work because it's UB to pass any modes but
> the standards-specified ones.

You are right, according to POSIX, failure with EINVAL is not guaranteed.
That is not nice as the function is allowed to perform not as requested 
without any indication.

> In any case use of fopen is just gratuitously bad for software that
> targets POSIX. The right way to do things is a two-step open+fdopen.
> This avoids needing to depend on new features to open and lets you use
> all the modern open flags, openat if needed, etc.

I agree. This would be the best approach.

While dietlibc does not support "e" mode and glibc, musl, uclibc-ng 
support "e" mode, checking for particular library is bad.
Checking for feature support is not reliable (function may not return 
error, while the flag is ignored or even used differently) and not 
possible when cross-compiling.

-- 
Best,
Evgeny


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3215 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [musl] Re: [shadow-maint/shadow] Add cheap defense mechanisms (PR #1171)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

Hi Rich,

On Mon, Feb 17, 2025 at 09:44:58AM -0500, Rich Felker wrote:
> It's in the latest POSIX and we have supported it for a long time as
> POSIX-future (since 2012/release 0.9.7).

Thanks!  That agrees with my own research.

> > > Isn't it safe to use constructs like 
> > > ``` C
> > > shadow = fopen (SGROUP_FILE, "re");
> > > if (NULL == shadow )
> > >   shadow = fopen (SGROUP_FILE, "r");
> > > ```
> > > ?
> 
> Unfortunately this doesn't work because it's UB to pass any modes but
> the standards-specified ones.

Makes sense.  Thanks!

> In any case use of fopen is just gratuitously bad for software that
> targets POSIX. The right way to do things is a two-step open+fdopen.
> This avoids needing to depend on new features to open and lets you use
> all the modern open flags, openat if needed, etc.

Hmmm, thanks for the idea!  This code is old, so we didn't actually
write it; we only added 'e' recently to it.  If we need to write new
code, I'll take into consideration doing that as a two-step process,
maybe adding a wrapper around that process.


Have a lovely day!
Alex

> P.S. Had to omit shadow-utils <~hallyn/shadow@lists.sr.ht> from CC
> because my mail software rejects / in an address... gotta fix that.
> Apologies.

No problem.  Thanks!

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]