Closed Bug 1384986 Opened 7 years ago Closed 7 years ago

DConf regressions from filesystem read restrictions

Categories

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

Product:
Core
Component:
Security: Process Sandboxing
Platform:
Unspecified
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: sb+)

Attachments

(2 files)

Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1384986 - Adjust sandbox policy for dconf's `mkdir -p` behavior.
59 bytes, text/x-review-board-request
gcp
: review+
Details 
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.
Assignee: nobody → jld
Priority: -- → P1
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.
Summary: mkdir()/EEXIST regressions from filesystem read restrictions → PulseAudio and DConf regressions from filesystem read restrictions
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.
Attachment #8891357 - Flags: review?(gpascutto) → review+
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.
Attachment #8891358 - Flags: review?(gpascutto) → review+
(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?
(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.
(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.
Something else in the past week seems to have broken PulseAudio on top of how it was already broken, so now I'm bisecting.
(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 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.
Pushed by jedavis@mozilla.com:
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
…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.
Blocks: 1385523
Whiteboard: sb+
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.
Flags: needinfo?(jld)
Attachment #8891357 - Flags: review+ → review?(gpascutto)
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.
Attachment #8891357 - Flags: review?(gpascutto)
The pulseaudio regression is now bug 1388545.
Summary: PulseAudio and DConf regressions from filesystem read restrictions → DConf regressions from filesystem read restrictions
No longer blocks: 1388545
See Also: → 1388545
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.
Attachment #8891357 - Flags: review?(gpascutto) → review+
(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.
Pushed by jedavis@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2f541b1c207d
https://hg.mozilla.org/mozilla-central/rev/7894c44fbcb6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: