[gentoo-dev] [PATCH 1/2] fcaps.eclass: introduce FCAPS_CAPS_MODE and FCAPS_NOCAPS_MODE

[gentoo-dev] [PATCH 1/2] fcaps.eclass: introduce FCAPS_CAPS_MODE and FCAPS_NOCAPS_MODE

[Mike Gilbert]

Signed-off-by: Mike Gilbert <floppym@gentoo.org>
---
 eclass/fcaps.eclass | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/eclass/fcaps.eclass b/eclass/fcaps.eclass
index 477e1e954ab8..bf05776ba760 100644
--- a/eclass/fcaps.eclass
+++ b/eclass/fcaps.eclass
@@ -1,4 +1,4 @@
-# Copyright 1999-2023 Gentoo Authors
+# Copyright 1999-2024 Gentoo Authors
 # Distributed under the terms of the GNU General Public License v2
 
 # @ECLASS: fcaps.eclass
@@ -66,6 +66,18 @@ esac
 #
 # Note: If you override pkg_postinst, you must call fcaps_pkg_postinst yourself.
 
+# @ECLASS_VARIABLE: FCAPS_CAPS_MODE
+# @USER_VARIABLE
+# @DESCRIPTION:
+# Mode to use when capabilities are supported.
+: ${FCAPS_CAPS_MODE:=0711}
+
+# @ECLASS_VARIABLE: FCAPS_NOCAPS_MODE
+# @USER_VARIABLE
+# @DESCRIPTION:
+# Mode to use when capabilities are not supported.
+: ${FCAPS_NOCAPS_MODE:=4711}
+
 # @FUNCTION: fcaps
 # @USAGE: [-o <owner>] [-g <group>] [-m <mode>] [-M <caps mode>] <capabilities> <file[s]>
 # @DESCRIPTION:
@@ -96,8 +108,8 @@ fcaps() {
 	# Process the user options first.
 	local owner='0'
 	local group='0'
-	local mode='4711'
-	local caps_mode='711'
+	local mode=${FCAPS_NOCAPS_MODE}
+	local caps_mode=${FCAPS_CAPS_MODE}
 
 	while [[ $# -gt 0 ]] ; do
 		case $1 in
-- 
2.47.0

[gentoo-dev] [PATCH 2/2] fcaps.eclass: make binaries readable by default

[Mike Gilbert]

Removing the read bit from suid binaries has questionable security
benefit, and may cause problems for some software.

Users may override FCAPS_CAPS_MODE and FCAPS_NOCAPS_MODE should they
desire the old behavior.

Bug: https://bugs.gentoo.org/938164
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
---
 eclass/fcaps.eclass | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/eclass/fcaps.eclass b/eclass/fcaps.eclass
index bf05776ba760..da4a52099396 100644
--- a/eclass/fcaps.eclass
+++ b/eclass/fcaps.eclass
@@ -70,13 +70,13 @@ esac
 # @USER_VARIABLE
 # @DESCRIPTION:
 # Mode to use when capabilities are supported.
-: ${FCAPS_CAPS_MODE:=0711}
+: ${FCAPS_CAPS_MODE:=0755}
 
 # @ECLASS_VARIABLE: FCAPS_NOCAPS_MODE
 # @USER_VARIABLE
 # @DESCRIPTION:
 # Mode to use when capabilities are not supported.
-: ${FCAPS_NOCAPS_MODE:=4711}
+: ${FCAPS_NOCAPS_MODE:=4755}
 
 # @FUNCTION: fcaps
 # @USAGE: [-o <owner>] [-g <group>] [-m <mode>] [-M <caps mode>] <capabilities> <file[s]>
-- 
2.47.0

Re: [gentoo-dev] [PATCH 2/2] fcaps.eclass: make binaries readable by default

[Eli Schwartz]

[-- Attachment #1.1: Type: text/plain, Size: 2106 bytes --]

On 11/10/24 4:54 PM, Mike Gilbert wrote:
> Removing the read bit from suid binaries has questionable security
> benefit, and may cause problems for some software.
> 
> Users may override FCAPS_CAPS_MODE and FCAPS_NOCAPS_MODE should they
> desire the old behavior.
> 
> Bug: https://bugs.gentoo.org/938164
> Signed-off-by: Mike Gilbert <floppym@gentoo.org>
> ---
>  eclass/fcaps.eclass | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/eclass/fcaps.eclass b/eclass/fcaps.eclass
> index bf05776ba760..da4a52099396 100644
> --- a/eclass/fcaps.eclass
> +++ b/eclass/fcaps.eclass
> @@ -70,13 +70,13 @@ esac
>  # @USER_VARIABLE
>  # @DESCRIPTION:
>  # Mode to use when capabilities are supported.
> -: ${FCAPS_CAPS_MODE:=0711}
> +: ${FCAPS_CAPS_MODE:=0755}


Considering the context of the linked bug, and the change offered here,
I don't really understand the proposed solution.

This is a very flexible variable. Way too flexible. There is no valid
use case for setting it to anything other than removing read
permissions, or preserving read permissions -- so why is it acceptable
to offer users the opportunity to define

FCAPS_CAPS_MODE="4123"
FCAPS_NOCAPS_MODE="0644"

Which is an error condition?

If we want a user variable at all here, let it be

: ${FCAPS_DENY_WORLD_READ:=no}

But I'm not convinced any optionality is necessary at all here. If the
expected behavior is to have read, and users are free to toggle sfperms
at the portage level, why is it necessary to make this additionally
configurable as an eclass variable?

Either way, there is also a stale comment in the function body:

# If everything goes well, we don't want the file to be readable
# by people.



>  # @ECLASS_VARIABLE: FCAPS_NOCAPS_MODE
>  # @USER_VARIABLE
>  # @DESCRIPTION:
>  # Mode to use when capabilities are not supported.
> -: ${FCAPS_NOCAPS_MODE:=4711}
> +: ${FCAPS_NOCAPS_MODE:=4755}
>  
>  # @FUNCTION: fcaps
>  # @USAGE: [-o <owner>] [-g <group>] [-m <mode>] [-M <caps mode>] <capabilities> <file[s]>


-- 
Eli Schwartz

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [gentoo-dev] [PATCH 2/2] fcaps.eclass: make binaries readable by default

[Mike Gilbert]

On Sun, Nov 10, 2024 at 5:38 PM Eli Schwartz <eschwartz@gentoo.org> wrote:
>
> On 11/10/24 4:54 PM, Mike Gilbert wrote:
> > Removing the read bit from suid binaries has questionable security
> > benefit, and may cause problems for some software.
> >
> > Users may override FCAPS_CAPS_MODE and FCAPS_NOCAPS_MODE should they
> > desire the old behavior.
> >
> > Bug: https://bugs.gentoo.org/938164
> > Signed-off-by: Mike Gilbert <floppym@gentoo.org>
> > ---
> >  eclass/fcaps.eclass | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/eclass/fcaps.eclass b/eclass/fcaps.eclass
> > index bf05776ba760..da4a52099396 100644
> > --- a/eclass/fcaps.eclass
> > +++ b/eclass/fcaps.eclass
> > @@ -70,13 +70,13 @@ esac
> >  # @USER_VARIABLE
> >  # @DESCRIPTION:
> >  # Mode to use when capabilities are supported.
> > -: ${FCAPS_CAPS_MODE:=0711}
> > +: ${FCAPS_CAPS_MODE:=0755}
>
>
> Considering the context of the linked bug, and the change offered here,
> I don't really understand the proposed solution.
>
> This is a very flexible variable. Way too flexible. There is no valid
> use case for setting it to anything other than removing read
> permissions, or preserving read permissions -- so why is it acceptable
> to offer users the opportunity to define
>
> FCAPS_CAPS_MODE="4123"
> FCAPS_NOCAPS_MODE="0644"
>
> Which is an error condition?
>
> If we want a user variable at all here, let it be
>
> : ${FCAPS_DENY_WORLD_READ:=no}

Good point, I can make this adjustment.

> But I'm not convinced any optionality is necessary at all here. If the
> expected behavior is to have read, and users are free to toggle sfperms
> at the portage level, why is it necessary to make this additionally
> configurable as an eclass variable?

FEATURES="sfperms" modifies the files during the merge phase.

fcaps.eclass modifies the files in pkg_postinst, after the merge phase
is complete.
It does this because there is no guarantee that ${D} will reside on a
filesystem that supports file-based capabilities.

I don't think we want to have fcaps.eclass start looking at FEATURES
since this is undefined in PMS.