[RFC,v4,2/2] iputils: add capability for clockdiff, ping, traceroute6
diff mbox series

Message ID 20190730213846.7488-2-petr.vorel@gmail.com
State Superseded
Delegated to: Yann E. MORIN
Headers show
Series
  • [RFC,v4,1/2] makedevs: only warn when xattr support disabled
Related show

Commit Message

Petr Vorel July 30, 2019, 9:38 p.m. UTC
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(+)

Comments

Yann E. MORIN July 31, 2019, 4 p.m. UTC | #1
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
>
Petr Vorel July 31, 2019, 8:11 p.m. UTC | #2
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
Thomas Petazzoni July 31, 2019, 10:13 p.m. UTC | #3
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
Petr Vorel July 31, 2019, 10:24 p.m. UTC | #4
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/
Yann E. MORIN Aug. 1, 2019, 7:29 a.m. UTC | #5
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.
Petr Vorel Aug. 1, 2019, 7:33 a.m. UTC | #6
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

Patch
diff mbox series

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))