[Buildroot] [RFC PATCH v4 1/2] makedevs: only warn when xattr support disabled

[Buildroot] [RFC PATCH v4 1/2] makedevs: only warn when xattr support disabled

[Petr Vorel]

Previously makedevs failed when xattr configuration defined while
disabled BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES.  Thus
check for this configuration would be required in each use in packages.
Changing makedevs to only print warning with number of ignored lines, so
package configuration don't have to deal with
BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES.

No package is using this feature so far (it's used only in tests),
thus no need to change any package config.

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
Changes v3->v4 (Addressed Yann's issues):
* s/discharged/ignored/
* exit with EXIT_SUCCESS (no need to abort fakeroot script just due
ignored xattr lines)

 package/makedevs/makedevs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
index c57b964f5c..dbe5b13714 100644
--- a/package/makedevs/makedevs.c
+++ b/package/makedevs/makedevs.c
@@ -493,6 +493,7 @@ int main(int argc, char **argv)
 		printf("table=<stdin>\n");
 	}
 
+	unsigned int xattr_warned = 0;
 	while ((line = bb_get_chomped_line_from_file(table))) {
 		char type;
 		unsigned int mode = 0755;
@@ -518,9 +519,7 @@ int main(int argc, char **argv)
 			if (bb_set_xattr(full_name, xattr) < 0)
 				bb_error_msg_and_die("can't set cap %s on file %s\n", xattr, full_name);
 #else
-			bb_error_msg_and_die("line %d not supported: '%s'\nDid you forget to enable "
-					     "BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES?\n",
-					     linenum, line);
+			xattr_warned++;
 #endif /* EXTENDED_ATTRIBUTES */
 			continue;
 		}
@@ -641,6 +640,13 @@ int main(int argc, char **argv)
 loop:
 		free(line);
 	}
+
+	if (xattr_warned)
+			bb_error_msg("%u lines with xattr configuration ignored, enable "
+					     "BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES "
+						 "to get xattr support\n",
+					     xattr_warned);
+
 	fclose(table);
 
 	return ret;
-- 
2.22.0

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Petr Vorel]

Not setting for arping as it can be used for ARP Poisoning.

Use cap_net_raw+p (drop +e) as upstream sets that via
cap_set_flag(), see https://github.com/iputils/iputils/issues/194

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 package/iputils/iputils.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
index 8e6a3e2fc5..f1d3e1fc6a 100644
--- a/package/iputils/iputils.mk
+++ b/package/iputils/iputils.mk
@@ -76,8 +76,11 @@ IPUTILS_CONF_OPTS += -DNO_SETCAP_OR_SUID=true
 define IPUTILS_PERMISSIONS
 	/usr/sbin/arping      f 4755 0 0 - - - - -
 	/usr/bin/clockdiff    f 4755 0 0 - - - - -
+	|xattr cap_net_raw+p
 	/bin/ping             f 4755 0 0 - - - - -
+	|xattr cap_net_raw+p
 	/usr/bin/traceroute6  f 4755 0 0 - - - - -
+	|xattr cap_net_raw+p
 endef
 
 $(eval $(meson-package))
-- 
2.22.0

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Yann E. MORIN]

Petr, All,

On 2019-07-30 23:38 +0200, Petr Vorel spake thusly:
> Not setting for arping as it can be used for ARP Poisoning.
> 
> Use cap_net_raw+p (drop +e) as upstream sets that via
> cap_set_flag(), see https://github.com/iputils/iputils/issues/194

So, now we set the capabilities to those exectuables, do they still need
to be setuid?

But then, if one really does not want xattr, setuid is still required.

So, we have no way to express that a file should have either setuid or
xattrs, except as a big if-block like:

    ifeq ($(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),y)
    define IPUTILS_PERMISSIONS
        /usr/bin/clockdiff    f 0755 0 0 - - - - -
        |xattr cap_net_raw+p
    endef
    else
    define IPUTILS_PERMISSIONS
        /usr/bin/clockdiff    f 4755 0 0 - - - - -
    endef
    endif

... which is what we were trying to avoid in the firstplace...

We could write something like:

    /usr/bin/clockdiff    f $(MAYBE_SUID)755 0 0 - - - - -
    |xattr cap_net_raw+p

Where MAYBE_SUID would be set as:

    MAYBE_SUID = $(if $(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),0,4)

But it is starting to be a bit more complex than what you initially
envisionned, I guess.

Regards,
Yann E. MORIN.

> Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
> ---
>  package/iputils/iputils.mk | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
> index 8e6a3e2fc5..f1d3e1fc6a 100644
> --- a/package/iputils/iputils.mk
> +++ b/package/iputils/iputils.mk
> @@ -76,8 +76,11 @@ IPUTILS_CONF_OPTS += -DNO_SETCAP_OR_SUID=true
>  define IPUTILS_PERMISSIONS
>  	/usr/sbin/arping      f 4755 0 0 - - - - -
>  	/usr/bin/clockdiff    f 4755 0 0 - - - - -
> +	|xattr cap_net_raw+p
>  	/bin/ping             f 4755 0 0 - - - - -
> +	|xattr cap_net_raw+p
>  	/usr/bin/traceroute6  f 4755 0 0 - - - - -
> +	|xattr cap_net_raw+p
>  endef
>  
>  $(eval $(meson-package))
> -- 
> 2.22.0
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Petr Vorel]

Hi Yann,

> So, now we set the capabilities to those exectuables, do they still need
> to be setuid?

> But then, if one really does not want xattr, setuid is still required.

> So, we have no way to express that a file should have either setuid or
> xattrs, except as a big if-block like:

>     ifeq ($(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),y)
>     define IPUTILS_PERMISSIONS
>         /usr/bin/clockdiff    f 0755 0 0 - - - - -
>         |xattr cap_net_raw+p
>     endef
>     else
>     define IPUTILS_PERMISSIONS
>         /usr/bin/clockdiff    f 4755 0 0 - - - - -
>     endef
>     endif

> ... which is what we were trying to avoid in the firstplace...

> We could write something like:

>     /usr/bin/clockdiff    f $(MAYBE_SUID)755 0 0 - - - - -
>     |xattr cap_net_raw+p

> Where MAYBE_SUID would be set as:

>     MAYBE_SUID = $(if $(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),0,4)
Good point, I fixed it in v5 (with whitespace).

> But it is starting to be a bit more complex than what you initially
> envisionned, I guess.
Yep :(. But your solution is good enough, thank you!


Kind regards,
Petr

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Thomas Petazzoni]

Hello,

On Wed, 31 Jul 2019 18:00:59 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > Use cap_net_raw+p (drop +e) as upstream sets that via
> > cap_set_flag(), see https://github.com/iputils/iputils/issues/194  
> 
> So, now we set the capabilities to those exectuables, do they still need
> to be setuid?
> 
> But then, if one really does not want xattr, setuid is still required.

Ah, yes, indeed.

> So, we have no way to express that a file should have either setuid or
> xattrs, except as a big if-block like:
> 
>     ifeq ($(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),y)
>     define IPUTILS_PERMISSIONS
>         /usr/bin/clockdiff    f 0755 0 0 - - - - -
>         |xattr cap_net_raw+p
>     endef
>     else
>     define IPUTILS_PERMISSIONS
>         /usr/bin/clockdiff    f 4755 0 0 - - - - -
>     endef
>     endif
> 
> ... which is what we were trying to avoid in the firstplace...

Yes, but I believe it's the best solution for now, let's keep a
conditional like you're showing here. Which of course makes the change
to makedevs no longer relevant.

I really hope Petr is not going to hate us for all the discussion, back
and forth and change of mind/opinion about this topic :-/

> We could write something like:
> 
>     /usr/bin/clockdiff    f $(MAYBE_SUID)755 0 0 - - - - -
>     |xattr cap_net_raw+p
> 
> Where MAYBE_SUID would be set as:
> 
>     MAYBE_SUID = $(if $(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),0,4)
> 
> But it is starting to be a bit more complex than what you initially
> envisionned, I guess.

Meh, no, please not like this.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Petr Vorel]

Hi Thomas, Yann,

I've sent v5 before your mail, sorry for forgetting to Cc you.
> Hello,

> On Wed, 31 Jul 2019 18:00:59 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > > Use cap_net_raw+p (drop +e) as upstream sets that via
> > > cap_set_flag(), see https://github.com/iputils/iputils/issues/194  

> > So, now we set the capabilities to those exectuables, do they still need
> > to be setuid?

> > But then, if one really does not want xattr, setuid is still required.

> Ah, yes, indeed.

> > So, we have no way to express that a file should have either setuid or
> > xattrs, except as a big if-block like:

> >     ifeq ($(BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES),y)
> >     define IPUTILS_PERMISSIONS
> >         /usr/bin/clockdiff    f 0755 0 0 - - - - -
> >         |xattr cap_net_raw+p
> >     endef
> >     else
> >     define IPUTILS_PERMISSIONS
> >         /usr/bin/clockdiff    f 4755 0 0 - - - - -
> >     endef
> >     endif

> > ... which is what we were trying to avoid in the firstplace...

> Yes, but I believe it's the best solution for now, let's keep a
> conditional like you're showing here. Which of course makes the change
> to makedevs no longer relevant.
Sure :). So merge the original version [1], related only to iputils?

> I really hope Petr is not going to hate us for all the discussion, back
> and forth and change of mind/opinion about this topic :-/
No, not at all :).

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/patch/1138055/

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Yann E. MORIN]

Petr, All,

On 2019-08-01 00:24 +0200, Petr Vorel spake thusly:
> > On Wed, 31 Jul 2019 18:00:59 +0200
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
[--SNIP--]
> > Yes, but I believe it's the best solution for now, let's keep a
> > conditional like you're showing here. Which of course makes the change
> > to makedevs no longer relevant.
> Sure :). So merge the original version [1], related only to iputils?

Not really, because your first iteration still kept the suid bit on the
orograms.

I'm intrigued why you could not use the ifeq(...) construct, though...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [RFC PATCH v4 2/2] iputils: add capability for clockdiff, ping, traceroute6

[Petr Vorel]

Hi Yann, Thomas,

> Petr, All,

> On 2019-08-01 00:24 +0200, Petr Vorel spake thusly:
> > > On Wed, 31 Jul 2019 18:00:59 +0200
> > > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> [--SNIP--]
> > > Yes, but I believe it's the best solution for now, let's keep a
> > > conditional like you're showing here. Which of course makes the change
> > > to makedevs no longer relevant.
> > Sure :). So merge the original version [1], related only to iputils?

> Not really, because your first iteration still kept the suid bit on the
> orograms.

> I'm intrigued why you could not use the ifeq(...) construct, though...
Oh, sorry. I'll post v6.

> Regards,
> Yann E. MORIN.

Kind regards,
Petr