Closed
Bug 1401339
(CVE-2017-7836)
Opened 7 years ago
Closed 7 years ago
The pingsender executable dynamically loads libcurl, using dlopen and hardcoded library list. [Mac/Linux]
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | wontfix |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: e, Assigned: gsvelto)
References
Details
(Keywords: csectype-other, reporter-external, sec-moderate, Whiteboard: [adv-main57+][post-critsmash-triage])
Attachments
(2 files, 1 obsolete file)
3.35 KB,
text/x-csrc
|
Details | |
2.28 KB,
patch
|
Dexter
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36
Steps to reproduce:
Introduction:
The pingsender executable dynamically loads libcurl, using dlopen and hardcoded library list.
According to the manpages, the order of execution for dlopen will be:
When path does not contain a slash character (i.e. it is just a leaf name), dlopen() searches the following until it finds a compatible Mach-O file:
$LD_LIBRARY_PATH, $DYLD_LIBRARY_PATH, current working directory, $DYLD_FALLBACK_LIBRARY_PATH.
Further:
Note: If DYLD_FALLBACK_LIBRARY_PATH is not set, dlopen operates as if DYLD_FALLBACK_LIBRARY_PATH was set to $HOME/lib:/usr/local/lib:/usr/lib.
The vulnerable file is located on: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp
The offending dlopen code dlopen is in line 105
mLib = dlopen(libname, RTLD_NOW);
And in line 84 through 101:
const char* libcurlNames[] = {
"libcurl.so",
"libcurl.so.4",
// Debian gives libcurl a different name when it is built against GnuTLS
"libcurl-gnutls.so.4",
// Older libcurl if we can't find anything better
"libcurl.so.3",
#ifndef HAVE_64BIT_BUILD
// 32-bit versions on 64-bit hosts
"/usr/lib32/libcurl.so",
"/usr/lib32/libcurl.so.4",
"/usr/lib32/libcurl-gnutls.so.4",
"/usr/lib32/libcurl.so.3",
#endif
// macOS
"libcurl.dylib",
"libcurl.4.dylib",
"libcurl.3.dylib"
};
By writing a malicious dylib, in ~/lib, an attacker can attain local execution privileges.
Further - on every subsequent telemetry request, the code will be executed.
Instruction to reproduce:
Save the C file and compile to generate a dyilib:
gcc -c poc.c
gcc -dynamiclib -current_version 1.0 poc.o -o libcurl.dylib
Ensure that Enable Firefox Health Report is enabled On Firefox -> Preferences -> Advanced -> Data Choices
Open and close Firefox.
This is a bad security practice, you can read more about it (although on a Windows world) on:
http://blog.opensecurityresearch.com/2014/01/unsafe-dll-loading-vulnerabilities.html
Actual results:
Code was executed (Calculator was opened)
Expected results:
A list with the known paths should had been provided.
Comment 1•7 years ago
|
||
:gsvelto, can you take a look? Looks like you wrote this.
Component: Untriaged → Telemetry
Flags: needinfo?(gsvelto)
Product: Firefox → Toolkit
Assignee | ||
Comment 2•7 years ago
|
||
Yeah, this is going to be tricky on Linux. On Mac it's easy to hardcode the paths where Apple usually puts libcurl, but on Linux there's a plethora of possible places, some architecture dependent (e.g. on Debian). I'll look into this tomorrow, worst case we'll have a very long list.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gsvelto)
Just checking - this isn't a sandbox escape right? I assume post-level3 (actually level2 even), that the content process would not have the ability to write to ~/lib?
Updated•7 years ago
|
Flags: sec-bounty?
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Paul Theriault [:pauljt] from comment #3)
> Just checking - this isn't a sandbox escape right? I assume post-level3
> (actually level2 even), that the content process would not have the ability
> to write to ~/lib?
I'm not familiar with Firefox sandbox architecture, but from what I read in the documentation:
https://wiki.mozilla.org/Security/Sandbox/Deny_Filesystem_Access
"Some directories are read/write protected, but this will not provide real security until the bulk of the $HOME directory is read/write protected.
On OS X, the Firefox Profile directory is stored within ~/Library/Application Support/Firefox/Profiles/. ~/Library is read/write protected with a few exceptions for some specific subdirectories. Access to $HOME and other areas of the filesystem is not restricted. i.e., the content process can read and write to/from anywhere the OS permits: $HOME and temporary directories. The ~/Library read/write prevention could be bypassed because the rest of the $HOME is read/write accessible. For example, a compromised process could add malicious commands to ~/.login-type files to copy data from ~/Library when a user logs in."
Comment 5•7 years ago
|
||
(In reply to Ezra Caltum from comment #4)
> (In reply to Paul Theriault [:pauljt] from comment #3)
> > Just checking - this isn't a sandbox escape right? I assume post-level3
> > (actually level2 even), that the content process would not have the ability
> > to write to ~/lib?
>
> I'm not familiar with Firefox sandbox architecture, but from what I read in
> the documentation:
>
> https://wiki.mozilla.org/Security/Sandbox/Deny_Filesystem_Access
>
> "Some directories are read/write protected, but this will not provide real
> security until the bulk of the $HOME directory is read/write protected.
> On OS X, the Firefox Profile directory is stored within
> ~/Library/Application Support/Firefox/Profiles/. ~/Library is read/write
> protected with a few exceptions for some specific subdirectories. Access to
> $HOME and other areas of the filesystem is not restricted. i.e., the content
> process can read and write to/from anywhere the OS permits: $HOME and
> temporary directories. The ~/Library read/write prevention could be bypassed
> because the rest of the $HOME is read/write accessible. For example, a
> compromised process could add malicious commands to ~/.login-type files to
> copy data from ~/Library when a user logs in."
Unfortunately that wikipage hasn't been updated since october 2016 ( https://wiki.mozilla.org/index.php?title=Security/Sandbox/Deny_Filesystem_Access&action=history ). Based on https://groups.google.com/forum/#!searchin/mozilla.dev.platform/sandbox|sort:date/mozilla.dev.platform/HJZg_RVZDao/I3I3yYuDAwAJ , I think the situation is better than that, especially for 56 which we're shipping next week, but Paul will have more details.
Comment 6•7 years ago
|
||
"By writing a malicious dylib, in ~/lib, an attacker can attain local execution privileges."
If an attacker has the ability to put a malicious dylib in ~/lib they _already_ have local execution privileges. As Paul was getting at, this is mostly interesting as a potential vector for sandbox escapes, and the wiki quote shows we know our current protection is incomplete.
> This is a bad security practice, you can read more about it (although on a Windows world) on:
> http://blog.opensecurityresearch.com/2014/01/unsafe-dll-loading-vulnerabilities.html
The Windows problem is worse. The "Current" directory is often easily writable depending on how you launch an executable, and the original search path looked there _first_. Even the "Safe" version eventually looked there. Only if you set the search path to "" can you mitigate the worst of the problem. Even then, if the attacker can write to the installation directory or any of the Windows directories they can usually inject into your process--but then again they could also just overwrite your program or one of the legitimate Windows .dlls.
Group: firefox-core-security → toolkit-core-security
Keywords: csectype-other,
sec-moderate
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #6)
> The Windows problem is worse. The "Current" directory is often easily
> writable depending on how you launch an executable, and the original search
> path looked there _first_. Even the "Safe" version eventually looked there.
> Only if you set the search path to "" can you mitigate the worst of the
> problem. Even then, if the attacker can write to the installation directory
> or any of the Windows directories they can usually inject into your
> process--but then again they could also just overwrite your program or one
> of the legitimate Windows .dlls.
Note that on Windows the pingsender doesn't load any DLL dynamically. This is Mac/Linux specific.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
Tentative patch, I still have to test it on Mac and a couple more Linux distros before asking for review.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8911156 -
Attachment is obsolete: true
Attachment #8911359 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 10•7 years ago
|
||
Comment 11•7 years ago
|
||
Comment on attachment 8911359 [details] [diff] [review]
[PATCH v2] Look for libcurl under platform-specific paths
Review of attachment 8911359 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me with the comment below addressed (in case we need to fix it).
Did you check if you can still reproduce with the STR from 0 on at least a machine?
::: toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp
@@ +82,5 @@
> {
> + const char* libcurlPaths[] = {
> + "/usr/lib",
> +#ifdef XP_LINUX
> + "/usr/lib32",
Do we need to check for |HAVE_64BIT_BUILD| here?
Attachment #8911359 -
Flags: review?(alessio.placitelli) → review+
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #6)
> "By writing a malicious dylib, in ~/lib, an attacker can attain local
> execution privileges."
>
> If an attacker has the ability to put a malicious dylib in ~/lib they
> _already_ have local execution privileges. As Paul was getting at, this is
> mostly interesting as a potential vector for sandbox escapes, and the wiki
> quote shows we know our current protection is incomplete.
>
> > This is a bad security practice, you can read more about it (although on a Windows world) on:
> > http://blog.opensecurityresearch.com/2014/01/unsafe-dll-loading-vulnerabilities.html
>
> The Windows problem is worse. The "Current" directory is often easily
> writable depending on how you launch an executable, and the original search
> path looked there _first_. Even the "Safe" version eventually looked there.
> Only if you set the search path to "" can you mitigate the worst of the
> problem. Even then, if the attacker can write to the installation directory
> or any of the Windows directories they can usually inject into your
> process--but then again they could also just overwrite your program or one
> of the legitimate Windows .dlls.
Well, I played a little bit with this, and there are two interesting scenarios in Mac:
a) CWD is the first directory where it's searched. (so potential sandbox escape...)
b) By doing dlopen, you're bypassing Mac gatekeeper (you're not getting the pesky "This binary was downloaded from the internet / was not signed" ) dialog box.
Although I'm happy that it's being taken into consideration (and worked on it quickly... BIG KUDOS!), I'm not sure about the moderate impact classification (due to the gatekeeper bypass by using Firefox signed and trusted binaries....)
Anyways, thanks for your attention. This is my first time disclosing to the Mozilla Foundation and I'm happily surprised by the time to patch.
Reporter | ||
Comment 13•7 years ago
|
||
Actually - I'm not sure about the potential sandbox escape, as I wrote before I'm not familiar with the sandbox, but I'm downloading and building Firefox from source now...
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #11)
> Looks good to me with the comment below addressed (in case we need to fix
> it).
> Did you check if you can still reproduce with the STR from 0 on at least a
> machine?
I've just tested it, double-checked what the pingsender was doing, and I cannot reproduce anymore.
> ::: toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp
> @@ +82,5 @@
> > {
> > + const char* libcurlPaths[] = {
> > + "/usr/lib",
> > +#ifdef XP_LINUX
> > + "/usr/lib32",
>
> Do we need to check for |HAVE_64BIT_BUILD| here?
Not really, dlopen() will pick the right library anyway so I thought it wasn't worth the fuss.
Assignee | ||
Comment 15•7 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 17•7 years ago
|
||
Do we need to uplift this (and bug 1402966) to 57?
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #17)
> Do we need to uplift this (and bug 1402966) to 57?
Yes, we should.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8911359 [details] [diff] [review]
[PATCH v2] Look for libcurl under platform-specific paths
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1310703
[User impact if declined]: Potential security vulnerability, see comment 0
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: I've manually verified it but it hasn't received formal QA
[Needs manual test from QE? If yes, steps to reproduce]: We can use the STR in comment 0, it shouldn't be reproducible anymore with this patch applied.
[List of other uplifts needed for the feature/fix]: Bug 1402966 will also need to be uplifted if this is uplifted, or we'll break tier 3 platforms
[Is the change risky?]: No
[Why is the change risky/not risky?]: the change affects a tool that lives outside of gecko and which was designed so that failures have no impact on Firefox functionality
[String changes made/needed]: None
Attachment #8911359 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Blocks: 1310703
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
Version: 57 Branch → 54 Branch
Comment 20•7 years ago
|
||
Comment on attachment 8911359 [details] [diff] [review]
[PATCH v2] Look for libcurl under platform-specific paths
Fix a sec moderate, taking it.
Attachment #8911359 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 21•7 years ago
|
||
uplift |
Updated•7 years ago
|
Group: toolkit-core-security → core-security-release
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 22•7 years ago
|
||
> Not really, dlopen() will pick the right library anyway so I thought it wasn't worth the fuss.
Well, it won't pick the right one on Linux !x86 !x86-64.
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #22)
> Well, it won't pick the right one on Linux !x86 !x86-64.
Yep, I was aware of that but couldn't find a reliable way to do it except for adding the entire list of architectures Debian supports to the list. I suppose we can add more as we need them. If there is an easy way to guess the right directory at runtime we could make it more robust though but I haven't investigated that either.
Comment 24•7 years ago
|
||
I'm inclined to question the need to hardcode paths on linux at all. There's no stupid fallback like DYLD_FALLBACK_LIBRARY_PATH or the current working directory, and if an attacker can make you run pingsender with an altered LD_LIBRARY_PATH, you're already owned.
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #24)
> I'm inclined to question the need to hardcode paths on linux at all. There's
> no stupid fallback like DYLD_FALLBACK_LIBRARY_PATH or the current working
> directory, and if an attacker can make you run pingsender with an altered
> LD_LIBRARY_PATH, you're already owned.
I tend to agree but I'm no security expert so I'm not sure what are the current best practices as far as dlopen() is concerned.
Updated•7 years ago
|
Whiteboard: [adv-main57+]
Updated•7 years ago
|
Alias: CVE-2017-7836
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main57+] → [adv-main57+][post-critsmash-triage]
Comment 26•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #24)
> I'm inclined to question the need to hardcode paths on linux at all. There's
> no stupid fallback like DYLD_FALLBACK_LIBRARY_PATH or the current working
> directory, and if an attacker can make you run pingsender with an altered
> LD_LIBRARY_PATH, you're already owned.
I tend to agree.
0. If you could get libraries into a malicious directory that dlopen would search, probably you could just shadow something like libc that's DT_NEEDED instead.
1. In particular, it looks like we wouldn't search $ORIGIN in official builds, so the search path should be entirely directories specified in system-level config or in the environment (which, as mentioned above, if that's a problem then you're already owned).
2. The only case I can think of that would cause a sandbox escape as of 57 is if Firefox is run from a subdirectory of /tmp (see also bug 1386404), *and* it's built with $ORIGIN in DT_RUNPATH for some reason. However, in that case the attacker could just replace part of the Firefox install itself, so this change doesn't help.
Assignee | ||
Comment 27•7 years ago
|
||
Shall I open a bug to drop the hardcoded paths from Linux then? I haven't done it yet because I wanted to be 100% I wouldn't be regressing anything but I'm happy to limit this to Mac, it makes things a lot easier.
Comment 28•7 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #27)
> Shall I open a bug to drop the hardcoded paths from Linux then? I haven't
> done it yet because I wanted to be 100% I wouldn't be regressing anything
> but I'm happy to limit this to Mac, it makes things a lot easier.
Yes, but I'd suggest having glandium double-check my reasoning in comment #26 and make sure this is safe; he knows the dynamic linker better than I do.
Comment 29•7 years ago
|
||
Comment 26 sounds about right.
Assignee | ||
Comment 30•7 years ago
|
||
Alright, I'll open a new bug to drop the hard-coded paths from Linux.
Updated•6 years ago
|
Group: core-security-release
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•