1377959 - jvm_android.cc passes va_list to varags methods

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Nathan Froyd [:froydnj]]

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

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: fix compiler warning about varargs functions

Fortunately, the fix is pretty simple.

Comment 2

[Daniel Veditz [:dveditz]]

Looks like this came from upstream -- can you file a security bug against them for this, too?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

https://bugs.chromium.org/p/webrtc/issues/detail?id=7965

Comment 4

[Nils Ohlmeier [:drno]]

For those without access to the upstream bug: fix has landed upstream. The bug is closed on their end.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

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

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.

Comment 8

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

This is a pretty small patch, we can take in b13. Julien FYI

Comment 9

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

sec-approval+ for trunk. Please nominate a beta patch as well ASAP.

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This was pushed to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d844f6fdaf3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eec75edfd85a

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d844f6fdaf3