[PATCH] Removed unneeded access().

[PATCH] Removed unneeded access().

[Pasi Savanainen]

* ctl.c: access() is not needed because previous open() will fail
if user has no read write accees to a command channel file.
Actually existance of access() check generates a misleading error:
"Error, 'stap_26013' (stap_26013) is not a zombie systemtap module."
when stap is runned by regular user (stapusr group)
and /sys/kernel/debug is mounted as 0700.
---
 runtime/staprun/ctl.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/runtime/staprun/ctl.c b/runtime/staprun/ctl.c
index 1ffc8e3..96c140d 100644
--- a/runtime/staprun/ctl.c
+++ b/runtime/staprun/ctl.c
@@ -30,15 +30,6 @@ int init_ctl_channel(const char *name, int verb)
 	control_channel = open(buf, O_RDWR);
 	dbug(2, "Opened %s (%d)\n", buf, control_channel);
 
-/* It's actually safe to do this check before the open(),
- * as the file we're trying to access connot be modified
- * by a typical user.
- */
-	if (access(buf, R_OK|W_OK) != 0){
-		close(control_channel);
-		return -5;
-	}
-
 	if (control_channel < 0) {
 		if (verb) {
 			if (attach_mod && errno == ENOENT)
-- 
1.7.5.4

Re: [PATCH] Removed unneeded access().

[Josh Stone]

On 06/13/2012 05:01 AM, Pasi Savanainen wrote:
> * ctl.c: access() is not needed because previous open() will fail
> if user has no read write accees to a command channel file.

Sorry, this is not true, because staprun is setuid-root.  The open()
should always succeed if the file exists, due to the effective UID 0,
but access() is making sure that the *real* UID (of the original user)
also has permissions for that file.

Later when the control channel is opened by stapio, your statement about
open() and access() is true, but we must be careful with staprun.

> Actually existance of access() check generates a misleading error:
> "Error, 'stap_26013' (stap_26013) is not a zombie systemtap module."
> when stap is runned by regular user (stapusr group)
> and /sys/kernel/debug is mounted as 0700.

This is true - regular users won't be able to reach anything underneath
if debugfs is mounted this way.  My first thought is, "Don't do that!"

We could at least clean up the error messages though.  I think we could
just add an access("/sys/kernel/debug", X_OK) check in staprun_funcs.c
mountfs() to make sure the user will be able to get that far.

It might also be possible to solve this more fully.  We could open the
directory in staprun and do the access check with faccessat().  Then
we'd have to also pass a control_channel fd across the exec to stapio,
since it can't open that path itself.  Given the security concerns about
all this though, I'd want stronger motivation for tinkering here.

Josh

Re: [PATCH] Removed unneeded access().

[Pasi Savanainen]

On Jun 13, 2012, at 8:13 PM, Josh Stone wrote:

> On 06/13/2012 05:01 AM, Pasi Savanainen wrote:
>> * ctl.c: access() is not needed because previous open() will fail
>> if user has no read write accees to a command channel file.
> 
> Sorry, this is not true, because staprun is setuid-root.  The open()
> should always succeed if the file exists, due to the effective UID 0,
> but access() is making sure that the *real* UID (of the original user)
> also has permissions for that file.
> 
Yes, very true. I did't think about that.

Actually my real problem was not an error message but it hides the fact from me that that 
loaded module couldn't be unloaded by staprun anymore. I had to manually remove the module.

> Later when the control channel is opened by stapio, your statement about
> open() and access() is true, but we must be careful with staprun.
> 
>> Actually existance of access() check generates a misleading error:
>> "Error, 'stap_26013' (stap_26013) is not a zombie systemtap module."
>> when stap is runned by regular user (stapusr group)
>> and /sys/kernel/debug is mounted as 0700.
> 
> This is true - regular users won't be able to reach anything underneath
> if debugfs is mounted this way.  My first thought is, "Don't do that!"
> 

I'm running Ubuntu 11.10 and it mounts debugfs by default as 0700.

Br,

Pasi

Re: [PATCH] Removed unneeded access().

[Josh Stone]

On 06/13/2012 11:39 PM, Pasi Savanainen wrote:
> Actually my real problem was not an error message but it hides the
> fact from me that that loaded module couldn't be unloaded by staprun
> anymore. I had to manually remove the module.

Ah, right, because after failing to open it normally, we call
remove_module, and that's what gives up with the zombie message.  I
think that a new access() check as I mentioned on /sys/kernel/debug/
could be done before the module is ever loaded, to squash this earlier.

> I'm running Ubuntu 11.10 and it mounts debugfs by default as 0700.

That's unfortunate, but I can kind of understand that paranoia.  We
should at least figure out and document a remount command for stap fans
to use, and then perhaps later we'll have to really do that extra work
to pass file descriptors around.  Ugh.

I'll file a few bugs, and try to add that access() check at least.


Josh