Closed Bug 1337304 (CVE-2017-5397) Opened 4 years ago Closed 4 years ago

Fennec cache directory is world writable

Categories

(Firefox for Android :: General, defect, P1)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
fennec 51+ ---
firefox51 blocking fixed
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 + fixed
firefox54 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

(Keywords: sec-critical, Whiteboard: [adv-main51.0.3+])

Attachments

(1 file)

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: 4 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.