Message ID | 1406050049-29658-1-git-send-email-basile@opensource.dyc.edu |
---|---|
State | Superseded |
Headers | show |
I should add that this updated patch addresses Rich's points: 1) I've tested this for both dynamic and static linking and I tested that libressl builds and works correctly. 2) I added a link to the musl commit for the reasoning behind this approach. On 07/22/14 13:27, basile@opensource.dyc.edu wrote: > From: "Anthony G. Basile" <blueness@gentoo.org> > > issetugid() returns 1 if the process environment or memory address space > is considered tainted, and returns 0 otherwise. This happens, for example, > when a process's privileges are elevated by the setuid or setgid flags on > an executable belonging to root. This function first appeard in OpenBSD 2.0 > and is needed for the LibreSSL. > > This patch follows the same logic as the equivalent musl commit. For more > information see the commit message at > > http://git.musl-libc.org/cgit/musl/commit/?id=ddddec106fd17c3aca3287005d21e92f742aa9d4 > --- > include/unistd.h | 8 ++++++++ > libc/misc/file/issetugid.c | 12 ++++++++++++ > libc/misc/internals/__uClibc_main.c | 12 ++++++++++++ > 3 files changed, 32 insertions(+) > create mode 100644 libc/misc/file/issetugid.c > > diff --git a/include/unistd.h b/include/unistd.h > index 540062a..6c2c8c2 100644 > --- a/include/unistd.h > +++ b/include/unistd.h > @@ -1168,6 +1168,14 @@ extern long int syscall (long int __sysno, ...) __THROW; > > #endif /* Use misc. */ > > +#ifdef __USE_MISC > +/* issetugid() returns 1 if the process environment or memory address space > + is considered tainted, and returns 0 otherwise. This happens, for example, > + when a process's privileges are elevated by the setuid or setgid flags on > + an executable belonging to root. > +*/ > +extern int issetugid(void); > +#endif > > #if (defined __USE_MISC || defined __USE_XOPEN_EXTENDED) && !defined F_LOCK > /* NOTE: These declarations also appear in <fcntl.h>; be sure to keep both > diff --git a/libc/misc/file/issetugid.c b/libc/misc/file/issetugid.c > new file mode 100644 > index 0000000..29a4167 > --- /dev/null > +++ b/libc/misc/file/issetugid.c > @@ -0,0 +1,12 @@ > +/* Copyright (C) 2013 Gentoo Foundation > + * Licensed under LGPL v2.1 or later, see the file COPYING.LIB in this tarball. > + */ > + > +#include <unistd.h> > + > +extern int _pe_secure; > + > +int issetugid(void) > +{ > + return _pe_secure; > +} > diff --git a/libc/misc/internals/__uClibc_main.c b/libc/misc/internals/__uClibc_main.c > index a37751f..b062e62 100644 > --- a/libc/misc/internals/__uClibc_main.c > +++ b/libc/misc/internals/__uClibc_main.c > @@ -40,6 +40,13 @@ > #include <locale.h> > #endif > > +/* Are we in a secure process environment or are we dealing > + * with setuid stuff? If we are dynamically linked, then we > + * already have _dl_secure, otherwise we need to re-examine > + * auxvt[]. > + */ > +int _pe_secure = 1; > + > #ifndef SHARED > void *__libc_stack_end = NULL; > > @@ -387,6 +394,11 @@ void __uClibc_main(int (*main)(int, char **, char **), int argc, > #else > if (_dl_secure) > #endif > + _pe_secure = 1 ; > + else > + _pe_secure = 0 ; > + > + if (_pe_secure) > { > __check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW); > __check_one_fd (STDOUT_FILENO, O_RDWR | O_NOFOLLOW); >
On Wed, Jul 23, 2014 at 07:28:26AM -0400, Anthony G. Basile wrote: > I should add that this updated patch addresses Rich's points: 1) I've tested > this for both dynamic and static linking and I tested that libressl builds > and works correctly. 2) I added a link to the musl commit for the reasoning > behind this approach. > > On 07/22/14 13:27, basile@opensource.dyc.edu wrote: > >From: "Anthony G. Basile" <blueness@gentoo.org> > > > >issetugid() returns 1 if the process environment or memory address space > >is considered tainted, and returns 0 otherwise. This happens, for example, > >when a process's privileges are elevated by the setuid or setgid flags on > >an executable belonging to root. This function first appeard in OpenBSD 2.0 > >and is needed for the LibreSSL. > > > >This patch follows the same logic as the equivalent musl commit. For more > >information see the commit message at > > > >http://git.musl-libc.org/cgit/musl/commit/?id=ddddec106fd17c3aca3287005d21e92f742aa9d4 > >--- > > include/unistd.h | 8 ++++++++ > > libc/misc/file/issetugid.c | 12 ++++++++++++ > > libc/misc/internals/__uClibc_main.c | 12 ++++++++++++ > > 3 files changed, 32 insertions(+) > > create mode 100644 libc/misc/file/issetugid.c > > > >diff --git a/include/unistd.h b/include/unistd.h > >index 540062a..6c2c8c2 100644 > >--- a/include/unistd.h > >+++ b/include/unistd.h > >@@ -1168,6 +1168,14 @@ extern long int syscall (long int __sysno, ...) __THROW; > > > > #endif /* Use misc. */ > > > >+#ifdef __USE_MISC is MISC (or MISC alone) an appropriate guard? > >+/* issetugid() returns 1 if the process environment or memory address space > >+ is considered tainted, and returns 0 otherwise. This happens, for example, > >+ when a process's privileges are elevated by the setuid or setgid flags on > >+ an executable belonging to root. > >+*/ > >+extern int issetugid(void); > >+#endif > > > > #if (defined __USE_MISC || defined __USE_XOPEN_EXTENDED) && !defined F_LOCK > > /* NOTE: These declarations also appear in <fcntl.h>; be sure to keep both > >diff --git a/libc/misc/file/issetugid.c b/libc/misc/file/issetugid.c > >new file mode 100644 > >index 0000000..29a4167 > >--- /dev/null > >+++ b/libc/misc/file/issetugid.c > >@@ -0,0 +1,12 @@ > >+/* Copyright (C) 2013 Gentoo Foundation > >+ * Licensed under LGPL v2.1 or later, see the file COPYING.LIB in this tarball. > >+ */ > >+ > >+#include <unistd.h> > >+ > >+extern int _pe_secure; > >+ > >+int issetugid(void) > >+{ > >+ return _pe_secure; > >+} > >diff --git a/libc/misc/internals/__uClibc_main.c b/libc/misc/internals/__uClibc_main.c > >index a37751f..b062e62 100644 > >--- a/libc/misc/internals/__uClibc_main.c > >+++ b/libc/misc/internals/__uClibc_main.c > >@@ -40,6 +40,13 @@ > > #include <locale.h> > > #endif > > > >+/* Are we in a secure process environment or are we dealing > >+ * with setuid stuff? If we are dynamically linked, then we > >+ * already have _dl_secure, otherwise we need to re-examine > >+ * auxvt[]. > >+ */ > >+int _pe_secure = 1; I'd default that to 0 and i'd make that libc_hidden_data_def(_pe_secure) > >+ > > #ifndef SHARED > > void *__libc_stack_end = NULL; > > > >@@ -387,6 +394,11 @@ void __uClibc_main(int (*main)(int, char **, char **), int argc, > > #else > > if (_dl_secure) > > #endif > >+ _pe_secure = 1 ; > >+ else > >+ _pe_secure = 0 ; > >+ > >+ if (_pe_secure) > > { > > __check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW); > > __check_one_fd (STDOUT_FILENO, O_RDWR | O_NOFOLLOW); > > Please reformat the hunk above like: @@ -388,10 +388,12 @@ void __uClibc_main(int (*main)(int, char **, char **), int argc, if (_dl_secure) #endif { + _pe_secure = 1; __check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW); __check_one_fd (STDOUT_FILENO, O_RDWR | O_NOFOLLOW); __check_one_fd (STDERR_FILENO, O_RDWR | O_NOFOLLOW); - } + } else + _pe_secure = 0; #endif __uclibc_progname = *argv; TIA,
On 07/24/14 16:41, Bernhard Reutner-Fischer wrote: > On Wed, Jul 23, 2014 at 07:28:26AM -0400, Anthony G. Basile wrote: >> I should add that this updated patch addresses Rich's points: 1) I've tested >> this for both dynamic and static linking and I tested that libressl builds >> and works correctly. 2) I added a link to the musl commit for the reasoning >> behind this approach. >> >> On 07/22/14 13:27, basile@opensource.dyc.edu wrote: >>> From: "Anthony G. Basile" <blueness@gentoo.org> >>> >>> issetugid() returns 1 if the process environment or memory address space >>> is considered tainted, and returns 0 otherwise. This happens, for example, >>> when a process's privileges are elevated by the setuid or setgid flags on >>> an executable belonging to root. This function first appeard in OpenBSD 2.0 >>> and is needed for the LibreSSL. >>> >>> This patch follows the same logic as the equivalent musl commit. For more >>> information see the commit message at >>> >>> http://git.musl-libc.org/cgit/musl/commit/?id=ddddec106fd17c3aca3287005d21e92f742aa9d4 >>> --- >>> include/unistd.h | 8 ++++++++ >>> libc/misc/file/issetugid.c | 12 ++++++++++++ >>> libc/misc/internals/__uClibc_main.c | 12 ++++++++++++ >>> 3 files changed, 32 insertions(+) >>> create mode 100644 libc/misc/file/issetugid.c >>> >>> diff --git a/include/unistd.h b/include/unistd.h >>> index 540062a..6c2c8c2 100644 >>> --- a/include/unistd.h >>> +++ b/include/unistd.h >>> @@ -1168,6 +1168,14 @@ extern long int syscall (long int __sysno, ...) __THROW; >>> >>> #endif /* Use misc. */ >>> >>> +#ifdef __USE_MISC > > is MISC (or MISC alone) an appropriate guard? I had a hard time (and still have a hard time) deciding this even after carefully reading include/features.h. The function started in openbsd and migrated to free and netbsd, but its not in 4.3BSD. _USE_MISC is looser but does include SYS V. I'm thinking now to just remove the guard. I did speak to Rich about what musl's doing but it doesn't seem appropriate here. If there are no strong opinions, I'll just remove the guard and resubmit in a few days. Your other comments below are good. > >>> +/* issetugid() returns 1 if the process environment or memory address space >>> + is considered tainted, and returns 0 otherwise. This happens, for example, >>> + when a process's privileges are elevated by the setuid or setgid flags on >>> + an executable belonging to root. >>> +*/ >>> +extern int issetugid(void); >>> +#endif >>> >>> #if (defined __USE_MISC || defined __USE_XOPEN_EXTENDED) && !defined F_LOCK >>> /* NOTE: These declarations also appear in <fcntl.h>; be sure to keep both >>> diff --git a/libc/misc/file/issetugid.c b/libc/misc/file/issetugid.c >>> new file mode 100644 >>> index 0000000..29a4167 >>> --- /dev/null >>> +++ b/libc/misc/file/issetugid.c >>> @@ -0,0 +1,12 @@ >>> +/* Copyright (C) 2013 Gentoo Foundation >>> + * Licensed under LGPL v2.1 or later, see the file COPYING.LIB in this tarball. >>> + */ >>> + >>> +#include <unistd.h> >>> + >>> +extern int _pe_secure; >>> + >>> +int issetugid(void) >>> +{ >>> + return _pe_secure; >>> +} >>> diff --git a/libc/misc/internals/__uClibc_main.c b/libc/misc/internals/__uClibc_main.c >>> index a37751f..b062e62 100644 >>> --- a/libc/misc/internals/__uClibc_main.c >>> +++ b/libc/misc/internals/__uClibc_main.c >>> @@ -40,6 +40,13 @@ >>> #include <locale.h> >>> #endif >>> >>> +/* Are we in a secure process environment or are we dealing >>> + * with setuid stuff? If we are dynamically linked, then we >>> + * already have _dl_secure, otherwise we need to re-examine >>> + * auxvt[]. >>> + */ >>> +int _pe_secure = 1; > > I'd default that to 0 > and i'd make that libc_hidden_data_def(_pe_secure) > >>> + >>> #ifndef SHARED >>> void *__libc_stack_end = NULL; >>> >>> @@ -387,6 +394,11 @@ void __uClibc_main(int (*main)(int, char **, char **), int argc, >>> #else >>> if (_dl_secure) >>> #endif >>> + _pe_secure = 1 ; >>> + else >>> + _pe_secure = 0 ; >>> + >>> + if (_pe_secure) >>> { >>> __check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW); >>> __check_one_fd (STDOUT_FILENO, O_RDWR | O_NOFOLLOW); >>> > > Please reformat the hunk above like: > @@ -388,10 +388,12 @@ void __uClibc_main(int (*main)(int, char **, char **), int argc, > if (_dl_secure) > #endif > { > + _pe_secure = 1; > __check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW); > __check_one_fd (STDOUT_FILENO, O_RDWR | O_NOFOLLOW); > __check_one_fd (STDERR_FILENO, O_RDWR | O_NOFOLLOW); > - } > + } else > + _pe_secure = 0; > #endif > > __uclibc_progname = *argv; > > TIA, >
On Sat, Jul 26, 2014 at 08:12:53AM -0400, Anthony G. Basile wrote: > On 07/24/14 16:41, Bernhard Reutner-Fischer wrote: > >On Wed, Jul 23, 2014 at 07:28:26AM -0400, Anthony G. Basile wrote: > >>I should add that this updated patch addresses Rich's points: 1) I've tested > >>this for both dynamic and static linking and I tested that libressl builds > >>and works correctly. 2) I added a link to the musl commit for the reasoning > >>behind this approach. > >> > >>On 07/22/14 13:27, basile@opensource.dyc.edu wrote: > >>>From: "Anthony G. Basile" <blueness@gentoo.org> > >>> > >>>issetugid() returns 1 if the process environment or memory address space > >>>is considered tainted, and returns 0 otherwise. This happens, for example, > >>>when a process's privileges are elevated by the setuid or setgid flags on > >>>an executable belonging to root. This function first appeard in OpenBSD 2.0 > >>>and is needed for the LibreSSL. > >>> > >>>This patch follows the same logic as the equivalent musl commit. For more > >>>information see the commit message at > >>> > >>>http://git.musl-libc.org/cgit/musl/commit/?id=ddddec106fd17c3aca3287005d21e92f742aa9d4 > >>>--- > >>> include/unistd.h | 8 ++++++++ > >>> libc/misc/file/issetugid.c | 12 ++++++++++++ > >>> libc/misc/internals/__uClibc_main.c | 12 ++++++++++++ > >>> 3 files changed, 32 insertions(+) > >>> create mode 100644 libc/misc/file/issetugid.c > >>> > >>>diff --git a/include/unistd.h b/include/unistd.h > >>>index 540062a..6c2c8c2 100644 > >>>--- a/include/unistd.h > >>>+++ b/include/unistd.h > >>>@@ -1168,6 +1168,14 @@ extern long int syscall (long int __sysno, ...) __THROW; > >>> > >>> #endif /* Use misc. */ > >>> > >>>+#ifdef __USE_MISC > > > >is MISC (or MISC alone) an appropriate guard? > > I had a hard time (and still have a hard time) deciding this even > after carefully reading include/features.h. The function started in > openbsd and migrated to free and netbsd, but its not in 4.3BSD. > _USE_MISC is looser but does include SYS V. I'm thinking now to > just remove the guard. I did speak to Rich about what musl's doing > but it doesn't seem appropriate here. > > If there are no strong opinions, I'll just remove the guard and > resubmit in a few days. > > Your other comments below are good. I don't think removing the guard would be correct at all; that would expose it even in profiles where the namespace is supposed to conform to POSIX/XSI. If __USE_MISC is inappropriate, the solution would be to move it to a more-inclusive featureset, not a less-inclusive one. Rich
On 07/27/14 05:36, Rich Felker wrote: > On Sat, Jul 26, 2014 at 08:12:53AM -0400, Anthony G. Basile wrote: >> On 07/24/14 16:41, Bernhard Reutner-Fischer wrote: >>> On Wed, Jul 23, 2014 at 07:28:26AM -0400, Anthony G. Basile wrote: >>>> I should add that this updated patch addresses Rich's points: 1) I've tested >>>> this for both dynamic and static linking and I tested that libressl builds >>>> and works correctly. 2) I added a link to the musl commit for the reasoning >>>> behind this approach. >>>> >>>> On 07/22/14 13:27, basile@opensource.dyc.edu wrote: >>>>> From: "Anthony G. Basile" <blueness@gentoo.org> >>>>> >>>>> issetugid() returns 1 if the process environment or memory address space >>>>> is considered tainted, and returns 0 otherwise. This happens, for example, >>>>> when a process's privileges are elevated by the setuid or setgid flags on >>>>> an executable belonging to root. This function first appeard in OpenBSD 2.0 >>>>> and is needed for the LibreSSL. >>>>> >>>>> This patch follows the same logic as the equivalent musl commit. For more >>>>> information see the commit message at >>>>> >>>>> http://git.musl-libc.org/cgit/musl/commit/?id=ddddec106fd17c3aca3287005d21e92f742aa9d4 >>>>> --- >>>>> include/unistd.h | 8 ++++++++ >>>>> libc/misc/file/issetugid.c | 12 ++++++++++++ >>>>> libc/misc/internals/__uClibc_main.c | 12 ++++++++++++ >>>>> 3 files changed, 32 insertions(+) >>>>> create mode 100644 libc/misc/file/issetugid.c >>>>> >>>>> diff --git a/include/unistd.h b/include/unistd.h >>>>> index 540062a..6c2c8c2 100644 >>>>> --- a/include/unistd.h >>>>> +++ b/include/unistd.h >>>>> @@ -1168,6 +1168,14 @@ extern long int syscall (long int __sysno, ...) __THROW; >>>>> >>>>> #endif /* Use misc. */ >>>>> >>>>> +#ifdef __USE_MISC >>> >>> is MISC (or MISC alone) an appropriate guard? >> >> I had a hard time (and still have a hard time) deciding this even >> after carefully reading include/features.h. The function started in >> openbsd and migrated to free and netbsd, but its not in 4.3BSD. >> _USE_MISC is looser but does include SYS V. I'm thinking now to >> just remove the guard. I did speak to Rich about what musl's doing >> but it doesn't seem appropriate here. >> >> If there are no strong opinions, I'll just remove the guard and >> resubmit in a few days. >> >> Your other comments below are good. > > I don't think removing the guard would be correct at all; that would > expose it even in profiles where the namespace is supposed to conform > to POSIX/XSI. If __USE_MISC is inappropriate, the solution would be to > move it to a more-inclusive featureset, not a less-inclusive one. > > Rich > Yeah since unistd.h is POSIX/XSI and issetugid is not, I guess you really do need *some* guard. As I see it, we have only two possibilities (from include/features.h): __USE_BSD Define 4.3BSD things. __USE_MISC Define things common to BSD and System V Unix. The other choices _USE_POSIX* _USE_XOPEN* _USE_GNU etc are clearly wrong. issetugid is not in 4.3BSD, and MISC is more inclusive, hence my original choice. So now I'm leaning back using __USE_MISC.
On 27 July 2014 11:36:00 CEST, Rich Felker <dalias@libc.org> wrote: >On Sat, Jul 26, 2014 at 08:12:53AM -0400, Anthony G. Basile wrote: >> On 07/24/14 16:41, Bernhard Reutner-Fischer wrote: >> >On Wed, Jul 23, 2014 at 07:28:26AM -0400, Anthony G. Basile wrote: >> >>> >> >>> #endif /* Use misc. */ >> >>> >> >>>+#ifdef __USE_MISC >> > >> >is MISC (or MISC alone) an appropriate guard? >> >> I had a hard time (and still have a hard time) deciding this even >> after carefully reading include/features.h. The function started in >> openbsd and migrated to free and netbsd, but its not in 4.3BSD. >> _USE_MISC is looser but does include SYS V. I'm thinking now to >> just remove the guard. I did speak to Rich about what musl's doing >> but it doesn't seem appropriate here. >> >> If there are no strong opinions, I'll just remove the guard and >> resubmit in a few days. >> >> Your other comments below are good. > >I don't think removing the guard would be correct at all; that would >expose it even in profiles where the namespace is supposed to conform >to POSIX/XSI. If __USE_MISC is inappropriate, the solution would be to >move it to a more-inclusive featureset, not a less-inclusive one. Indeed.
On Sun, Jul 27, 2014 at 01:20:46PM -0400, Anthony G. Basile wrote: > >I don't think removing the guard would be correct at all; that would > >expose it even in profiles where the namespace is supposed to conform > >to POSIX/XSI. If __USE_MISC is inappropriate, the solution would be to > >move it to a more-inclusive featureset, not a less-inclusive one. > > Yeah since unistd.h is POSIX/XSI and issetugid is not, I guess you > really do need *some* guard. As I see it, we have only two > possibilities (from include/features.h): > > __USE_BSD Define 4.3BSD things. > __USE_MISC Define things common to BSD and System V Unix. > > The other choices _USE_POSIX* _USE_XOPEN* _USE_GNU etc are clearly wrong. > > issetugid is not in 4.3BSD, and MISC is more inclusive, hence my > original choice. > > So now I'm leaning back using __USE_MISC. Note that uclibc's strlcpy is under __USE_BSD. It's an analogous function (adopted from later BSD, but not in 4.3BSD) so I think __USE_BSD might be the right place for issetugid. Rich
diff --git a/include/unistd.h b/include/unistd.h index 540062a..6c2c8c2 100644 --- a/include/unistd.h +++ b/include/unistd.h @@ -1168,6 +1168,14 @@ extern long int syscall (long int __sysno, ...) __THROW; #endif /* Use misc. */ +#ifdef __USE_MISC +/* issetugid() returns 1 if the process environment or memory address space + is considered tainted, and returns 0 otherwise. This happens, for example, + when a process's privileges are elevated by the setuid or setgid flags on + an executable belonging to root. +*/ +extern int issetugid(void); +#endif #if (defined __USE_MISC || defined __USE_XOPEN_EXTENDED) && !defined F_LOCK /* NOTE: These declarations also appear in <fcntl.h>; be sure to keep both diff --git a/libc/misc/file/issetugid.c b/libc/misc/file/issetugid.c new file mode 100644 index 0000000..29a4167 --- /dev/null +++ b/libc/misc/file/issetugid.c @@ -0,0 +1,12 @@ +/* Copyright (C) 2013 Gentoo Foundation + * Licensed under LGPL v2.1 or later, see the file COPYING.LIB in this tarball. + */ + +#include <unistd.h> + +extern int _pe_secure; + +int issetugid(void) +{ + return _pe_secure; +} diff --git a/libc/misc/internals/__uClibc_main.c b/libc/misc/internals/__uClibc_main.c index a37751f..b062e62 100644 --- a/libc/misc/internals/__uClibc_main.c +++ b/libc/misc/internals/__uClibc_main.c @@ -40,6 +40,13 @@ #include <locale.h> #endif +/* Are we in a secure process environment or are we dealing + * with setuid stuff? If we are dynamically linked, then we + * already have _dl_secure, otherwise we need to re-examine + * auxvt[]. + */ +int _pe_secure = 1; + #ifndef SHARED void *__libc_stack_end = NULL; @@ -387,6 +394,11 @@ void __uClibc_main(int (*main)(int, char **, char **), int argc, #else if (_dl_secure) #endif + _pe_secure = 1 ; + else + _pe_secure = 0 ; + + if (_pe_secure) { __check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW); __check_one_fd (STDOUT_FILENO, O_RDWR | O_NOFOLLOW);
From: "Anthony G. Basile" <blueness@gentoo.org> issetugid() returns 1 if the process environment or memory address space is considered tainted, and returns 0 otherwise. This happens, for example, when a process's privileges are elevated by the setuid or setgid flags on an executable belonging to root. This function first appeard in OpenBSD 2.0 and is needed for the LibreSSL. This patch follows the same logic as the equivalent musl commit. For more information see the commit message at http://git.musl-libc.org/cgit/musl/commit/?id=ddddec106fd17c3aca3287005d21e92f742aa9d4 --- include/unistd.h | 8 ++++++++ libc/misc/file/issetugid.c | 12 ++++++++++++ libc/misc/internals/__uClibc_main.c | 12 ++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 libc/misc/file/issetugid.c