Closed Bug 1387837 Opened 7 years ago Closed 7 years ago

Consider using /etc/ld.so.conf for creating the broker read access policy

Categories

(Core :: Security: Process Sandboxing, defect, P2)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
All
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tedd, Assigned: gcp)

References

Details

(Whiteboard: [sb+])

Attachments

(1 file)

Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy.
59 bytes, text/x-review-board-request
jld
: review+
Sylvestre
: approval-mozilla-beta-
Details 
Instead of hard coding all possible paths for libraries, such as /lib, /lib64, /lib32, etc. We could parse /etc/ld.so.conf to dynamically construct a whitelist, at least when it comes to library locations.

However, I don't know how much we can trust the content of the configuration file, mine looks like this:

> $ cat /etc/ld.so.conf                                 
> # ld.so.conf autogenerated by env-update; make all changes to
> # contents of /etc/env.d directory
> /lib64
> /usr/lib64
> /usr/local/lib64
> /lib32
> /usr/lib32
> /usr/local/lib32
> /lib
> /usr/lib
> /usr/local/lib
> include ld.so.conf.d/*.conf
> /usr/lib64/OpenCL/vendors/intel
> /usr/lib64/qt4

> $ cat /etc/ld.so.conf.d/*.conf
> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/32
> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0

the file itself is owned by root:root (on my system) and only root has write permissions:

> root ~ # ls -la /etc/ld.so.cache
> -rw-r--r-- 1 root root 120307 Aug  6 14:02 /etc/ld.so.cache

Using this approach, we would prevent crashes based on denied file access when the content process tries to load a legitimate library that is not installed in one of the default directories.
While it's super useful to have an exhaustive list, I'm wondering if implementing a config file parser and establishing that whole list during runtime is adding too much complexity to security critical code.
Doing it at compile time is probably not really solving the problem.

I'd lightly suggest surveying these config files on the most popular distros to improve our hard-coded list.
(In reply to Frederik Braun [:freddyb] (PTO Aug 12th - Sep 4th) from comment #1)
> While it's super useful to have an exhaustive list, I'm wondering if
> implementing a config file parser and establishing that whole list during
> runtime is adding too much complexity to security critical code.

I don't think so. Those config files in /etc aren't typically writable by anyone besides root, so there are no real security issues. If the user could write there, you could exploit anyone on the system, no need for Firefox.

I'm not sure how complex the file format is, but if it's just "every line is a path" and "include glob", we can probably deal with that (or just assume it's ld.so.conf.d and bail if that's not what's in the include line). If we can't parse it, we can bail, no harm done in trying.

I like this idea because it potentially fixes the "long tail" of compatibility issues with non-standard libs.
Priority: -- → P2
Whiteboard: [sb?] → [sb+]
As a note, every system I checked has their
/etc/ld.so.conf as

include ld.so.conf.d/*.conf
or
include /etc/ld.so.conf.d/*.conf

and the files in that dir don't use include. So I'll probably go with the simplest possible parser.
Comment on attachment 8916028 [details]
Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy.

https://reviewboard.mozilla.org/r/187298/#review192366

I took a look at the glibc source and found some things that this parser doesn't handle, but I haven't tested them to confirm that glibc does what I think it does, and I have no idea how likely we'd be to encounter them in the wild.  You can decide which of these are worth trying to fix.  Looks good otherwise.

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:124
(Diff revision 1)
> +  bool more = true;
> +  do {
> +    rv = lineStream->ReadLine(line, &more);
> +    // Simplify our following parsing by trimming whitespace
> +    line.CompressWhitespace(true, true);
> +    if (line.IsEmpty() || line.First() == '#') {

There can also be comments at the end of a non-empty line.  (The glibc source notes that there's no quoting or escaping, so it's safe to search for the first `#` and truncate the string there.)

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:137
(Diff revision 1)
> +      line.BeginReading(start);
> +      line.EndReading(end);
> +      token_end = end;
> +
> +      if (FindInReadable(NS_LITERAL_CSTRING("include "), start, token_end)) {
> +          nsAutoCString includeGlob(Substring(token_end, end));

glibc accepts multiple globs on the same `include` line, separated by whitespace.  (But not 0 globs, so there's no need to check for both `"include"` and `"include "`.)

Also, the include globs can apparently be relative to the directory the including file is in, but only if `ldconfig` isn't run with the `-r` flag.  I don't know whether that's something we'd actually run into.

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:148
(Diff revision 1)
> +            }
> +            globfree(&globbuf);
> +          }
> +      }
> +    }
> +    aPolicy->AddDir(rdonly, line.get());

This might not be a path.  glibc seems to also recognize `hwcap` directives, which I think we can safely ignore.  It's probably safe to assume that the path will be absolute and skip anything that doesn't start with `/`.

Also, glibc accepts strings of the form `dirname=TYPE` where `TYPE` basically indicates the libc major version.  (Unlike some of the other things I found, this one is actually documented in the `ldconfig(8)` man page).  It probably isn't used anymore, but it would be easy enough to handle by truncating at the first `=` if present.  

And it looks like glibc will canonicalize the path, so we'd want to `realpath` it.  (I haven't tested that.)

However, unlike with `include`, there can't be multiple paths on one line, so that's one thing this case *doesn't* need to deal with.
Attachment #8916028 - Flags: review?(jld) → review+
Comment on attachment 8916028 [details]
Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy.

https://reviewboard.mozilla.org/r/187298/#review192472

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:110
(Diff revision 1)
> +  nsCOMPtr<nsIFileInputStream> fileStream(
> +    do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv));
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }
> +  rv = fileStream->Init(ldconfig, -1, -1, false);

Almost forgot this while I was staring at the glibc source: that last argument is an `int32_t`, not a `bool`.

There's legacy code like nsOSHelperAppService that's passing `false`, but it stopped being a boolean in 2002 (bug 126829).
Assignee: nobody → gpascutto
I implemented basically all extras (aside from relative globbing), so can you have another look?
Flags: needinfo?(jld)
Comment on attachment 8916028 [details]
Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy.

https://reviewboard.mozilla.org/r/187298/#review193394

Looks good to me.  One nit:

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:129
(Diff revisions 1 - 2)
> +    int32_t hash = line.FindChar('#');
> +    if (hash >= 0) {
> +      line = Substring(line, 0, hash);
> +    }
> +    // Cut off anything behind an = sign, used by dirname=TYPE directives
> +    int32_t equals = line.FindChar('=');

I'd have done this right before the `realpath`, but there probably won't be an `=` in an include directive so it's not likely to matter.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s cb599a162d73 -d 89b6554cef5e: rebasing 425096:cb599a162d73 "Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy. r=jld" (tip)
merging config/system-headers
merging security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
warning: conflicts while merging security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67487a0a224b
Add library paths from /etc/ld.so.conf to broker read access policy. r=jld
https://hg.mozilla.org/mozilla-central/rev/67487a0a224b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(jld)
Comment on attachment 8916028 [details]
Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy.

Approval Request Comment
[Feature/Bug causing the regression]: Defense against potential regressions from Bug 1308400
[User impact if declined]: Obscure configurations might have various functionality problems.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Landed a few days ago
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk.
[Why is the change risky/not risky?]: Adding to a whitelist, on failure we proceed as without the patch.
[String changes made/needed]: N/A
Attachment #8916028 - Flags: approval-mozilla-beta?
Hi Selena, just reading through the comments, this doesn't seem to be a must fix for 57. Could you please confirm whether this one can ride the 58 train? Thanks!
Flags: needinfo?(sdeckelmann)
Jim/GCP and I consulted -- we're ok letting this ride 58. Thanks for checking in.
status-firefox57: affected → wontfix
Flags: needinfo?(sdeckelmann)
It could be argued that this patch reduces the risks of 57 against the "longer tail" of configurations on release. But so far we have seen relatively few problems. If a bunch end up getting triaged against sandboxing around the final betas, and it looks like this would help, we can still reconsider here.
Comment on attachment 8916028 [details]
Bug 1387837 - Add library paths from /etc/ld.so.conf to broker read access policy.

Per comment #17
Attachment #8916028 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: