| 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 | expand |
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 >
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
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
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/
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.
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
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))
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(+)