Bug 1337304 (CVE-2017-5397)

Fennec cache directory is world writable

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

({sec-critical})

unspecified
Firefox 54
All
Android
Points:
---

Firefox Tracking Flags

(fennec51+, firefox51blocking fixed, firefox52+ fixed, firefox-esr52 fixed, firefox53+ fixed, firefox54 fixed)

Details

(Whiteboard: [adv-main51.0.3+])

Attachments

(1 attachment)

This is bad. We set the Fennec cache directory to be world writable here [1]. Now that we default to extracting libraries to the cache directory, it means any app can change our extracted libraries and inject arbitrary code.

[1] https://dxr.mozilla.org/mozilla-central/rev/af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/GeckoLoader.java#219
installations that already had the permissions set.
Attachment #8834330 - Flags: review?(snorp)
Priority: -- → P1
Comment on attachment 8834330 [details] [diff] [review]
Patch gets rid of those lines, and reverts the permission for existing

Review of attachment 8834330 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. Nice catch.

::: mozglue/linker/Mappable.cpp
@@ +149,5 @@
>          "not extracting");
>      return nullptr;
>    }
> +
> +  // Ensure that the cache dir is private.

Maybe mention this bug there, otherwise it's kind of a puzzling change. The cache directory is private by default.
Attachment #8834330 - Flags: review?(snorp) → review+
We obviously need to uplift this immediately, and probably consider a dot release.
Eugen, I forget, what verification do we do on the extracted files on startup?
Flags: needinfo?(esawin)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Eugen, I forget, what verification do we do on the extracted files on
> startup?

None, we only check the CRC as provided by the APK against the cached CRC.
Flags: needinfo?(esawin)
Assignee: nobody → nchen
Comment on attachment 8834330 [details] [diff] [review]
Patch gets rid of those lines, and reverts the permission for existing

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It would be fairly easy to write an inconspicuous Android app that exploits this, but users would have to be tricked into installing that app first.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes it's fairly obvious what the problem is.

Which older supported branches are affected by this flaw? All branches up to release

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Very easy and not risky.

How likely is this patch to cause regressions; how much testing does it need? Tested locally and unlikely to cause regressions.
Attachment #8834330 - Flags: sec-approval?
Comment on attachment 8834330 [details] [diff] [review]
Patch gets rid of those lines, and reverts the permission for existing

sec-approval+ for trunk.
Attachment #8834330 - Flags: sec-approval? → sec-approval+
Hi Jim, Wes, can we get this patch landed on all branches asap? I'd like to gtb 51.0.3 (Fennec only dot release) in the next hour or so.
Flags: needinfo?(wkocher)
Flags: needinfo?(nchen)
Comment on attachment 8834330 [details] [diff] [review]
Patch gets rid of those lines, and reverts the permission for existing

Approving for uplift to Aurora53, Beta52, Release51
Attachment #8834330 - Flags: approval-mozilla-release+
Attachment #8834330 - Flags: approval-mozilla-beta+
Attachment #8834330 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/4f8620080eef
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Alias: CVE-2017-5397
Group: firefox-core-security → core-security-release
Whiteboard: [adv-main51.0.3+]
Comment on attachment 8834330 [details] [diff] [review]
Patch gets rid of those lines, and reverts the permission for existing

Review of attachment 8834330 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +150,5 @@
>      return nullptr;
>    }
> +
> +  // Ensure that the cache dir is private.
> +  chmod(cachePath, 0770);

This was the wrong level to be doing this. And this still leaves the directory group-writable. Which is fine-ish on Android, but I like to keep this code not making any Android-only assumptions.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.