Re: [PATCH] implement issetugid(2)

[Brent Cook <busterb@gmail.com>]

On Jul 12, 2014, at 10:12 PM, Isaac Dunham <ibid.ag@gmail.com> wrote:

> On Sat, Jul 12, 2014 at 11:55:06AM -0600, Brent Cook wrote:
>> From OpenBSD 2.0 and later
>> http://www.openbsd.org/cgi-bin/man.cgi?query=issetugid&sektion=2
>> ---
>> include/unistd.h       | 1 +
>> src/unistd/issetugid.c | 9 +++++++++
>> 2 files changed, 10 insertions(+)
>> create mode 100644 src/unistd/issetugid.c
> 
> Hello Brent Cook,
> Would you mind giving an explanation of the rationale when you 
> add functions?
> 
> My understanding is that this patch arose from an issue with
> libressl-portable. libressl needs to check if it's running in
> privileged code; while getauxval() is the simplest way to check on
> Linux systems, it's unreliable.
> Specifically, getauxval() on glibc < 2.19, klibc, and on bionic will not
> set errno if it cannot check AT_SECURE, but only returns 0.
> uclibc does not implement this, and I see no reference to dietlibc
> or newlib implementations.
> Given the number of known-bad versions, expecting an unknown version to
> set errno is excessivey optimistic.
> So libressl cannot trust getauxval() unless it's known to be a version
> that sets errno; this currently means glibc 2.19+ or musl.
> glibc did not version getauxval when they fixed it to set errno, so relying
> on symbol versioning will not prevent running against lower versions.
> 
> (But might other versioned functions that libressl uses prevent
> using older glibc versions with libressl built against glibc 2.19+?)

This is a fair point.

Compile-time tests were ruled out because static libraries can be built against a safe libc, then linked to an app that uses an unsafe libc, causing a vulnerability.

For example, here I have built a static libressl library with musl-gcc, then a link a program with the resulting library using glibc:

$ touch crypto/compat/arc4random.c

$ make V=1
Making all in crypto
make[1]: Entering directory `/home/bcook/libressl-2.0.0/crypto'
/bin/bash ../libtool  --tag=CC   --mode=compile musl-gcc -DPACKAGE_NAME=\"libressl\" -DPACKAGE_TARNAME=\"libressl\" -DPACKAGE_VERSION=\"2.0.0\" -DPACKAGE_STRING=\"libressl\ 2.0.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"libressl\" -DVERSION=\"2.0.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_GETAUXVAL=1 -I.  -I../include -DOPENSSL_NO_ASM -DHAVE_CRYPTODEV -DLIBRESSL_INTERNAL -I../crypto/asn1 -I../crypto/evp -I../crypto/modes  -Wall -std=c99 -g -D_BSD_SOURCE -D_POSIX_SOURCE -D_GNU_SOURCE -O2  -Wall -std=c99 -g -D_BSD_SOURCE -D_POSIX_SOURCE -D_GNU_SOURCE -MT compat/libcompat_la-arc4random.lo -MD -MP -MF compat/.deps/libcompat_la-arc4random.Tpo -c -o compat/libcompat_la-arc4random.lo `test -f 'compat/arc4random.c' || echo './'`compat/arc4random.c

..library builds..

$:~/libressl-2.0.0$ cat test.c
#include <stdio.h>
int main()
{
	printf("arc4random %u\n", arc4random());
}

$:~/libressl-2.0.0$ gcc test.c crypto/.libs/libcompat.a crypto/.libs/libcrypto.a

$:~/libressl-2.0.0$ ./a.out
arc4random 565490330

$:~/libressl-2.0.0$ ldd a.out
	linux-vdso.so.1 =>  (0x00007fffff492000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f2fde3b3000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2fde78e000)

With the requirement of issetugid, we get a link-time failure instead:

$ cat test.c
#include <stdio.h>
int main()
{
	printf("issetugid %u\n", issetugid());
}

$ gcc test.c ./crypto/.libs/libcompat.a
/tmp/ccPenDq1.o: In function `main':
test.c:(.text+0xa): undefined reference to `issetugid'
collect2: error: ld returned 1 exit status

$ musl-gcc test.c ./crypto/.libs/libcompat.a
$ ./a.out
issetugid 0


> 
> My guess is that the logic here is that a system providing issetugid()
> can be assumed to have a working version, unlike getauxval().
> 
> Is that the reason for this?

Yes, this is an excellent summary of the issue. I should have been clearer in the initial presentation.

>> diff --git a/include/unistd.h b/include/unistd.h
>> index bb19cd8..30290c3 100644
>> --- a/include/unistd.h
>> +++ b/include/unistd.h
>> @@ -109,6 +109,7 @@ uid_t geteuid(void);
>> gid_t getgid(void);
>> gid_t getegid(void);
>> int getgroups(int, gid_t []);
>> +int issetugid(void);
> 
> This part is a namespace violation.
> issetugid() should be conditional on _BSD_SOURCE if it is added, since
> unistd.h is in POSIX.

OK, I do not think that this would be a problem.

>> int setuid(uid_t);
>> int setreuid(uid_t, uid_t);
>> int seteuid(uid_t);
>> diff --git a/src/unistd/issetugid.c b/src/unistd/issetugid.c
>> new file mode 100644
>> index 0000000..8c81336
>> --- /dev/null
>> +++ b/src/unistd/issetugid.c
>> @@ -0,0 +1,9 @@
>> +#include <errno.h>
>> +#include <unistd.h>
>> +#include <sys/auxv.h>
>> +
>> +int issetugid(void)
>> +{
>> +	errno = 0;
>> +	return !(getauxval(AT_SECURE) == 0 && errno != ENOENT);
>> +}
>> --
>> 1.9.1
>>