package/sudo: new config to add sudo group and rule
diff mbox series

Message ID 20191103214153.24621-1-stephan@asklandd.dk
State Accepted
Headers show
Series
  • package/sudo: new config to add sudo group and rule
Related show

Commit Message

Stephan Henningsen Nov. 3, 2019, 9:41 p.m. UTC
From: Stephan Henningsen <stephan+buildroot@asklandd.dk>

Signed-off-by: Stephan Henningsen <stephan+buildroot@asklandd.dk>
---
 package/sudo/Config.in | 15 +++++++++++++++
 package/sudo/sudo.mk   | 13 +++++++++++++
 2 files changed, 28 insertions(+)

Comments

Yann E. MORIN Nov. 5, 2019, 6:51 p.m. UTC | #1
Stephan, All,

On 2019-11-03 22:41 +0100, Stephan Henningsen spake thusly:
> From: Stephan Henningsen <stephan+buildroot@asklandd.dk>
> 
> Signed-off-by: Stephan Henningsen <stephan+buildroot@asklandd.dk>
> ---
>  package/sudo/Config.in | 15 +++++++++++++++
>  package/sudo/sudo.mk   | 13 +++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/package/sudo/Config.in b/package/sudo/Config.in
> index cbef15d67b..403d634ceb 100644
> --- a/package/sudo/Config.in
> +++ b/package/sudo/Config.in
> @@ -9,3 +9,18 @@ config BR2_PACKAGE_SUDO
>  	  but still allow people to get their work done.
>  
>  	  http://www.sudo.ws/sudo/
> +
> +
> +if BR2_PACKAGE_SUDO
> +
> +config BR2_PACKAGE_SUDO_GROUP_AND_RULE
> +	bool "add group 'sudo' and enable associated sudo rule"
> +	select BR2_PACKAGE_SUDO_GROUP
> +	help
> +	  Creates a group named 'sudo', and enables the following rule
> +	  in the /etc/sudoers configuration file that allows members of
> +	  group 'sudo' to execute any command as root:
> +
> +	  %sudo ALL=(ALL) ALL

I thought the conclusion from the previous iteration was that the
addition of the group and sudo rules were to be non-optional.

> +endif
> diff --git a/package/sudo/sudo.mk b/package/sudo/sudo.mk
> index cf8b63b1db..5df39b193e 100644
> --- a/package/sudo/sudo.mk
> +++ b/package/sudo/sudo.mk
> @@ -64,4 +64,17 @@ define SUDO_PERMISSIONS
>  	/usr/bin/sudo f 4755 0 0 - - - - -
>  endef
>  
> +ifeq ($(BR2_PACKAGE_SUDO_GROUP_AND_RULE),y)
> +define SUDO_USERS
> +    -               -1   sudo            -1   -             -            -         -

When the username is '-', even the uid is ignored, we usually make it
'-' too. Also, there are too many spaces (see PERMISSIONS above).

So, I removed the condition (and thus the Config.in option), tweaked the
USERS variable, and pushed to master now.

Thanks!

Regards,
Yann E. MORIN.

> +endef
> +
> +define SUDO_ENABLE_SUDO_GROUP_RULE
> +	$(SED) '/^# \%sudo\tALL=(ALL) ALL/s/^# //' $(TARGET_DIR)/etc/sudoers
> +endef
> +
> +SUDO_POST_INSTALL_TARGET_HOOKS += SUDO_ENABLE_SUDO_GROUP_RULE
> +
> +endif
> +
>  $(eval $(autotools-package))
> -- 
> 2.17.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Stephan Henningsen Nov. 5, 2019, 8:42 p.m. UTC | #2
Hey,

On Tue, Nov 5, 2019 at 7:51 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> On 2019-11-03 22:41 +0100, Stephan Henningsen spake thusly:
> > +if BR2_PACKAGE_SUDO
> > +
> > +config BR2_PACKAGE_SUDO_GROUP_AND_RULE
> > +     bool "add group 'sudo' and enable associated sudo rule"
> > +     select BR2_PACKAGE_SUDO_GROUP
> > +     help
> > +       Creates a group named 'sudo', and enables the following rule
> > +       in the /etc/sudoers configuration file that allows members of
> > +       group 'sudo' to execute any command as root:
> > +
> > +       %sudo ALL=(ALL) ALL
>
> I thought the conclusion from the previous iteration was that the
> addition of the group and sudo rules were to be non-optional.

Sorry, I thought the idea was to combine the rule and group together
into one option, since they make perfect sense together, and aren't
very useful separately.

I don't agree that they should be non-optional (or default).

The sudo group+rule is just one of many use-cases for the sudo
program.  Others include granting only access to a specific user
(root, joe, etc.) or existing system groups (wheel, daemon, etc.).
People who rely on these features and are concerned about security,
would need to clean up the mess that the non-optional
BR2_PACKAGE_SUDO_GROUP_AND_RULE brought in, and revert the size of the
attack surface.  In fact, since this is now the default, most users
will most likely not notice this has been added.  If some already
added the 'sudo' system group and their own (different) rules, their
system might break or even open for permission elevation silently.

I don't see how anything good can come out of this.  (Except for my
particular case maybe, because I'm the one who had the need and made
the patch with the option ;).

I really think it should be an option, just like the patch provided.


> > +endif
> > diff --git a/package/sudo/sudo.mk b/package/sudo/sudo.mk
> > index cf8b63b1db..5df39b193e 100644
> > --- a/package/sudo/sudo.mk
> > +++ b/package/sudo/sudo.mk
> > @@ -64,4 +64,17 @@ define SUDO_PERMISSIONS
> >       /usr/bin/sudo f 4755 0 0 - - - - -
> >  endef
> >
> > +ifeq ($(BR2_PACKAGE_SUDO_GROUP_AND_RULE),y)
> > +define SUDO_USERS
> > +    -               -1   sudo            -1   -             -            -         -
>
> When the username is '-', even the uid is ignored, we usually make it
> '-' too.

Makes sense.


> Also, there are too many spaces (see PERMISSIONS above).

Even though I usually like spaces, I agree here =).  I don't know how
they got there.  Perhaps I had a header there for personal
documentation.


> So, I removed the condition (and thus the Config.in option), tweaked the
> USERS variable, and pushed to master now.

Please reconsider only cleaning up the USERS variable.  I think adding
potentially unused non-standard system groups, sudo rules, and
silenting, inadvertently allowing running commands as root is a bad
idea.

Patch
diff mbox series

diff --git a/package/sudo/Config.in b/package/sudo/Config.in
index cbef15d67b..403d634ceb 100644
--- a/package/sudo/Config.in
+++ b/package/sudo/Config.in
@@ -9,3 +9,18 @@  config BR2_PACKAGE_SUDO
 	  but still allow people to get their work done.
 
 	  http://www.sudo.ws/sudo/
+
+
+if BR2_PACKAGE_SUDO
+
+config BR2_PACKAGE_SUDO_GROUP_AND_RULE
+	bool "add group 'sudo' and enable associated sudo rule"
+	select BR2_PACKAGE_SUDO_GROUP
+	help
+	  Creates a group named 'sudo', and enables the following rule
+	  in the /etc/sudoers configuration file that allows members of
+	  group 'sudo' to execute any command as root:
+
+	  %sudo ALL=(ALL) ALL
+
+endif
diff --git a/package/sudo/sudo.mk b/package/sudo/sudo.mk
index cf8b63b1db..5df39b193e 100644
--- a/package/sudo/sudo.mk
+++ b/package/sudo/sudo.mk
@@ -64,4 +64,17 @@  define SUDO_PERMISSIONS
 	/usr/bin/sudo f 4755 0 0 - - - - -
 endef
 
+ifeq ($(BR2_PACKAGE_SUDO_GROUP_AND_RULE),y)
+define SUDO_USERS
+    -               -1   sudo            -1   -             -            -         -
+endef
+
+define SUDO_ENABLE_SUDO_GROUP_RULE
+	$(SED) '/^# \%sudo\tALL=(ALL) ALL/s/^# //' $(TARGET_DIR)/etc/sudoers
+endef
+
+SUDO_POST_INSTALL_TARGET_HOOKS += SUDO_ENABLE_SUDO_GROUP_RULE
+
+endif
+
 $(eval $(autotools-package))