1384986 - DConf regressions from filesystem read restrictions

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The filesystem read restrictions from bug 1308400 caused some regressions of what bug 1335329 fixed.  The mechanism to return EEXIST for trying to mkdir() an existing directory/file still works, so the gtests still pass, but it requires that the broker policy allow testing for the file's existence (e.g., with lstat()).  And the new policy doesn't give MAY_ACCESS to some of the paths in question.

In general it seems a little weird that we can allow access to files inside a directory but won't admit that the directory itself exists; there's an argument to be made that any ancestor of any path in the policy should get MAY_ACCESS.

But we might be able to fix this by adding a few special cases.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Widening this bug title a little, because we have another problem: when gUM starts its PulseAudio instance, it tries to connect to the X server to read the PA server info from a root window property, and we're not allowing access to the Xauthority file.  It works anyway in stock Ubuntu, and maybe other mainstream desktop environments, because there's an xhost(1) entry for SI:localuser:$(whoami), so any local-domain connection from a process owned by the user is allowed.

If that's not that case, then even if you're using module-x11-publish, the client can't see it; this means it falls back on the case that we fixed in bug 1335329 and just regressed.

Note that a determined attacker can send requests on the existing X11 connection — see bug 1129492 — so allowing access to $XAUTHORITY isn't much of a decrease in security.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

Review commit: https://reviewboard.mozilla.org/r/162316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162316/

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.

Review commit: https://reviewboard.mozilla.org/r/162318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162318/

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

https://reviewboard.mozilla.org/r/162316/#review168358

::: security/sandbox/linux/broker/SandboxBroker.h:100
(Diff revision 1)
> +    // Adds MAY_ACCESS for all ancestors of a given path.  Useful for
> +    // libraries that try to do the equivalent of `mkdir -p`.  This
> +    // does not include the root directory, and it includes the path
> +    // itself only if it has a trailing slash.
> +    void AddAncestors(const char* aPath);
> +    void AddAncestors(nsCString&& aPath);

Might as well inline it? And use nsDependentCString instead of AutoCString?

::: security/sandbox/linux/broker/SandboxBroker.cpp:300
(Diff revision 1)
> +    if (lastSlash <= 0) {
> +      MOZ_ASSERT(lastSlash == 0);
> +      break;
> +    }
> +    aPath.Truncate(lastSlash);
> +    AddPath(0, aPath.get());

No magic numbers, use Perms::MAY_ACCESS please.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8891358 [details]
Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.

https://reviewboard.mozilla.org/r/162318/#review168360

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:123
(Diff revision 1)
>    policy->AddDir(rdonly, "/sys/devices/cpu");
>    policy->AddDir(rdonly, "/sys/devices/system/cpu");
>  
> +#ifdef MOZ_PULSEAUDIO
> +  // See bug 1384986 comment #1.
> +  if (const auto xauth = PR_GetEnv("XAUTHORITY")) {

We need to file a bug to remove this again, linked to some metabug to get rid of the X11 connection.

I obviously care more about this line than the preceding ones.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #1)
> when gUM starts its PulseAudio instance, it tries to connect to the X server to read
> the PA server info from a root window property, and we're not allowing
> access to the Xauthority file.

Obvious question: isn't the PA info available anywhere else? I guess there's no point in bothering much until we get more X11 lockdown?

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> > +  if (const auto xauth = PR_GetEnv("XAUTHORITY")) {
> 
> We need to file a bug to remove this again, linked to some metabug to get
> rid of the X11 connection.
> 
> I obviously care more about this line than the preceding ones.

This will happen long before we can get rid of the X11 connection.  I've filed bug 1386019, because we don't seem to have a bug for general post-PulseAudio cleanup.

> Obvious question: isn't the PA info available anywhere else? I guess there's no point in bothering much until we get more X11 lockdown?

Not as far as I know.  Maybe DBus, which is also bad.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> Obvious question: isn't the PA info available anywhere else? I guess there's
> no point in bothering much until we get more X11 lockdown?

On second thought, this needs a longer answer: There's a default location, which is used if the X11 property isn't found (unless the open/mkdir thing fails, which we fixed in bug 1335329 are re-fixing in this bug), but if the server socket isn't in the default location, as far as I know the client will depend on X.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Something else in the past week seems to have broken PulseAudio on top of how it was already broken, so now I'm bisecting.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #9)
> Something else in the past week seems to have broken PulseAudio on top of
> how it was already broken, so now I'm bisecting.

Bug 1007283, which has been backed out.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162316/diff/1-2/

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891358 [details]
Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162318/diff/1-2/

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

https://reviewboard.mozilla.org/r/162316/#review168460

::: security/sandbox/linux/broker/SandboxBroker.h:100
(Diff revision 1)
> +    // Adds MAY_ACCESS for all ancestors of a given path.  Useful for
> +    // libraries that try to do the equivalent of `mkdir -p`.  This
> +    // does not include the root directory, and it includes the path
> +    // itself only if it has a trailing slash.
> +    void AddAncestors(const char* aPath);
> +    void AddAncestors(nsCString&& aPath);

This side-effects the string to cut off the path components, so it can't use `nsDependentCString`.  Since we have move references, why copy the buffer if it's not going to be used for anything else?

And it's not inlined (or, rather, I factored it out because) I suspect we'll need it for other things in the future.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60408af056d9
Fix DConf breakage caused by read restrictions. r=gcp
https://hg.mozilla.org/integration/autoland/rev/23dae62b5ece
Fix PulseAudio breakage caused by read restrictions. r=gcp

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=120156099&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/a3ced6b85bda58605e9270ff8ecf2b9cff2fc12d

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

…oh.  The rules with MAY_ACCESS for $XDG_RUNTIME_DIR/dconf and all of its non-root ancestors take precedence over the rule with MAY_READ|RECURSIVE for /.  So if $XDG_RUNTIME_DIR is under $HOME, then no read access to $HOME (but subdirectories of $HOME, if they aren't also ancestors of $XDG_RUNTIME_DIR, are fine).

Historical note: Once upon a time, adding rules would always increase permissions, because we didn't have any of the dynamic recursive/prefix stuff we do now.  Each rule was a single path, exactly as given by the client, with no server-side parsing or anything, because that was all we needed on B2G.  Different rules did not interact with each other like they do now, so checking for an existing rule with the exact same path was sufficient.  This is, incidentally, why the broker used a single flat hash table of paths, instead of something more like a virtual filesystem that would make directory structure explicit: given the functional requirements at the time, it was simpler, and for code enforcing a security boundary, simpler is generally better.

But I think I can work around this particular problem without making fundamental changes to the broker.

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162316/diff/2-3/

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891358 [details]
Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162318/diff/2-3/

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

MozReview, sigh.  GKTBPtAea5J was r+'ed; this is 5HVMTWw5W8T and is not.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

https://reviewboard.mozilla.org/r/162316/#review171066

::: commit-message-03798:6
(Diff revision 3)
> +Bug 1384986 - Allow MAY_ACCESS on ancestors of any allowed path for Linux sandboxing. r?gcp
> +
> +This is handled at policy lookup time instead of by adding additional
> +policy entries, because otherwise (for example) a MAY_ACCESS entry for
> +/home/jld, added as an ancestor of /home/jld/something/dconf, would hide
> +the MAY_ACCESS|MAY_READ|RECURSIVE entry for / and remove read permission.

I think I'd prefer to handle this at creation time. The problem you mention would be solved by accumulating permissions as we strip away path elements? That may be done either at policy creation or lookup, but preferably the former.

That is, if we (add|look up) /home/jld/something/dconf, we check that, and then |= the permissions for /home/jld/something, /home/jld, /home, /

This seems like it would cause less unexpected surprises if the policy gets more complicated.

If this is done at creation, this operation should run when we are done with creating the policy, to avoid order dependence. At the end of it, you'd be able to print a precise map of the actual permissions.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The pulseaudio regression is now bug 1388545.

Comment 23

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162316/diff/3-4/

Comment 24

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891358 [details]
Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162318/diff/3-4/

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

https://reviewboard.mozilla.org/r/162316/#review174502

::: security/sandbox/linux/broker/SandboxBroker.h:75
(Diff revision 4)
> +    // made redundant by this process.
> +    //
> +    // Call this after adding rules and before using the policy to
> +    // prevent the descendent rules from shadowing the ancestor rules
> +    // and removing permissions that we expect the file to have.
> +    void FixRecursion();

very minor nit: The name feels a bit weird. FixupRecursivePermissions?

::: security/sandbox/linux/broker/SandboxBroker.cpp:332
(Diff revision 4)
> +    if ((newPerms & ~RECURSIVE) == inheritedPerms) {
> +      if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
> +        SANDBOX_LOG_ERROR("removing redundant %s: %d -> %d", PromiseFlatCString(path).get(),
> +                          localPerms, newPerms);
> +      }
> +      continue;

I'd add a comment like "Skip adding this to the new map" or something similar, since you have to look below to see that this is what the continue actually achieves.

::: security/sandbox/linux/broker/SandboxBroker.cpp:341
(Diff revision 4)
> +        SANDBOX_LOG_ERROR("recursion fix for %s: %d -> %d", PromiseFlatCString(path).get(),
> +                          localPerms, newPerms);
> +      }
> +    } else {
> +      if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
> +        SANDBOX_LOG_ERROR("unchanged for %s: %d", PromiseFlatCString(path).get(), newPerms);

In terms of logging, instead of showing what didn't change, is it not more useful to show the resulting complete mapping at the end of the function?

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:257
(Diff revision 4)
>    // This requires accessing user preferences so we can only do it now.
>    // Our constructor is initialized before user preferences are read in.
>    if (GetEffectiveContentSandboxLevel() <= 2 || aFileProcess) {
>      policy->AddDir(rdonly, "/");
> -    return policy;
> +    // Any other read-only rules will be removed as redundant by
> +    // Policy::FixRecursion, so there's no need to early-return here.

With this fix, I'd group the two AddDynamicPathList calls together again.

Comment 26

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #25)
> ::: security/sandbox/linux/broker/SandboxBroker.cpp:341
> (Diff revision 4)
> > +        SANDBOX_LOG_ERROR("recursion fix for %s: %d -> %d", PromiseFlatCString(path).get(),
> > +                          localPerms, newPerms);
> > +      }
> > +    } else {
> > +      if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
> > +        SANDBOX_LOG_ERROR("unchanged for %s: %d", PromiseFlatCString(path).get(), newPerms);
> 
> In terms of logging, instead of showing what didn't change, is it not more
> useful to show the resulting complete mapping at the end of the function?

The "recursion fix" and "unchanged" lines, together, show the new policy.  But I can simplify this by using the same message for everything that's not removed, which should make the contents of new policy more easily readable.

Comment 27

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891357 [details]
Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162316/diff/4-5/

Comment 28

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8891358 [details]
Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162318/diff/4-5/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f541b1c207d
Prevent sandbox file broker rules from removing rights granted by more general rules. r=gcp
https://hg.mozilla.org/integration/autoland/rev/7894c44fbcb6
Adjust sandbox policy for dconf's `mkdir -p` behavior. r=gcp

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f541b1c207d
https://hg.mozilla.org/mozilla-central/rev/7894c44fbcb6