Re: [gentoo-dev] eclass for handling of file-based capabilities

[Brian Harring]

On Sat, Mar 05, 2011 at 02:24:22PM +0100, Constanze Hausner wrote:
> fcaps() {
> 	debug-print-function ${FUNCNAME} "$@"
> 	debug-print "${FUNCNAME}: Trying to set capabilities for ${4}"
> 	local uid_gid=$1
> 	local perms=$2
> 	export fallbackFileMode=$perms
> 	local capset=$3
> 	local path=$4

That export of fallbackFileMode looks pointless... clarify please.  
Also, your arg count checking here means it's going to throw some 
weird ass errors if people invoke it incorrectly.  You likely want a 

[ $# -ne 4 ] && die "wrong arg count"

thrown in there..


> 	if [ $# -eq 5 ]; then
> 		local set_mode=$5
> 	else
> 		debug-print "${FUNCNAME}: no set-mode provided, setting it to ep"
> 		#if there is no set_mode provided, it is set to true
> 		local set_mode=1
> 	fi
>
> 	#set owner/group
> 	debug-print "${FUNCNAME}: setting owner and group to ${uid_gid}"
> 	chown $uid_gid $path
> 	if [ $? -ne 0 ]; then
> 		eerror "chown "$uid_gid" "$path" failed."
> 		return 2
> 	fi
>
> 	#set file-mode including suid
> 	debug-print "${FUNCNAME}: setting file-mode ${perms}, including suid"
> 	chmod $perms $path

Arbitrary/user-controlled path's always need to be quoted...

> 	if [ $? -ne 0 ]; then
> 		eerror "chmod "$perms" "$path" failed."
> 		return 3
> 	fi
>
> 	#if filecaps is not enabled all is done
> 	use !filecaps && return 0
>
> 	#if libcap is not installed caps cannot be set
> 	if [ ! -f "/sbin/setcap" ]; then
> 		debug-print "${FUNCNAME}: libcap not installed, could not set caps"
> 		return 4
> 	fi
>
> 	#Check for set mode
> 	if [ $set_mode -eq 1 ]; then
> 		debug-print "${FUNCNAME}: set-mode = ep"
> 		local sets="=ep"
> 	else
> 		debug-print "${FUNCNAME}: set-mode = ei"
> 		local sets="=ei"
> 	fi

Shift this into the initial arg processing; eliminates the need for 
set_mode and is simpler to follow.

>
> 	#set the capability
> 	debug-print "${FUNCNAME}: setting capabilities"

lay off the debug-print's a bit...

> 	setcap "$capset""$sets" "$path" &> /dev/null
>
> 	#check if the capabilitiy got set correctly
> 	debug-print "${FUNCNAME}: checking capabilities"
> 	setcap -v "$capset""$sets" "$path" &> /dev/null
>
> 	local res=$?
>
> 	#if caps could be set, remove suid-bit
> 	if [ $res -eq 0 ]; then
> 		debug-print "${FUNCNAME}: caps were set, removing suid-bit"
> 		chmod -s $path
> 	else
> 		debug-print "${FUNCNAME}: caps could not be set"
> 		ewarn "setcap "$capset" "$path" failed."
> 		ewarn "Check your kernel and filesystem."
> 		ewarn "Fallback file-mode was set."
> 	fi
>
> 	return $res
> }

I'd take a different approach here; this code basically assumes that 
the PM knows of it- note the chmod -s.  The use flag protection you 
tried adding, without some profile hacks, is user modifiable- meaning 
users can flip it on even if the PM doesn't support it.

Or consider that the code above is purely doing it's thing during the 
install phase, specifically against whatever filesystem is used for 
building- while capabilities might be able to be set there, it's 
possible the final merge location won't support it.  End result of 
that is you'll get a setuid stripped binary merged to the 
livefs lacking the caps- borkage.  Or consider the inverse- the 
buildroot can't do capabilities, but the livefs could.  You get the 
idea.

Instead, write the code so the PM has to export a marker in some 
fashion to explicitly state "yes, I can do capabilities"- I'm 
specifically suggestining checking for a callback function exposed to 
the env.

If that function isn't there, then the PM can't do it- end of story.  
If it is, the PM takes the args and will try to apply the 
capabilities at the correct time- stripping setuid/setgid if it 
succeeds.

Please go that route; and please do not stick "portage" into the 
function name, something generic we can use for a later EAPI is 
better.

Implementing it as I suggested has the nice side affect of not being 
limited by PMS also, although it's an approach that still requires 
planning for compatibility.

Thanks-
~brian