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.