Re: [kernel-hardening] [RFC v1] procfs mount options

[Vasiliy Kulikov <segoon@openwall.com>]

[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]

Solar,

On Sun, Jun 05, 2011 at 23:26 +0400, Solar Designer wrote:
> On Sun, Jun 05, 2011 at 10:24:31PM +0400, Vasiliy Kulikov wrote:
> > This patch introduces support of procfs mount options and adds mount
> > options to restrict access to /proc/PID/ directories and /proc/PID/net/
> > contents.  The default backward-compatible behaviour is left untouched.
> > 
> > The first mount option is called "hidepid" and its value defines how much
> > info about processes we want to be available for non-owners:
> > 
> > hidepid=0 (default) means the current behaviour - anybody may read all
> > /proc/PID/* files.
> 
> Aren't some /prod/PID/* files restricted by default, in stock kernels?
> I think several are (auxv, fd/, mem).  So perhaps re-word this.

Yes, I've mentioned still accessible files in hidepid=1.  Will do.

> > hidepid=1 means all files not running as current user and group are
> 
> "... files not running ..." needs to be re-worded.

Oops, made a mistake while rewording.

> > TODO/thoughs:
> >   - /proc/pid/net/ currently doesn't show ANYTHING, even "." and "..".
> >     This is confusing :)
> 
> Ouch.  Can't you simply restrict its perms such that this directory
> can't be listed unless you have privs?

Well, yes, but it would touch too many code - currently it is handled as
an entry in a static table.  Changing this would touch many high level
loops of the table handling.  Hiding its contents is just simplier.

Another solution - create a fake net namespace and process this
namespace if not enough permissions :)  It also removes weird netstat
errors like "seems like networking was disabled for this kernel".

>  It should act as a normal mode
> 550 directory on a regular filesystem.

What 550 perms would give?  /proc/pid/net/ contains all network
information about _all_ network connections in current net namespace.
So, /proc/1/net and /proc/2/net are logically the same directory.

However, changing the mode to 550 _and_ changing uid and gid will help.

> >   - need to alter "(inode->i_mode == (S_IFDIR|S_IRUGO|S_IXUGO))" checks
> >     to honour non-pid directories.
> 
> I see this in the patch, but can't really comment without reviewing the
> code in full context myself.

As Spender commented, this is a weird thing to recognize pids from other
files, which is needed due to some weird caching :)

> >   - what if one keeps open /proc/PID/ while executing set*id/capable
> >     binary?
> 
> Then they deliberately grant this privilege to this process (and maybe
> to its heirs).  I see no problem with that.

Ehh...  I mean another thing:

Process A with UID=1000 opens /proc/123/, while 123 has UID=1000.

123 exec's setuid binary, /proc/123/ becomes unaccessible to A.

However, A still keeps the directory opened and may read its contents.

> > +	if (pid->hide_net &&
> > +	    (!capable(CAP_NET_ADMIN) && !in_group_p(pid->pid_gid)))
> 
> capable() sets a flag when it makes use of the capability (or at least
> it used to), visible via pacct.  So, unless anything has changed in this
> area, it is best to check capable() last, such that it's only reached
> when it actually makes a difference.  Thus, I'd write:
> 
> 	if (pid->hide_net && !in_group_p(pid->pid_gid) &&
> 	    !capable(CAP_NET_ADMIN))

OK.

> Also, what did you mean by the extra braces?  Just separating the
> setting check from the permissions check for readability?

Initially I wrote some ||, so the braces were needed.  Will remove.


Thanks you for the comments, will fix and repost,

-- 
Vasiliy

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]