[gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

All,

please take a look at the following issue.

https://github.com/openrc/openrc/issues/195

The first part of the fix is committed to master as shown on the issue;
checkpath should *never* follow symbolic links when changing ownership,
so I have moved to the lchown call instead of chown.

However, I'm not sure how to deal with the hard link issue in a way that
will not break service scripts.

If anyone has any suggestions for this, let me know.

Thanks,

William

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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/09/2018 07:07 PM, William Hubbs wrote
> 
> However, I'm not sure how to deal with the hard link issue in a way that
> will not break service scripts.
> 

Systemd mitigates this by enabling the fs.protected_hardlinks sysctl by
default, but they have the liberty of requiring a relatively new Linux
kernel. The larger problem there is that, unless you have some kernel
protection built-in, the "Z" type in the tmpfiles.d spec is always going
to be exploitable:

  * https://github.com/OpenRC/opentmpfiles/issues/3

  * https://github.com/systemd/systemd/issues/7736

(I didn't realize at the time that the OpenRC fix still contained a race
condition.)

Ultimately, it's not safe to chown/chmod/setfacl/whatever in a directory
that is not writable only by yourself and root. There's some precedent
for this in e.g.

 https://wiki.gentoo.org/wiki/Hardened/Grsecurity_Trusted_Path_Execution

where they refuse to *execute* something that is writable by others. But
a solution like that would break existing scripts.

If it's any consolation, the tools like chown, chgrp, chmod, setfacl,
etc. are all vulnerable to the same issue themselves. GNU chown has the
"--from" flag (which still contains a race, by the way), but the other
tools don't, and are all exploitable in the same way. Again the advice
seems to be "don't do things if somebody can write to the directory
you're in."

Here's a very tedious proposal for OpenRC:

  1. Create a new helper, called e.g. "newpath", that is like checkpath
     but only creates things, and doesn't modify them.

  2. Have newpath throw a warning if it's used in a directory that is
     writable by someone other than root and the OpenRC user. This will
     prevent people from creating /foo/bar after /foo has already been
     created with owner "foo:foo". In other words, service script
     writers will be encouraged to do things in a safe order. Since
     we're starting over, this might even be made an error.

  3. Deprecate checkpath

  4. Wait a million years for people to switch from checkpath to newpath

  5. Get rid of checkpath

I'm not even sure that this solves the problem completely, but it's the
only idea I've got left.

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

On Tue, Jan 09, 2018 at 08:19:24PM -0500, Michael Orlitzky wrote:

*snip*

> Ultimately, it's not safe to chown/chmod/setfacl/whatever in a directory
> that is not writable only by yourself and root.

Let me try to phrase this another way.

If the directory we are in is not owned by us or root and is group or
world writable, checkpath should not change the ownership or permissions
of the file passed to it.

> Here's a very tedious proposal for OpenRC:
> 
>   1. Create a new helper, called e.g. "newpath", that is like checkpath
>      but only creates things, and doesn't modify them.
> 
>   2. Have newpath throw a warning if it's used in a directory that is
>      writable by someone other than root and the OpenRC user. This will
>      prevent people from creating /foo/bar after /foo has already been
>      created with owner "foo:foo". In other words, service script
>      writers will be encouraged to do things in a safe order. Since
>      we're starting over, this might even be made an error.
> 
>   3. Deprecate checkpath
> 
>   4. Wait a million years for people to switch from checkpath to newpath
> 
>   5. Get rid of checkpath
> 
> I'm not even sure that this solves the problem completely, but it's the
> only idea I've got left.

I'm not really a fan of creating a new helper unless I have to; I would
rather modify checkpath's behaviour.

The first stage of that modification would be to release a version that
outputs error messages, then convert the error messages to hard failures
in a later release.

Is this reasonable? If we go this route, what should checkpath start
complaining about?

William


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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Kristian Fiskerstrand]

[-- Attachment #1.1: Type: text/plain, Size: 761 bytes --]

On 01/10/2018 02:19 AM, Michael Orlitzky wrote:
> On 01/09/2018 07:07 PM, William Hubbs wrote
>>
>> However, I'm not sure how to deal with the hard link issue in a way that
>> will not break service scripts.
>>
> 
> Systemd mitigates this by enabling the fs.protected_hardlinks sysctl by
> default, but they have the liberty of requiring a relatively new Linux

so does gentoo-sources since discussion in
https://bugs.gentoo.org/540006#c19
> 
> (I didn't realize at the time that the OpenRC fix still contained a race
> condition.)

This was mentioned already in https://bugs.gentoo.org/540006#c15
-- 
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/10/2018 01:04 PM, William Hubbs wrote:
> On Tue, Jan 09, 2018 at 08:19:24PM -0500, Michael Orlitzky wrote:
> 
>> Ultimately, it's not safe to chown/chmod/setfacl/whatever in a directory
>> that is not writable only by yourself and root.
> 
> Let me try to phrase this another way.
> 
> If the directory we are in is not owned by us or root and is group or
> world writable, checkpath should not change the ownership or permissions
> of the file passed to it.

There are also POSIX ACLs, NFSv4 ACLs, and god-knows-what-else to worry
about, but the above is a good start.


>> Here's a very tedious proposal for OpenRC: ...
>>
>>   2. Have newpath throw a warning if it's used in a directory that is
>>      writable by someone other than root and the OpenRC user. This will
>>      prevent people from creating /foo/bar after /foo has already been
>>      created with owner "foo:foo". In other words, service script
>>      writers will be encouraged to do things in a safe order. Since
>>      we're starting over, this might even be made an error.
> 
> I'm not really a fan of creating a new helper unless I have to; I would
> rather modify checkpath's behaviour.
> 
> The first stage of that modification would be to release a version that
> outputs error messages, then convert the error messages to hard failures
> in a later release.
> 
> Is this reasonable? If we go this route, what should checkpath start
> complaining about?

/*
 Disclaimer: I'm not even sure that this difficult proposal will solve
 the problem. Moreover there may be legitimate things going on in some
 init scripts that I haven't accounted for.
*/

The downside to keeping the name "checkpath" is that it makes it
difficult to identify unfixed scripts. If we change the name, then "grep
-rl checkpath" points them out for you; but if checkpath is modified,
you have to install the package and attempt to start/stop/save/reload it
and look for warnings.

Aside from that, it sounds workable. I guess it should warn when either,

  1 checkpath is used to modify an existing path and actually changes it

  2 checkpath is used to create a path whose parent is not writable only
    by root and the OpenRC user (i.e. the heuristic you mentioned above)

My rationale for those is as follows:

  1 Modifying existing paths in an automated fashion will never be safe
    due to the hardlink issue. If start_pre() creates both /foo
    and /foo/bar with owner "foo", then you can do that safely the first
    time around (when they're created), but the second time involves
    calling chown/chmod inside /foo which is writable by the "foo" user.

    If the service is restarted, /foo/bar might still exist, at which
    point we have to figure out what to do with it (assuming we don't
    blindly fix its permissions). I guess checkpath should check the
    existing permissions/ownership, and compare them to the desired
    ones? It's not an error if everything is the same, but if the init
    script wants something other than what's there, we should refuse to
    change things.

  2 Suppose again that we're creating both /foo and /foo/bar with owner
    "foo". If we run,

      checkpath --owner foo --directory /foo
      checkpath --owner foo --file      /foo/bar

    then there's still an opportunity for the "foo" user to trick you in
    the short window where /foo exists but /foo/bar does not. Instead,
    we'd like to nudge the service script writer to do it some other
    way. Keeping in mind that  we won't be able to modify the owner of
    /foo after /foo/bar is created (from item #1), this is the best that
    I can come up with:

      checkpath --owner foo --directory /foo
      su foo --command 'checkpath --file /foo/bar'

    That's safe because the "foo" user can only trick himself.

    The behavior of "su" isn't POSIX, but it's standard enough. If the
    construction above is common, maybe it makes sense to have checkpath
    drop privileges? So instead of,

      su foo --command 'checkpath --file /foo/bar'

    you could do,

      checkpath --as foo --file /foo/bar

    That would work because /foo is owned by "foo", and therefore
    writable by only the OpenRC user (now "foo") and root.


This is all quite half-baked, so if anyone thinks that I've overlooked
something obvious, you're probably not wrong.

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

On Wed, Jan 10, 2018 at 03:25:55PM -0500, Michael Orlitzky wrote:
> On 01/10/2018 01:04 PM, William Hubbs wrote:
> > On Tue, Jan 09, 2018 at 08:19:24PM -0500, Michael Orlitzky wrote:
> > 
> >> Ultimately, it's not safe to chown/chmod/setfacl/whatever in a directory
> >> that is not writable only by yourself and root.
> > 
> > Let me try to phrase this another way.
> > 
> > If the directory we are in is not owned by us or root and is group or
> > world writable, checkpath should not change the ownership or permissions
> > of the file passed to it.
> 
> There are also POSIX ACLs, NFSv4 ACLs, and god-knows-what-else to worry
> about, but the above is a good start.
> 
> 
> >> Here's a very tedious proposal for OpenRC: ...
> >>
> >>   2. Have newpath throw a warning if it's used in a directory that is
> >>      writable by someone other than root and the OpenRC user. This will
> >>      prevent people from creating /foo/bar after /foo has already been
> >>      created with owner "foo:foo". In other words, service script
> >>      writers will be encouraged to do things in a safe order. Since
> >>      we're starting over, this might even be made an error.
> > 
> > I'm not really a fan of creating a new helper unless I have to; I would
> > rather modify checkpath's behaviour.
> > 
> > The first stage of that modification would be to release a version that
> > outputs error messages, then convert the error messages to hard failures
> > in a later release.
> > 
> > Is this reasonable? If we go this route, what should checkpath start
> > complaining about?
> 
> /*
>  Disclaimer: I'm not even sure that this difficult proposal will solve
>  the problem. Moreover there may be legitimate things going on in some
>  init scripts that I haven't accounted for.
> */
> 
> The downside to keeping the name "checkpath" is that it makes it
> difficult to identify unfixed scripts. If we change the name, then "grep
> -rl checkpath" points them out for you; but if checkpath is modified,
> you have to install the package and attempt to start/stop/save/reload it
> and look for warnings.

Good point, this may be a good reason to make a new helper and deprecate
checkpath. What I would do is make checkpath throw an error but keep
running,. It would have a message, something like:

"Checkpath is deprecated, please use newpath instead."

What are we saying newpath should do differently than checkpath if I
go this route?

William

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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[James Le Cuirot]

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

On Tue, 9 Jan 2018 18:07:41 -0600
William Hubbs <williamh@gentoo.org> wrote:

> All,
> 
> please take a look at the following issue.
> 
> https://github.com/openrc/openrc/issues/195
> 
> The first part of the fix is committed to master as shown on the issue;
> checkpath should *never* follow symbolic links when changing ownership,
> so I have moved to the lchown call instead of chown.
> 
> However, I'm not sure how to deal with the hard link issue in a way that
> will not break service scripts.
> 
> If anyone has any suggestions for this, let me know.

I faced the hard link problem in another package (bug still restricted)
recently. I'm about to push the fix out but I just want check what I've
done is okay. The init script used to call chown/chmod -R, which is
obviously bad. I've compromised by only calling these on the
directories themselves (ignoring symlinks). I believe this is safe
because it's not possible to create hard linked directories these days?
Would you agree?

-- 
James Le Cuirot (chewi)
Gentoo Linux Developer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/10/2018 05:18 PM, James Le Cuirot wrote:
> 
> The init script used to call chown/chmod -R, which is
> obviously bad. I've compromised by only calling these on the
> directories themselves (ignoring symlinks). I believe this is safe
> because it's not possible to create hard linked directories these days?
> Would you agree?

Are you still using chown and chmod? If so, you should switch to
checkpath -- chown and chmod don't even try to avoid hard links. I would
be surprised to see a "chown" or "chmod" in an init script that can't be
replaced by something better.

The race condition that we're talking about here is trying to squeeze
the last 1% of security out of checkpath; it's already much safer than
chown/chmod.

For example, if your script is calling chown and chmod on two
directories /foo and /foo/bar, then whoever owns /foo can kill /foo/bar
entirely and replace it with a hard link to /etc/passwd. When the
service restarts, chown and chmod won't care that you think /foo/bar
should be a directory instead.

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/10/2018 01:17 PM, Kristian Fiskerstrand wrote:
>>
>> (I didn't realize at the time that the OpenRC fix still contained a race
>> condition.)
> 
> This was mentioned already in https://bugs.gentoo.org/540006#c15
> 

So it is =)

I took the RESOLVED FIXED for granted when I reported the tmpfiles.d
issues. At least for symlinks, there was an easy way to avoid the race,
so something useful came out of my unwillingness to read.

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/10/2018 04:54 PM, William Hubbs wrote:
> 
> What are we saying newpath should do differently than checkpath if I
> go this route?

I think this covers everything that we've talked about:

 1. It should refuse to modify existing paths.

    1.a. If newpath is called on an existing path, and if the requested
         owner/permissions agree with the existing set, then do nothing.
         This is expected when services restart without a reboot.

    1.b. If newpath is called on an existing path, and if the desired
         permissions differ from the existing set, then do nothing and
         log a warning.


 2. It should have a flag (say, --as=<user>[:group]) to make it run as
    an unprivileged user. Basically a portable "su -c".


 3. It should die if it's used in a directory that is writable by
    anyone other than itself or root. (If it's feasible, we might want
    to check the parent directories all the way up to the root; if I can
    write to "b", then I can write to "e" in /a/b/c/d/e.)

    Since newpath can't modify existing paths, the aforementioned "--as"
    flag will be needed to avoid this error.


And just to put it out there, this will probably make a lot of people
mad. It discourages you from doing things like setting FOO_USER=foo in
the conf.d file, because you can't "fix" the permissions on things like
/var/log/foo.log if the value of $FOO_USER ever changes. That was
inherently unsafe anyway, but I'll eat my shorts if nobody complains.

(User variables, or RC_SVCNAME, should still work fine work multiple
instances.)

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

On Sat, Jan 13, 2018 at 03:48:10PM -0500, Michael Orlitzky wrote:
> On 01/10/2018 04:54 PM, William Hubbs wrote:
> > 
> > What are we saying newpath should do differently than checkpath if I
> > go this route?
> 
> I think this covers everything that we've talked about:
> 
>  1. It should refuse to modify existing paths.
> 
>     1.a. If newpath is called on an existing path, and if the requested
>          owner/permissions agree with the existing set, then do nothing.
>          This is expected when services restart without a reboot.
> 
>     1.b. If newpath is called on an existing path, and if the desired
>          permissions differ from the existing set, then do nothing and
>          log a warning.
 
 For both A and B above I think you mean owner/group/permissions right?

>  2. It should have a flag (say, --as=<user>[:group]) to make it run as
>     an unprivileged user. Basically a portable "su -c".

I'm not following why I need this.

>  3. It should die if it's used in a directory that is writable by
>     anyone other than itself or root. (If it's feasible, we might want
>     to check the parent directories all the way up to the root; if I can
>     write to "b", then I can write to "e" in /a/b/c/d/e.)
 
 Checkpath doesn't handle multiple layers of directories currently; you
 can't do "checkpath -d /run/a/b" without doing "checkpath -d /run/a"
 first, so I don't see a way to check parents.

>     Since newpath can't modify existing paths, the aforementioned "--as"
>     flag will be needed to avoid this error.

Which error are you referring to? I don't follow you here. I don't see
how newpath not modifying existing paths is related to this.

William


> And just to put it out there, this will probably make a lot of people
> mad. It discourages you from doing things like setting FOO_USER=foo in
> the conf.d file, because you can't "fix" the permissions on things like
> /var/log/foo.log if the value of $FOO_USER ever changes. That was
> inherently unsafe anyway, but I'll eat my shorts if nobody complains.
> 
> (User variables, or RC_SVCNAME, should still work fine work multiple
> instances.)
> 

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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/17/2018 10:21 AM, William Hubbs wrote:
>  
>  For both A and B above I think you mean owner/group/permissions right?

Yep.


>>  2. It should have a flag (say, --as=<user>[:group]) to make it run as
>>     an unprivileged user. Basically a portable "su -c".
> 
> I'm not following why I need this.
> 
>>  3. It should die if it's used in a directory that is writable by
>>     anyone other than itself or root. (If it's feasible, we might want
>>     to check the parent directories all the way up to the root; if I can
>>     write to "b", then I can write to "e" in /a/b/c/d/e.)
> 
>>     Since newpath can't modify existing paths, the aforementioned "--as"
>>     flag will be needed to avoid this error.
> 
> Which error are you referring to? I don't follow you here. I don't see
> how newpath not modifying existing paths is related to this.
> 


If I want to create /run/foo and /run/foo/bar, both owned by the "foo"
user, how would I do it using newpath?

1. I could create /run/foo with owner "foo", and then create
   /run/foo/bar with owner "foo". That can be done without modifying
   existing permissions, but it's not safe, because you wind up working
   as root in the directory /run/foo which is owned by the non-root
   "foo" user. If newpath disallows that unsafe operation, this approach
   is out.

2. I could create /run/foo as root:root, and then create /run/foo/bar as
   "foo". That much is safe, but then what do I do about /run/foo? It
   already exists, so if newpath will refuse to modify existing paths,
   then this approach is out too.

That leaves...

3. I can create /run/foo with owner "foo", and then setuid to the foo
   user. Now, *as the foo user* I can create /run/foo/bar, which will be
   owned by "foo". There's no risk in doing so, because the "foo" user
   can only trick himself. Moreover, the directory is writable only by
   root and the OpenRC user (currently: foo) at that point, so the extra
   safety precautions don't get in the way.

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

On Wed, Jan 17, 2018 at 10:41:21AM -0500, Michael Orlitzky wrote:
> If I want to create /run/foo and /run/foo/bar, both owned by the "foo"
> user, how would I do it using newpath?
> 
> 1. I could create /run/foo with owner "foo", and then create
>    /run/foo/bar with owner "foo". That can be done without modifying
>    existing permissions, but it's not safe, because you wind up working
>    as root in the directory /run/foo which is owned by the non-root
>    "foo" user. If newpath disallows that unsafe operation, this approach
>    is out.
> 
> 2. I could create /run/foo as root:root, and then create /run/foo/bar as
>    "foo". That much is safe, but then what do I do about /run/foo? It
>    already exists, so if newpath will refuse to modify existing paths,
>    then this approach is out too.
> 
> That leaves...
> 
> 3. I can create /run/foo with owner "foo", and then setuid to the foo
>    user. Now, *as the foo user* I can create /run/foo/bar, which will be
>    owned by "foo". There's no risk in doing so, because the "foo" user
>    can only trick himself. Moreover, the directory is writable only by
>    root and the OpenRC user (currently: foo) at that point, so the extra
>    safety precautions don't get in the way.

Everything I'm saying here assumes that /run/foo and /run/foo/bar do not
exist. If they do, both approaches 1 and 3 will do nothing other than
warn if the permissions or ownership has changed.

In both approaches 1 and 3, the first step will be to create the
directory /run/foo then optionally adjust permissions on it. At that
point newpath will exit.

When the second invocation of newpath starts, we know /run/foo/bar
does not exist, and creating /run/foo/bar will fail if /run/foo doesn't
exist.

Since that's true, I don't see what the difference is
between approaches 1 and 3 or what makes approach 1 so unsafe. Call me
dense if you must, lol, I'm just not getting it. At this point we know
that /run/foo is owned by foo, and I've never heard that root working in
a directory it doesn't own isn't safe.

William

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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/17/2018 12:14 PM, William Hubbs wrote:
>>
>> 1. I could create /run/foo with owner "foo", and then create
>>    /run/foo/bar with owner "foo". That can be done without modifying
>>    existing permissions, but it's not safe, because you wind up working
>>    as root in the directory /run/foo which is owned by the non-root
>>    "foo" user. If newpath disallows that unsafe operation, this approach
>>    is out.
>>
>> ...
>>
>> 3. I can create /run/foo with owner "foo", and then setuid to the foo
>>    user. Now, *as the foo user* I can create /run/foo/bar, which will be
>>    owned by "foo". There's no risk in doing so, because the "foo" user
>>    can only trick himself. Moreover, the directory is writable only by
>>    root and the OpenRC user (currently: foo) at that point, so the extra
>>    safety precautions don't get in the way.
> 
> 
> In both approaches 1 and 3, the first step will be to create the
> directory /run/foo then optionally adjust permissions on it. At that
> point newpath will exit.
> 
> When the second invocation of newpath starts, we know /run/foo/bar
> does not exist, and creating /run/foo/bar will fail if /run/foo doesn't
> exist.
> 
> Since that's true, I don't see what the difference is
> between approaches 1 and 3 or what makes approach 1 so unsafe. Call me
> dense if you must, lol, I'm just not getting it. At this point we know
> that /run/foo is owned by foo, and I've never heard that root working in
> a directory it doesn't own isn't safe.

Not at all. I'm working this out as I go, so better to speak up if
something looks fishy.

There are a few risks that I see with the first approach...


Risk #1: From what I can tell, the current implementation of checkpath
first creates the path, and then adjusts the ownership and permissions
on it. After /run/foo/bar has been created but before the permissions
and ownership are changed, the "foo" user can replace "bar" with a hard
link (because he owns /run/foo). The lchown/chmod will operate on the
target of that link, and if he's fast enough, then the "foo" user can
use that to take over root's files. (Limited to /run in this example,
but that's beside the point.)

Maybe that can be avoided. Is there a portable way to atomically create
a file/directory with its ownership and permissions already set? Or is
it possible to operate on the descriptor that you get back when creating
the path?


Risk #2: Instead consider a four-component path /run/foo/bar/baz. If you
start creating those directories with owner "foo", then when you get to
creating "baz", it's possible that "bar" has been replaced by a symlink
somewhere else.

Certain tricks exist to avoid that, but I'm not an expert on them and I
don't know how portable or secure they really are. For example, you
might get the descriptor of "bar" and then use openat(dirfd,...) instead
of open(...) to create "baz".


If both of those can be solved portably, then all you have to worry
about is everything I haven't thought of =)

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

On Thu, Jan 18, 2018 at 07:19:59PM -0500, Michael Orlitzky wrote:
> Not at all. I'm working this out as I go, so better to speak up if
> something looks fishy.
> 
> There are a few risks that I see with the first approach...
> 
> 
> Risk #1: From what I can tell, the current implementation of checkpath
> first creates the path, and then adjusts the ownership and permissions
> on it. After /run/foo/bar has been created but before the permissions
> and ownership are changed, the "foo" user can replace "bar" with a hard
> link (because he owns /run/foo). The lchown/chmod will operate on the
> target of that link, and if he's fast enough, then the "foo" user can
> use that to take over root's files. (Limited to /run in this example,
> but that's beside the point.)
> 
> Maybe that can be avoided. Is there a portable way to atomically create
> a file/directory with its ownership and permissions already set? Or is
> it possible to operate on the descriptor that you get back when creating
> the path?

Permissions are set as part of the mkdir/open/mkfifo call, so they are
set immediately when the file is created. but ownership is adjusted
separately by calling chown/fchown/lchown.

At a high level, checkpath looks like this:

1. Creating and setting permissions.
a. If the file doesn't exist, set the permissions when it is created.
b. If the file does exist, only fix the permissions if necessary.
c. At this point, the file's permissions are correct.
2. setting ownership
a. always set the ownership if the file's ownership doesn't match the
specified ownership.

It looks like we can't use your --as suggestion if we want to be
able to create paths in /var/lib and /var/spool that are owned by
non-privileged users because of the permissions on those paths. It is
possible that service scripts are doing this.

Is it worth changing the algorithm to do this instead:

0. test for existance by opening a read-only file descriptor to this
file.
1. Creating the file/directory/fifo.
a. If it doesn't exist, create it -- note that I'm not setting
permissions with the create call.
b. Open a read-only file descriptor that attaches to the newly created
file.
2. Setting Permissions.
a. Fix the permissions of the file if necessary.
3. setting ownership
a. Set the ownership if it doesn't match the specified ownership.

> Risk #2: Instead consider a four-component path /run/foo/bar/baz. If you
> start creating those directories with owner "foo", then when you get to
> creating "baz", it's possible that "bar" has been replaced by a symlink
> somewhere else.
 
 It is possible, sure, but the question I would ask is, could this also
 be a legit situation where a user would want /run/foo/bar to be a
 symlink to some other location? If it is, there's no way to tell the
 difference.

> Certain tricks exist to avoid that, but I'm not an expert on them and I
> don't know how portable or secure they really are. For example, you
> might get the descriptor of "bar" and then use openat(dirfd,...) instead
> of open(...) to create "baz".
> 
> If both of those can be solved portably, then all you have to worry
> about is everything I haven't thought of =)

Thoughts?

William


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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/19/2018 07:16 PM, William Hubbs wrote:
> 
> It looks like we can't use your --as suggestion if we want to be
> able to create paths in /var/lib and /var/spool that are owned by
> non-privileged users because of the permissions on those paths. It is
> possible that service scripts are doing this.
> 

Why not? Since /var/lib is root:root and mode 755, we can create
/var/lib/foo while running --as=root (the default). Then afterwards,
anything beneath /var/lib/foo would need to be created "--as" the owner
of that directory.

/var/lib or /var/spool should be no different than /run in that regard.

(Although, the ebuild should be responsible for /var/lib and /var/spool)


> Is it worth changing the algorithm to do this instead:
> 
> 0. test for existance by opening a read-only file descriptor to this
> file.
> 1. Creating the file/directory/fifo.
> a. If it doesn't exist, create it -- note that I'm not setting
> permissions with the create call.
> b. Open a read-only file descriptor that attaches to the newly created
> file.
> 2. Setting Permissions.
> a. Fix the permissions of the file if necessary.
> 3. setting ownership
> a. Set the ownership if it doesn't match the specified ownership.

Is this for checkpath? Steps (a) and (b) would need to happen at the
same time. Is there a way to determine if a file descriptor is for a
hard link? There are likely some small ways that we could still improve
checkpath, but the main issue I'm trying to solve by jumping through all
these hoops is the hard link race condition.


>> Risk #2: Instead consider a four-component path /run/foo/bar/baz. If you
>> start creating those directories with owner "foo", then when you get to
>> creating "baz", it's possible that "bar" has been replaced by a symlink
>> somewhere else.
>  
>  It is possible, sure, but the question I would ask is, could this also
>  be a legit situation where a user would want /run/foo/bar to be a
>  symlink to some other location? If it is, there's no way to tell the
>  difference.

The init script author can use the real path instead of the one
involving the symlink if he needs to. So maybe he wants /run/foo/bar to
be a symlink to /herp/derp, but then instead of doing

  newpath /run/foo/bar/baz

he could do

  newpath /herp/derp/baz

and then there are no symlinks involved.

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[William Hubbs]

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

On Fri, Jan 19, 2018 at 07:53:06PM -0500, Michael Orlitzky wrote:
> On 01/19/2018 07:16 PM, William Hubbs wrote:
> > 
> > It looks like we can't use your --as suggestion if we want to be
> > able to create paths in /var/lib and /var/spool that are owned by
> > non-privileged users because of the permissions on those paths. It is
> > possible that service scripts are doing this.
> > 
> 
> Why not? Since /var/lib is root:root and mode 755, we can create
> /var/lib/foo while running --as=root (the default). Then afterwards,
> anything beneath /var/lib/foo would need to be created "--as" the owner
> of that directory.

That would create an extra level of indirection for some things though,
what if /var/lib/foo needs to be owned by foo? I have /var/lib/dhcp
which is owned by dhcp:dhcp. You can't creat that with --as=dhcp.

> 
> /var/lib or /var/spool should be no different than /run in that regard.
> 
> (Although, the ebuild should be responsible for /var/lib and /var/spool)
> 
> 
> > Is it worth changing the algorithm to do this instead:
> > 
> > 0. test for existance by opening a read-only file descriptor to this
> > file.
> > 1. Creating the file/directory/fifo.
> > a. If it doesn't exist, create it -- note that I'm not setting
> > permissions with the create call.
> > b. Open a read-only file descriptor that attaches to the newly created
> > file.
> > 2. Setting Permissions.
> > a. Fix the permissions of the file if necessary.
> > 3. setting ownership
> > a. Set the ownership if it doesn't match the specified ownership.
> 
> Is this for checkpath? Steps (a) and (b) would need to happen at the
> same time. Is there a way to determine if a file descriptor is for a
> hard link? There are likely some small ways that we could still improve
> checkpath, but the main issue I'm trying to solve by jumping through all
> these hoops is the hard link race condition.

You mean steps 1 (a) and (b)? They would happen in the sequence shown.
The only call that gives you a file descriptor is open() which is not
used to create a directory or a fifo.

The link status is available via fstat() or stat().
An example of checking it is in line 146 of checkpath.c. We refuse to
chmod a file that has more than one hard link to it.

> 
> 
> >> Risk #2: Instead consider a four-component path /run/foo/bar/baz. If you
> >> start creating those directories with owner "foo", then when you get to
> >> creating "baz", it's possible that "bar" has been replaced by a symlink
> >> somewhere else.
> >  
> >  It is possible, sure, but the question I would ask is, could this also
> >  be a legit situation where a user would want /run/foo/bar to be a
> >  symlink to some other location? If it is, there's no way to tell the
> >  difference.
> 
> The init script author can use the real path instead of the one
> involving the symlink if he needs to. So maybe he wants /run/foo/bar to
> be a symlink to /herp/derp, but then instead of doing
> 
>   newpath /run/foo/bar/baz
> 
> he could do
> 
>   newpath /herp/derp/baz
> 
> and then there are no symlinks involved.

Let me think about this... :-)

William

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

Re: [gentoo-dev] rfc: ideas for fixing OpenRC checkpath issue

[Michael Orlitzky]

On 01/19/2018 08:14 PM, William Hubbs wrote:
>>
>> Why not? Since /var/lib is root:root and mode 755, we can create
>> /var/lib/foo while running --as=root (the default). Then afterwards,
>> anything beneath /var/lib/foo would need to be created "--as" the owner
>> of that directory.
> 
> That would create an extra level of indirection for some things though,
> what if /var/lib/foo needs to be owned by foo? I have /var/lib/dhcp
> which is owned by dhcp:dhcp. You can't creat that with --as=dhcp.
> 

The same way you do it now:

  newpath --directory /var/lib/dhcp --owner dhcp:dhcp

There's no new obstacle, because /var/lib is writable only by root and
the current OpenRC user (also root, in this case).

Now if you need /var/lib/dhcp/something-else to be owned by dhcp:dhcp,
*then* you would do it --as=dhcp.