jvm_android.cc passes va_list to varags methods

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

({csectype-other, sec-high})

Trunk
mozilla56
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [adv-main55-][post-critsmash-triage])

Attachments

(1 attachment)

jvm_android.cc contains:

jobject JavaClass::CallStaticObjectMethod(jmethodID methodID, ...) {
  va_list args;
  va_start(args, methodID);
  jobject res = jni_->CallStaticObjectMethod(j_class_, methodID, args);
  CHECK_EXCEPTION(jni_) << "Error during CallStaticObjectMethod";
  return res;
}

jint JavaClass::CallStaticIntMethod(jmethodID methodID, ...) {
  va_list args;
  va_start(args, methodID);
  jint res = jni_->CallStaticIntMethod(j_class_, methodID, args);
  CHECK_EXCEPTION(jni_) << "Error during CallStaticIntMethod";
  return res;
}

Unfortunately, those CallStatic{Object,Int}Method calls don't take a va_list; they take varargs.  GCC doesn't catch the error, but clang does, and issues a warning.

Filing this as a security bug for safety; it's not immediately obvious to me if you could actually exploit this, but varargs mismatches and JNI can't be a good combination...
Fortunately, the fix is pretty simple.
Attachment #8883092 - Flags: review?(rjesup)
Looks like this came from upstream -- can you file a security bug against them for this, too?
Group: core-security → media-core-security
Flags: needinfo?(rjesup)
Blocks: 1163171
Attachment #8883092 - Flags: review?(rjesup) → review+
Flags: needinfo?(rjesup)
For those without access to the upstream bug: fix has landed upstream. The bug is closed on their end.
froyd: let's get s-a and land, since it's landed upstream, and uplift.  No indication from their end as to the severity, so going with sec-high.
Flags: needinfo?(nfroyd)
Comment on attachment 8883092 [details] [diff] [review]
fix compiler warning about varargs functions

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

Not sure.  Carefully crafted WebRTC exploits for Android only seem not particularly easy, though.

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?

Beta, Release.  ESR52 is not affected.

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

Bug 1250356, landed in Firefox 53.

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

This patch should apply cleanly.  Backports are trivial to make in any event.

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

If we haven't noticed problems before this patch, we are unlikely to notice problems after.
Flags: needinfo?(nfroyd)
Attachment #8883092 - Flags: sec-approval?
I need Ritu and Release Management to say whether they feel that we can take this with one beta left since I'd want it on Beta as well as Trunk. Given that it is a two line change, I think we can probably take it.
Flags: needinfo?(rkothari)
Rank: 15
Priority: -- → P1
This is a pretty small patch, we can take in b13. Julien FYI
Flags: needinfo?(rkothari) → needinfo?(jcristau)
sec-approval+ for trunk. Please nominate a beta patch as well ASAP.
Attachment #8883092 - Flags: sec-approval? → sec-approval+
Comment on attachment 8883092 [details] [diff] [review]
fix compiler warning about varargs functions

Approval Request Comment
[Feature/Bug causing the regression]: bug 1250356
[User impact if declined]: More security vulnerabilities are a bad thing.
[Is this code covered by automated tests?]: Unsure, but given that things would probably go haywire if this code was ever hit, I suspect not.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Pretty obvious fix once the compiler warned about it.  Upstream has also seen fit to fix the problem in the exact same way.
[String changes made/needed]: None.
Attachment #8883092 - Flags: approval-mozilla-beta?
Attachment #8883092 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(jcristau)
https://hg.mozilla.org/mozilla-central/rev/1d844f6fdaf3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [adv-main55-]
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [adv-main55-] → [adv-main55-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.