Closed Bug 1448014 Opened Last year Closed Last year

TranslateMimeType in AndroidDecoderModule returns pointers derived from a temporary

Categories

(Core :: Audio/Video, enhancement)

All
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main62-])

Attachments

(1 file)

Running the static analysis plugin with an android build turns up a lot of little errors and one that is worth red-flagging:

In file included from /opt/build/froydnj/build-android/dom/media/platforms/Unified_cpp_dom_media_platforms0.cpp:92:
/home/froydnj/src/gecko-dev.git/dom/media/platforms/android/AndroidDecoderModule.cpp:44:40: error: calling `get` on a temporary, potentially allowing use after free of the raw pointer
  return PromiseFlatCString(aMimeType).get();
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~

I *think* the semantics of our string classes make this safe, but it is entirely possible that they do not.  We should fix this regardless.
Group: core-security → media-core-security
I think it's possible that returning `const nsACString&` would be more
efficient, but that opens itself to similar security holes, I think?  So this
is less efficient, but the correctness is easily shown.
Attachment #8980049 - Flags: review?(rjesup)
Attachment #8980049 - Flags: review?(rjesup) → review+
Assignee: nobody → nfroyd
Comment on attachment 8980049 [details] [diff] [review]
avoid needless flattening in AndroidDecoderModule

Not entirely sure what sec-audit means to potential UAF bugs; asking for approval regardless.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Provide an audio file with a bogus mime type and go from there; unsure how are the second part is.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All of them.

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

Backports should be straightforward.

How likely is this patch to cause regressions; how much testing does it need?

If tests pass, we should be OK.
Attachment #8980049 - Flags: sec-approval?
Comment on attachment 8980049 [details] [diff] [review]
avoid needless flattening in AndroidDecoderModule

As a sec-audit rated issue, this doesn't need sec-approval to go in.
Attachment #8980049 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/275b9ead049c
Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
We're about to build the Fennec 61 RC. This is going to have to ride the trains.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.