On 08/14/2012 04:53 PM, Bernhard Voelker wrote:
On 08/14/2012 02:47 PM, Ondrej Oprala wrote:
On 08/10/2012 08:53 AM, Bernhard Voelker wrote:
On 08/09/2012 05:43 PM, Ondrej Oprala wrote:
Hi, I think I got a fix for this bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=556358.
I added a bit of permission checking to require_root_ so no tests have
to be rewriten.
Have a nice day :) ,
Ondrej
Hi Ondrej,
+setuidgid_has_perm_()
+{
+
+ cat << \EOF > cmds.tmp
+ IFS=:
+ for DIR in $PATH; do
+ test -x $DIR || exit 1
+ done
+ exit 0
+EOF
+
+ su -s /bin/sh $NON_ROOT_USERNAME < cmds.tmp
+
+ RET=$?
+ return $RET
+}
+
just a thought: if setuidgid is part of the test failure,
then why not using setuidgid in here?
Furthermore: the problem is finding the correct binary, right?
E.g. in tests/rm/fail-2eperm there is already such a test:
# Try to ensure that $NON_ROOT_USERNAME can access
# the required version of rm.
rm_version=$(
setuidgid $NON_ROOT_USERNAME env PATH="$PATH" rm --version |
sed -n '1s/.* //p'
)
case $rm_version in
$PACKAGE_VERSION) ;;
*) skip_ "cannot access just-built rm as user $NON_ROOT_USERNAME";;
esac
Have a nice day,
Berny
Well, I wanted to satisfy the requirements from comment #6 as much as
possible
and didn't want to change anything in the existing tests (such as
passing args to require_root_ )
to make it more automatic. If the check were to act on setuidgid output
like in the rm test, it'd either
have to be changed in all tests or require_root_ would need additional
arguments (which are again
changes in the tests). But if it needs to be changed that way, I will,
of course, redo it.
Hi Ondrej,
I think grep-ing for "setuidgid" in $0 is the right thing
in order to avoid changing all root-tests.
I just thought that setuidgid_has_perm_() should use the same
same mechanism via setuidgid as the actual test. This is just
to avoid potential problems if there are any discrepancies
between "the su way" and "the setuidgid way" - today or in
future.
And I think you wouldn't have to pass an extra arg - the CU
program? - to require_root_: it doesn't matter which program
is used to check the version. If the right version of e.g.
rm is found, then also the right version of e.g. touch would
be used.
The question I'm still asking myself is: is the only problem
to use the correct binary in the test, or do you also want
to ensure that the current directory is accessible.
To check both, I thought of something like this:
setuidgid_has_perm_()
{
# Try to ensure that $NON_ROOT_USERNAME can access
# the required version of CU binaries.
local rm_version=$(
setuidgid $NON_ROOT_USERNAME env PATH="$PATH" rm --version |
sed -n '1s/.* //p'
)
case "_${rm_version}_" in
_$PACKAGE_VERSION}_) ;;
*) return 1;;
esac
# Try to ensure that $NON_ROOT_USERNAME can access
# the temporary test directory.
setuidgid $NON_ROOT_USERNAME env PATH="$PATH" touch foo
[ -f foo ] || return 1
rm -f foo
return 0
}
Have a nice day,
Berny
Hi,
You're right about the rm test. I originally wanted to make the check by
checking
if $NON_ROOT_USERNAME has execute permissions for the src folder, but this
looks cleaner. There's still the very rare case of $NON_ROOT_USERNAME
having execute permissions for rm but not having them for other binaries
using
setuidgid, but that is something that would have to be done purposefully and
personally I don't see any reason to do that.
Anyway, the attachment should hopefully have it right.
Cheers,
Ondrej.
From b5a8fa324d089323f0af02f4eb7c8bfdc86b4337 Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <[email protected]>
Date: Thu, 9 Aug 2012 15:40:39 +0200
Subject: [PATCH] tests: Add error checking for root-only tests running
setuidgid
* NEWS: Mention the fix.
* tests/init.cfg: Modify the require_root_ function to check
for setuidgid calls and proper permissions.
---
NEWS | 6 ++++++
tests/init.cfg | 19 +++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/NEWS b/NEWS
index 46d0a41..173f861 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,12 @@ GNU coreutils NEWS -*-
outline -*-
certain options like -a, -l, -t and -x.
[This bug was present in "the beginning".]
+** Bug fixes
+
+ root-only tests now properly check for permissions of dummy
+ user $NON_ROOT_USERNAME before trying to run binaries from the
+ src dir.
+
* Noteworthy changes in release 8.18 (2012-08-12) [stable]
diff --git a/tests/init.cfg b/tests/init.cfg
index 4ff5ad4..2d0fe10 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -341,11 +341,30 @@ or use the shortcut target of the toplevel Makefile,
fi
}
+setuidgid_has_perm_()
+{
+ local rm_version=$(
+ setuidgid $NON_ROOT_USERNAME env PATH="$PATH" rm --version |
+ sed -n 'ls/.* //p'
+ )
+ case "_${rm_version}_" in
+ _$PACKAGE_VERSION}_) ;;
+ *) return 1;;
+ esac
+}
+
require_root_()
{
uid_is_privileged_ || skip_ "must be run as root"
NON_ROOT_USERNAME=${NON_ROOT_USERNAME=nobody}
NON_ROOT_GROUP=${NON_ROOT_GROUP=$(id -g $NON_ROOT_USERNAME)}
+
+ #if test contains a setuidgid call...
+ grep '^[ ]*setuidgid' "../$0"
+ if [ "$?" = "0" ]; then
+ setuidgid_has_perm_ ||
+ skip_ "user $NON_ROOT_USERNAME lacks execute permissions"
+ fi
}
skip_if_root_() { uid_is_privileged_ && skip_ "must be run as non-root"; }
--
1.7.11.2
