[PATCH] Fix busybox SUID support

[PATCH] Fix busybox SUID support

[Tom Rini]

I was about to just push this and I noticed that a number of
distributions (SlugOS, Angstrom, Kaelios, micro) currently set
FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
some way-back-when's of "busybox SUID is dangerous / crap!", I thought
it best to post the patch first and let folks speak up / ask me to drop
FEATURE_SUID=y when I do this.  So, here's the patch:

diff --git a/recipes/busybox/busybox.inc b/recipes/busybox/busybox.inc
index 5f52850..b165d0f 100644
--- a/recipes/busybox/busybox.inc
+++ b/recipes/busybox/busybox.inc
@@ -11,7 +11,7 @@ LICENSE = "GPL"
 SECTION = "base"
 PRIORITY = "required"
 
-INC_PR = "r24"
+INC_PR = "r25"
 
 SRC_URI = "\
   file://busybox-cron \
@@ -96,7 +96,11 @@ do_install () {
 	# Install /bin/busybox, and the /bin/sh link so the postinst script
 	# can run. Let update-alternatives handle the rest.
 	install -d ${D}${base_bindir}
-	install -m 0755 ${S}/busybox ${D}${base_bindir}
+	if grep -q "CONFIG_FEATURE_SUID=y" ${WORKDIR}/defconfig; then
+		install -m 4755 ${S}/busybox ${D}${base_bindir}
+	else
+		install -m 0755 ${S}/busybox ${D}${base_bindir}
+	fi
 	ln -sf busybox ${D}${base_bindir}/sh
 
 	if grep -q "CONFIG_SYSLOGD=y" ${WORKDIR}/defconfig; then


-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] Fix busybox SUID support

[Khem Raj]

On Tue, Feb 23, 2010 at 11:02 AM, Tom Rini <tom_rini@mentor.com> wrote:
> I was about to just push this and I noticed that a number of
> distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> it best to post the patch first and let folks speak up / ask me to drop
> FEATURE_SUID=y when I do this.  So, here's the patch:
>
> diff --git a/recipes/busybox/busybox.inc b/recipes/busybox/busybox.inc
> index 5f52850..b165d0f 100644
> --- a/recipes/busybox/busybox.inc
> +++ b/recipes/busybox/busybox.inc
> @@ -11,7 +11,7 @@ LICENSE = "GPL"
>  SECTION = "base"
>  PRIORITY = "required"
>
> -INC_PR = "r24"
> +INC_PR = "r25"
>
>  SRC_URI = "\
>   file://busybox-cron \
> @@ -96,7 +96,11 @@ do_install () {
>        # Install /bin/busybox, and the /bin/sh link so the postinst script
>        # can run. Let update-alternatives handle the rest.
>        install -d ${D}${base_bindir}
> -       install -m 0755 ${S}/busybox ${D}${base_bindir}
> +       if grep -q "CONFIG_FEATURE_SUID=y" ${WORKDIR}/defconfig; then

may be you should grep it in ${S}/.config because thats what defconfig
turns into finally
and used by busybox build.

> +               install -m 4755 ${S}/busybox ${D}${base_bindir}

what does 4755 translate to ? (curiosity)

> +       else
> +               install -m 0755 ${S}/busybox ${D}${base_bindir}
> +       fi
>        ln -sf busybox ${D}${base_bindir}/sh
>
>        if grep -q "CONFIG_SYSLOGD=y" ${WORKDIR}/defconfig; then
>
>
> --
> Tom Rini <tom_rini@mentor.com>
> Mentor Graphics Corporation
>
> _______________________________________________
> Openembedded-devel mailing list
> Openembedded-devel@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-devel
>

Re: [PATCH] Fix busybox SUID support

[Tom Rini]

On Tue, 2010-02-23 at 11:51 -0800, Khem Raj wrote:
> On Tue, Feb 23, 2010 at 11:02 AM, Tom Rini <tom_rini@mentor.com> wrote:
> > I was about to just push this and I noticed that a number of
> > distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> > FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> > some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> > it best to post the patch first and let folks speak up / ask me to drop
> > FEATURE_SUID=y when I do this.  So, here's the patch:
> >
> > diff --git a/recipes/busybox/busybox.inc b/recipes/busybox/busybox.inc
> > index 5f52850..b165d0f 100644
> > --- a/recipes/busybox/busybox.inc
> > +++ b/recipes/busybox/busybox.inc
> > @@ -11,7 +11,7 @@ LICENSE = "GPL"
> >  SECTION = "base"
> >  PRIORITY = "required"
> >
> > -INC_PR = "r24"
> > +INC_PR = "r25"
> >
> >  SRC_URI = "\
> >   file://busybox-cron \
> > @@ -96,7 +96,11 @@ do_install () {
> >        # Install /bin/busybox, and the /bin/sh link so the postinst script
> >        # can run. Let update-alternatives handle the rest.
> >        install -d ${D}${base_bindir}
> > -       install -m 0755 ${S}/busybox ${D}${base_bindir}
> > +       if grep -q "CONFIG_FEATURE_SUID=y" ${WORKDIR}/defconfig; then
> 
> may be you should grep it in ${S}/.config because thats what defconfig
> turns into finally
> and used by busybox build.

I'd be fine doing a follow-up to clean them all up, but today
busybox.inc does all of its checks to ${WORKDIR}/defconfig.

> > +               install -m 4755 ${S}/busybox ${D}${base_bindir}
> 
> what does 4755 translate to ? (curiosity)

4 is suid (2 is sgid, 1 is i forget the name but what you stick on /tmp
& such).

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] Fix busybox SUID support

[Chris Larson]

On Tue, Feb 23, 2010 at 1:14 PM, Tom Rini <tom_rini@mentor.com> wrote:

> On Tue, 2010-02-23 at 11:51 -0800, Khem Raj wrote:
> > On Tue, Feb 23, 2010 at 11:02 AM, Tom Rini <tom_rini@mentor.com> wrote:
> > > I was about to just push this and I noticed that a number of
> > > distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> > > FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> > > some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> > > it best to post the patch first and let folks speak up / ask me to drop
> > > FEATURE_SUID=y when I do this.  So, here's the patch:
> > >
> > > diff --git a/recipes/busybox/busybox.inc b/recipes/busybox/busybox.inc
> > > index 5f52850..b165d0f 100644
> > > --- a/recipes/busybox/busybox.inc
> > > +++ b/recipes/busybox/busybox.inc
> > > @@ -11,7 +11,7 @@ LICENSE = "GPL"
> > >  SECTION = "base"
> > >  PRIORITY = "required"
> > >
> > > -INC_PR = "r24"
> > > +INC_PR = "r25"
> > >
> > >  SRC_URI = "\
> > >   file://busybox-cron \
> > > @@ -96,7 +96,11 @@ do_install () {
> > >        # Install /bin/busybox, and the /bin/sh link so the postinst
> script
> > >        # can run. Let update-alternatives handle the rest.
> > >        install -d ${D}${base_bindir}
> > > -       install -m 0755 ${S}/busybox ${D}${base_bindir}
> > > +       if grep -q "CONFIG_FEATURE_SUID=y" ${WORKDIR}/defconfig; then
> >
> > may be you should grep it in ${S}/.config because thats what defconfig
> > turns into finally
> > and used by busybox build.
>
> I'd be fine doing a follow-up to clean them all up, but today
> busybox.inc does all of its checks to ${WORKDIR}/defconfig.
>
> > > +               install -m 4755 ${S}/busybox ${D}${base_bindir}
> >
> > what does 4755 translate to ? (curiosity)
>
> 4 is suid (2 is sgid, 1 is i forget the name but what you stick on /tmp
> & such).


Sticky :)
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

Re: [PATCH] Fix busybox SUID support

[Phil Blundell]

On Tue, 2010-02-23 at 12:02 -0700, Tom Rini wrote:
> +	if grep -q "CONFIG_FEATURE_SUID=y" ${WORKDIR}/defconfig; then
> +		install -m 4755 ${S}/busybox ${D}${base_bindir}
> +	else
> +		install -m 0755 ${S}/busybox ${D}${base_bindir}
> +	fi

Looks good to me.

p.

Re: [PATCH] Fix busybox SUID support

[Michael 'Mickey' Lauer]

Ah, finally someone does something about it. Next step would be to dump
tinylogin which is known broken and use busyboxes login utilities
instead.

Great work, Tom.

:M:

Re: [PATCH] Fix busybox SUID support

[Tom Rini]

On Tue, 2010-02-23 at 23:52 +0100, Michael 'Mickey' Lauer wrote:

> Ah, finally someone does something about it. Next step would be to dump
> tinylogin which is known broken and use busyboxes login utilities
> instead.

Not wanting to open that can of worms up just yet, yes, this will let
distros opt in to that kind of thing (I hit this because of 'su' the
other day..).

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] Fix busybox SUID support

[Marcin Juszkiewicz]

[-- Attachment #1: Type: Text/Plain, Size: 1013 bytes --]

Dnia wtorek, 23 lutego 2010 o 20:02:56 Tom Rini napisał(a):
> I was about to just push this and I noticed that a number of
> distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> it best to post the patch first and let folks speak up / ask me to drop
> FEATURE_SUID=y when I do this.  So, here's the patch:

Ok, but does not it require /etc/something to list which applets are allowed 
to be suid and which are not? 

Hm. checked sources. with FEATURE_SUID suid will be active only for "crontab, 
dnsd, findfs, ipcrm, ipcs, login, passwd, ping, su, traceroute, vlock" 
commands. /etc/busybox.conf is CONFIG_FEATURE_SUID_CONFIG option.

Acked-by: Marcin Juszkiewicz <marcin@juszkiewicz.com.pl>

Regards, 
-- 
JID:      hrw@jabber.org
Website:  http://marcin.juszkiewicz.com.pl/
LinkedIn: http://www.linkedin.com/in/marcinjuszkiewicz

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

Re: [PATCH] Fix busybox SUID support

[Tom Rini]

On Wed, 2010-02-24 at 11:19 +0100, Marcin Juszkiewicz wrote:
> Dnia wtorek, 23 lutego 2010 o 20:02:56 Tom Rini napisał(a):
> > I was about to just push this and I noticed that a number of
> > distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> > FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> > some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> > it best to post the patch first and let folks speak up / ask me to drop
> > FEATURE_SUID=y when I do this.  So, here's the patch:
> 
> Ok, but does not it require /etc/something to list which applets are allowed 
> to be suid and which are not? 
> 
> Hm. checked sources. with FEATURE_SUID suid will be active only for "crontab, 
> dnsd, findfs, ipcrm, ipcs, login, passwd, ping, su, traceroute, vlock" 
> commands. /etc/busybox.conf is CONFIG_FEATURE_SUID_CONFIG option.

To be clear, enabling one of those applets will force FEATURE_SUID to be
set.  FEATURE_SUID_CONFIG lets you configure who can run these SUID
programs.

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] Fix busybox SUID support

[Mike Westerhof]

Tom Rini wrote:
> I was about to just push this and I noticed that a number of
> distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> it best to post the patch first and let folks speak up / ask me to drop
> FEATURE_SUID=y when I do this.  So, here's the patch:

In the case of SlugOS, this was deliberate -- the thinking was that we
would let the user decide if they wanted to run busybox SUID after
installation.  In retrospect, that actually led to more problems than
good, so this change gets an ACK from me (albeit late - sorry for the
delay!)

+1

-Mike

Re: [PATCH] Fix busybox SUID support

[Koen Kooi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26-02-10 16:43, Mike Westerhof wrote:
> Tom Rini wrote:
>> I was about to just push this and I noticed that a number of
>> distributions (SlugOS, Angstrom, Kaelios, micro) currently set
>> FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
>> some way-back-when's of "busybox SUID is dangerous / crap!", I thought
>> it best to post the patch first and let folks speak up / ask me to drop
>> FEATURE_SUID=y when I do this.  So, here's the patch:
> 
> In the case of SlugOS, this was deliberate -- the thinking was that we
> would let the user decide if they wanted to run busybox SUID after
> installation.  In retrospect, that actually led to more problems than
> good, so this change gets an ACK from me (albeit late - sorry for the
> delay!)

The same logic was applied to angstrom "let users sort it out", but I
think this patch is a better way to go.

Acked-by: Koen Kooi <koen@openembedded.org>


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Darwin)

iD8DBQFLiBDuMkyGM64RGpERAj1iAJ92l81A14+d1zbGez1GZ1B2wtsxRgCdE5oP
sYCctrQ1jucxDfR0R4PMMf8=
=Bi6n
-----END PGP SIGNATURE-----

Re: [PATCH] Fix busybox SUID support

[C Michael Sundius]

On Fri, Feb 26, 2010 at 10:20 AM, Koen Kooi <k.kooi@student.utwente.nl>wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 26-02-10 16:43, Mike Westerhof wrote:
> > Tom Rini wrote:
> >> I was about to just push this and I noticed that a number of
> >> distributions (SlugOS, Angstrom, Kaelios, micro) currently set
> >> FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
> >> some way-back-when's of "busybox SUID is dangerous / crap!", I thought
> >> it best to post the patch first and let folks speak up / ask me to drop
> >> FEATURE_SUID=y when I do this.  So, here's the patch:
> >
> > In the case of SlugOS, this was deliberate -- the thinking was that we
> > would let the user decide if they wanted to run busybox SUID after
> > installation.  In retrospect, that actually led to more problems than
> > good, so this change gets an ACK from me (albeit late - sorry for the
> > delay!)
>
> The same logic was applied to angstrom "let users sort it out", but I
> think this patch is a better way to go.
>
>
just to give you a heads up, we're currently working on a patch to busybox
(and well have an OE recipe for it too) which will allow us to currently
create two busybox executables: busybox-suid and busybox-nsuid. this way we
can have the best of both worlds. maintain suid for programs that really
require it while keeping the likes of ls and cat safe for the masses...

once its through our internal review, we'll send it out.

mike

Re: [PATCH] Fix busybox SUID support

[Bernhard Reutner-Fischer]

On Fri, Feb 26, 2010 at 12:21:56PM -0800, C Michael Sundius wrote:
>On Fri, Feb 26, 2010 at 10:20 AM, Koen Kooi <k.kooi@student.utwente.nl>wrote:
>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 26-02-10 16:43, Mike Westerhof wrote:
>> > Tom Rini wrote:
>> >> I was about to just push this and I noticed that a number of
>> >> distributions (SlugOS, Angstrom, Kaelios, micro) currently set
>> >> FEATURE_SUID=y, but it's not actually install SUID.  And since I recall
>> >> some way-back-when's of "busybox SUID is dangerous / crap!", I thought
>> >> it best to post the patch first and let folks speak up / ask me to drop
>> >> FEATURE_SUID=y when I do this.  So, here's the patch:
>> >
>> > In the case of SlugOS, this was deliberate -- the thinking was that we
>> > would let the user decide if they wanted to run busybox SUID after
>> > installation.  In retrospect, that actually led to more problems than
>> > good, so this change gets an ACK from me (albeit late - sorry for the
>> > delay!)
>>
>> The same logic was applied to angstrom "let users sort it out", but I
>> think this patch is a better way to go.
>>
>>
>just to give you a heads up, we're currently working on a patch to busybox
>(and well have an OE recipe for it too) which will allow us to currently
>create two busybox executables: busybox-suid and busybox-nsuid. this way we
>can have the best of both worlds. maintain suid for programs that really
>require it while keeping the likes of ls and cat safe for the masses...

SUID_DROP applets do just that before the individual applet_main is called,
i.e. drops privs. But whatever..
If you really think you want to build the thing twice then i'd try
FEATURE_SHARED_BUSYBOX, fyi.
$ size 0_lib/{busybox,libbusybox.so.1.17.0.git}
   text	   data	    bss	    dec	    hex	filename
   1379	    520	     16	   1915	    77b	0_lib/busybox
 339569	   6705	   8552	 354826	  56a0a	0_lib/libbusybox.so.1.17.0.git

duplicating 2k is better than duplicating all the innocent rest, but maybe
that's just me..

cheers,

>once its through our internal review, we'll send it out.

Re: [PATCH] Fix busybox SUID support

[Phil Blundell]

On Fri, 2010-02-26 at 23:26 +0100, Bernhard Reutner-Fischer wrote:
> SUID_DROP applets do just that before the individual applet_main is called,
> i.e. drops privs. But whatever..
> If you really think you want to build the thing twice then i'd try
> FEATURE_SHARED_BUSYBOX, fyi.
> $ size 0_lib/{busybox,libbusybox.so.1.17.0.git}
>    text	   data	    bss	    dec	    hex	filename
>    1379	    520	     16	   1915	    77b	0_lib/busybox
>  339569	   6705	   8552	 354826	  56a0a	0_lib/libbusybox.so.1.17.0.git
> 
> duplicating 2k is better than duplicating all the innocent rest, but maybe
> that's just me..

Whether this is a good idea or not rather depends on what you're trying
to protect against.  The threat scenarios for a setuid busybox basically
fall into two categories:

a) user invokes an applet which shouldn't be setuid, but busybox fails
(for whatever reason) to drop its privs and allows the applet to run in
root context

b) user invokes an applet which is intended to be setuid, busybox
correctly retains the elevated privs while running it, but there is some
bug or vulnerability in the code which causes unwanted results

If you're primarily worried about case (a) then building two copies of
the frontend which share a common libbusybox, one setuid and one not,
probably is a reasonable thing to do.  However, as you say, busybox does
already have a fairly robust mechanism in place for dropping privs when
they are not wanted by a particular applet and hence the threat from
this side seems to be quite low anyway.

If you are primarily worried about case (b) then the easiest way to
mitigate the threat is to reduce the amount of code which is linked in
to the setuid binary (either statically or dynamically) and hence the
amount of code that you need to audit for security hazards.  In that
situation, linking with a shared libbusybox would not help, since you
still have essentially the whole of busybox to worry about and you might
as well just stick with a single large binary.  Instead you would be
better off making a minimal, standalone busybox binary which contained
the bare minimum of code that was required to fulfil its setuid
functions.

p.

Re: [PATCH] Fix busybox SUID support

[Bernhard Reutner-Fischer]

On Fri, Feb 26, 2010 at 10:42:30PM +0000, Phil Blundell wrote:

>If you're primarily worried about case (a) then building two copies of
>the frontend which share a common libbusybox, one setuid and one not,
>probably is a reasonable thing to do.  However, as you say, busybox does
>already have a fairly robust mechanism in place for dropping privs when
>they are not wanted by a particular applet and hence the threat from
>this side seems to be quite low anyway.

Yes, and that's what i've read into Michaels mail that this was what he
was primarily concerned about, but rereading him he didn't actually say
that. My apologies.
>
>If you are primarily worried about case (b) then the easiest way to
>mitigate the threat is to reduce the amount of code which is linked in

indeed