1337304 - (CVE-2017-5397) Fennec cache directory is world writable

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Patch gets rid of those lines, and reverts the permission for existing

installations that already had the permissions set.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

We obviously need to uplift this immediately, and probably consider a dot release.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen, I forget, what verification do we do on the extracted files on startup?

Comment 5

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8834330 [details] [diff] [review]
Patch gets rid of those lines, and reverts the permission for existing

sec-approval+ for trunk.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8620080eef03f82c57ac963aeeddf54a8af8e2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c51d7c9daa24
https://hg.mozilla.org/releases/mozilla-beta/rev/69819951895f
https://hg.mozilla.org/releases/mozilla-release/rev/0a1c7512c881

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f8620080eef

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/69819951895f

Comment 14

[Mike Hommey [:glandium]]

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.