On Thu, Aug 22, 2013 at 12:05:50PM -0700, Andy Lutomirski wrote:

On Thu, Aug 22, 2013 at 11:53 AM, Willy Tarreau [off-list ref] wrote:

quoted

On Thu, Aug 22, 2013 at 11:48:10AM -0700, Linus Torvalds wrote:

quoted

And I'm wondering if we shouldn't actually do that at "path_init"
time. Right now the code says:

                /* Caller must check execute permissions on the
starting path component */
                struct fd f = fdget_raw(dfd);

and then uses the struct file mindlessly.

I'm wondering if we should just do some validation in that place, and say:

 - for directories, we require exec permissions here
 - for everything else, we require that f->f_cred == current->cred check.

Does this work for the procfs case?  As far as I understand it (which
isn't saying much), it goes through the symlink-following path.

Indeed I checked yesterday that it seems to use proc_pid_follow_link() for
fd/, cwd, root and exe, which means the same tests are used everywhere.

quoted

quoted

I dunno. But that I_LINKABLE thing just bothers me. It screams "I'm
hacky" to me.

I agreed, simply because the condition here is different from the one in /proc.

I have read some code last evening to try to understand how /proc/pid/fd
entries were granted access to various processes, because I would love to
see the same condition being used in both places. Unfortunately, it's beyond
my skills, and I stopped after my random attempts gave me some panics.

What if we added another field to struct nameidata that's indicates
what restrictions need to be enforced when following magical symlinks
and then enforcing them when nd_jump_link gets used.  (There are only
two of these, both in procfs.)

I tried to add a test based on a mount option before this call to
nd_jump_link() when I realized my attempt was a total disaster because
I'm a noob. But what I think would be nice (at least as an opt-in) would
be :

  - processes which don't share the same root should not be allowed to
    access files through /proc/pid/{root,cwd,exe,fd/*}

  - keep the current restrictions as well of course

  - the exact same restrictions should apply to AT_EMPTY_PATH

I might be totally wrong, but as a user that's what I find natural and
what I tend to expect.

Then open could check that the original fd was opened for a superset
of the requested permissions (or that the caller has
CAP_DAC_OVERRIDE), linkat could check whatever it feels like checking,
etc.

Do not forget that 2 other syscalls seem to support AT_EMPTH_PATH as
well if that makes a difference.

This would allow all of these issues to be fixed for real (controlled
by sysctl, presumably).

If needed for backwards compatibility, possibly, though I doubt that
there are apps that *rely* on the current lack of isolation between
chroots. But at the same time I hate to break existing setups :-)

--Andy

Willy