1603055 - (CVE-2019-17017) BigInt and Object type confusion vulnerability exploitable via XSLTProcessor setParameter method

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[bo13oy]

Attachment: pocs.zip

Hello,

There is a Type confusion between BigInt and Object Vulnerability in Firefox.

Tested Platforms:

1)Ubuntu 64-bit 16.04.3 (Memory 8G)+ firefox release version nightly 71.0 (64-bit)

When using firefox to load the poc.html, it manifests itself in the form of the following crash:

---cut---

[----------------------------------registers-----------------------------------]
RAX: 0x7ffc8dd921f0 --> 0x2ac4b96192040
RBX: 0xac3e674300ed4900
RCX: 0xac3e674300ed4900
RDX: 0x7ffc8dd9208c --> 0x8dd921f000007ffc
RSI: 0x7ffc8dd921f0 --> 0x2ac4b96192040
RDI: 0x2ac4b96192040
RBP: 0x7ffc8dd91fb0 --> 0x7ffc8dd91fd0 --> 0x7ffc8dd91ff0 --> 0x7ffc8dd92040 --> 0x7ffc8dd920a0 --> 0x7ffc8dd920f0 (--> ...)
RSP: 0x7ffc8dd91fb0 --> 0x7ffc8dd91fd0 --> 0x7ffc8dd91ff0 --> 0x7ffc8dd92040 --> 0x7ffc8dd920a0 --> 0x7ffc8dd920f0 (--> ...)
RIP: 0x7fdb2df3367c (<js::GetObjectClass(JSObject const*)+12>: mov rdi,QWORD PTR [rdi])
R8 : 0xac3e674300ed4900
R9 : 0x0
R10: 0x7ffc8dd91c08 --> 0x7ffc8dd92410 --> 0x34a50a6004a0 --> 0x2c4b9615d610 --> 0x7fdb39940c70 --> 0x7fdb37415616 (--> ...)
R11: 0xac3e674300ed4900
R12: 0x8
R13: 0x7fdb296bc098 --> 0xfff9800000000000
R14: 0x7fdb2a05c601 --> 0x2560000000b
R15: 0x1
EFLAGS: 0x10206 (carry PARITY adjust zero sign trap INTERRUPT direction overflow)
[-------------------------------------code-------------------------------------]
0x7fdb2df33671 <js::GetObjectClass(JSObject const*)+1>: mov rbp,rsp
0x7fdb2df33674 <js::GetObjectClass(JSObject const*)+4>: mov QWORD PTR [rbp-0x8],rdi
0x7fdb2df33678 <js::GetObjectClass(JSObject const*)+8>: mov rdi,QWORD PTR [rbp-0x8]
=> 0x7fdb2df3367c <js::GetObjectClass(JSObject const*)+12>: mov rdi,QWORD PTR [rdi]
0x7fdb2df3367f <js::GetObjectClass(JSObject const*)+15>: mov rax,QWORD PTR [rdi]
0x7fdb2df33682 <js::GetObjectClass(JSObject const*)+18>: pop rbp
0x7fdb2df33683 <js::GetObjectClass(JSObject const*)+19>: ret
0x7fdb2df33684: nop WORD PTR cs:[rax+rax1+0x0]
[------------------------------------stack-------------------------------------]
0000| 0x7ffc8dd91fb0 --> 0x7ffc8dd91fd0 --> 0x7ffc8dd91ff0 --> 0x7ffc8dd92040 --> 0x7ffc8dd920a0 --> 0x7ffc8dd920f0 (--> ...)
0008| 0x7ffc8dd91fb8 --> 0x7fdb2df56165 (<js::IsProxy(JSObject const)+21>: mov rdi,rax)
0016| 0x7ffc8dd91fc0 --> 0x10000002a1a6c78
0024| 0x7ffc8dd91fc8 --> 0x2ac4b96192040
0032| 0x7ffc8dd91fd0 --> 0x7ffc8dd91ff0 --> 0x7ffc8dd92040 --> 0x7ffc8dd920a0 --> 0x7ffc8dd920f0 --> 0x7ffc8dd92240 (--> ...)
0040| 0x7ffc8dd91fd8 --> 0x7fdb34b04515 (<JSObject::is<js::ProxyObject>() const+21>: and al,0x1)
0048| 0x7ffc8dd91fe0 --> 0x7ffc8dd92208 --> 0xfffcac4b96192040
0056| 0x7ffc8dd91fe8 --> 0x2ac4b96192040
[------------------------------------------------------------------------------]
Legend: code, data, rodata, value
Stopped reason: SIGSEGV
0x00007fdb2df3367c in js::GetObjectClass (obj=0x2ac4b96192040) at /home/bo13oy/Desktop/firefox_build/mozilla-release/objdir-frontend/dist/include/jsfriendapi.h:613
613 return reinterpret_cast<const shadow::Object*>(obj)->group->clasp;
gdb-peda$

gdb-peda$ bt
#0 0x00007fdb2df3367c in js::GetObjectClass (obj=0x2ac4b96192040) at /home/bo13oy/Desktop/firefox_build/mozilla-release/objdir-frontend/dist/include/jsfriendapi.h:613
#1 0x00007fdb2df56165 in js::IsProxy (obj=0x2ac4b96192040) at /home/bo13oy/Desktop/firefox_build/mozilla-release/objdir-frontend/dist/include/js/Proxy.h:381
#2 0x00007fdb34b04515 in JSObject::is<js::ProxyObject> (this=0x2ac4b96192040) at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/vm/ProxyObject.h:148
#3 0x00007fdb34c7c815 in js::GetBuiltinClass (cx=0x7fdb2a02a000, obj=..., cls=0x7ffc8dd9208c) at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/jsfriendapi.cpp:264
#4 0x00007fdb34c3ef0e in IsGivenTypeObject (cx=0x7fdb2a02a000, obj=..., typeClass=@0x7ffc8dd920d4: js::ESClass::Array, isType=0x7ffc8dd921c7)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/jsapi.cpp:3124
#5 0x00007fdb34c3ee85 in JS_IsArrayObject (cx=0x7fdb2a02a000, obj=..., isArray=0x7ffc8dd921c7) at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/jsapi.cpp:3134
#6 0x00007fdb2f170d70 in XPCArrayHomogenizer::GetTypeForArray (cx=0x7fdb2a02a000, array=..., length=0xc, resultType=0x7ffc8dd923d8, resultID=0x7ffc8dd92490)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/xpconnect/src/XPCVariant.cpp:191
#7 0x00007fdb2f170787 in XPCVariant::InitializeData (this=0x7fdb2a1a6c40, cx=0x7fdb2a02a000)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/xpconnect/src/XPCVariant.cpp:336
#8 0x00007fdb2f16fe93 in XPCVariant::newVariant (cx=0x7fdb2a02a000, aJSVal=...) at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/xpconnect/src/XPCVariant.cpp:105
#9 0x00007fdb2f1946b1 in nsXPConnect::JSToVariant (this=0x7fdb2b91ed00, ctx=0x7fdb2a02a000, value=..., _retval=0x7ffc8dd92638)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/xpconnect/src/nsXPConnect.cpp:867
#10 0x00007fdb3232bad3 in txMozillaXSLTProcessor::SetParameter (this=0x7fdb29148280, aCx=0x7fdb2a02a000, aNamespaceURI=..., aLocalName=..., aValue=..., aRv=...)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1114
#11 0x00007fdb30c7f784 in mozilla::dom::XSLTProcessor_Binding::setParameter (cx=0x7fdb2a02a000, obj=..., self=0x7fdb29148280, args=...)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/objdir-frontend/dom/bindings/XSLTProcessorBinding.cpp:246
#12 0x00007fdb310596e8 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> (
cx=0x7fdb2a02a000, argc=0x3, vp=0x7ffc8dd93830) at /home/bo13oy/Desktop/firefox_build/mozilla-release/dom/bindings/BindingUtils.cpp:3198
#13 0x00007fdb34b04160 in CallJSNative (cx=0x7fdb2a02a000,
native=0x7fdb31059040 <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...) at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/vm/Interpreter.cpp:458
#14 js::InternalCallOrConstruct (cx=0x7fdb2a02a000, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/vm/Interpreter.cpp:550
#15 0x00007fdb34b04e83 in InternalCall (cx=0x7fdb2a02a000, args=..., reason=js::CallReason::Call)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/vm/Interpreter.cpp:619
#16 0x00007fdb34b04cff in js::CallFromStack (cx=0x7fdb2a02a000, args=...) at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/vm/Interpreter.cpp:623
#17 0x00007fdb355002c1 in js::jit::DoCallFallback (cx=0x7fdb2a02a000, frame=0x7ffc8dd938c8, stub=0x7fdb28846590, argc=0x3, vp=0x7ffc8dd93830, res=...)
at /home/bo13oy/Desktop/firefox_build/mozilla-release/js/src/jit/BaselineIC.cpp:2938
#18 0x00003d32d56df798 in ?? ()
#19 0x00007ffc8dd98100 in ?? ()
#20 0x00007ffc8dd937f0 in ?? ()
#21 0xfff9800000000000 in ?? ()
#22 0x00007fdb374024c8 in js::jit::vmFunctions () from /home/bo13oy/Desktop/firefox_build/mozilla-release/objdir-frontend/dist/bin/libxul.so

---cut---

Here's a snippet of the method.
-----source code -------

// static

bool XPCArrayHomogenizer::GetTypeForArray(JSContext* cx, HandleObject array,
uint32_t length,
nsXPTType* resultType,
nsID* resultID) {
Type state = tUnk;
Type type;

RootedValue val(cx);
RootedObject jsobj(cx);
for (uint32_t i = 0; i < length; i++) {
if (!JS_GetElement(cx, array, i, &val)) {
return false;
}

if (val.isInt32()) {
  type = tInt;
} else if (val.isDouble()) {
  type = tDbl;
} else if (val.isBoolean()) {
  type = tBool;
}
else if (val.isUndefined() || val.isSymbol()) {
  state = tVar;
  break;
} else if (val.isNull()) {
  type = tNull;
} else if (val.isString()) {
  type = tStr;
}

/***************** add patch sample code start ********************/

else if(val.isBigInt()) //patch 
{
	type = tBigInt; 
}  

/*****************add patch sample code end ********************/

else {
  MOZ_ASSERT(val.isObject(), "invalid type of jsval!");
  jsobj = &val.toObject();

  bool isArray;
  if (!JS_IsArrayObject(cx, jsobj, &isArray)) {
    return false;
  }

  if (isArray) {
    type = tArr;
  } else if (xpc::JSValue2ID(cx, val)) {
    type = tID;
  } else {
    type = tISup;
  }
}

......
-----source code -------

Thank you,
bo13oy
Qihoo 360 Vulcan Team.

Comment 1

[:Gijs (he/him)]

Unsure if this should go in JS or DOM bindings land, as at least the PoC seems to rely on the any type webidl parameter in XSLTProcessor's setParameter method. I don't know if this would affect basically all the APIs in a query roughly like this...

Jan, can you help confirm whether there's 1 or more issues here and where it belongs?

Comment 2

[Jan de Mooij [:jandem]]

Thanks for reporting. This is a good find.

Code in XPConnect is doing this:

    } else {
      MOZ_ASSERT(val.isObject(), "invalid type of jsval!");
      jsobj = &val.toObject();

It doesn't account for the new BigInt type.

I'll fix this to use ValueType with a switch-statement.

Comment 3

[Jan de Mooij [:jandem]]

We should also audit the code for similar issues elsewhere.

Comment 4

[Jan de Mooij [:jandem]]

We've been shipping BigInt since bug 1527902, Firefox 68.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1603055 - Handle BigInt values in XPCVariant code. r?bholley!

Ideally this code would use JS::ValueType with a switch-statement and rely on
compiler exhaustiveness checking, but this is the safer patch.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)

We should also audit the code for similar issues elsewhere.

I did a search for isSymbol(). Outside js/src we have:

• The two instances being fixed here for BigInt.
• Some js/public code that's correct.
• MaybeWrapValue in DOM bindings, already handles BigInt.
• Codegen.py code related to property ids.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9115167 [details]
Bug 1603055 - Handle BigInt values in XPCVariant code. r?bholley!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Pretty easy. It points to BigInt values not being handled and then you could see where this code is used.
• 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
• If not all supported branches, which bug introduced the flaw?: Bug 1527902
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely.

Comment 8

[Daniel Veditz [:dveditz]]

jandem: what's your holiday work schedule? how late could you land this in December (Soft freeze is Jan 2)?

jcristau: how late is RelMan comfortable having this land? It's a simple "missed case" fix, which should be extremely low risk. But it's also kind of obvious should someone see the patch go by, and Type Confusion has been the main bug class we've seen abused in practice the last several years (multiple Pwn2Own winners and the "Coinbase" 0-day).

I'm totally on board with sec-approval+, just want to have a date nailed down. Would love to have this land between Christmas and New Year's if possible, the closer to the soft freeze the better.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Daniel Veditz [:dveditz] from comment #8)

jandem: what's your holiday work schedule? how late could you land this in December (Soft freeze is Jan 2)?

I'll take PTO but I'll be (near) home most of that time and will likely be checking in on email etc anyway, so I'm happy to land this whenever.

January 1st?

Comment 10

[Julien Cristau [:jcristau]]

We're building the last 72 beta on Dec 27, and release candidate Dec 30. Can this land on the 26th?

Comment 11

[Jan de Mooij [:jandem]]

NI for comment 10.

Comment 12

[Daniel Veditz [:dveditz]]

This could go directly into the RC with minimal risk. Would prefer the 29th but could live with the 26th. That's Boxing Day in the UK and Canada, don't know if that's a common thing elsewhere in Europe. Would there be sheriffs around to do a checkin-needed?

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 9115167 [details]
Bug 1603055 - Handle BigInt values in XPCVariant code. r?bholley!

sec-approval to land as close to the release as jandem and jcristau can negotiate, preferably after Christmas.

Comment 14

[Julien Cristau [:jcristau]]

Sure, I can land on the 30th before building the RC.

Comment 15

[Julien Cristau [:jcristau]]

Please request uplift to beta and esr68.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 9115167 [details]
Bug 1603055 - Handle BigInt values in XPCVariant code. r?bholley!

Beta/Release Uplift Approval Request

• User impact if declined: Security issues.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very low risk because it just adds a check for a missing case.
• String changes made/needed: N/A

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9115167 [details]
Bug 1603055 - Handle BigInt values in XPCVariant code. r?bholley!

OK for uplift to esr68 and beta 72.

Comment 18

[Bobby Holley (:bholley)]

Strictly speaking, I didn't review this. I don't mind (the patch is fine), but it's probably worth updating the attachment metadata when reviewers change for non-Lando patches.

Comment 19

[Andreea Pavel [:apavel]]

Uplifted: https://hg.mozilla.org/releases/mozilla-beta/rev/fca21807338b1ad6e77f05499aa19d037a987e37

Comment 20

[Andreea Pavel [:apavel]]

(In reply to Bobby Holley (:bholley) from comment #18)

Strictly speaking, I didn't review this. I don't mind (the patch is fine), but it's probably worth updating the attachment metadata when reviewers change for non-Lando patches.

Do you need me to back it out and update the reviewer?

Comment 21

[Andreea Pavel [:apavel]]

(In reply to Bobby Holley (:bholley) from comment #18)

Strictly speaking, I didn't review this. I don't mind (the patch is fine), but it's probably worth updating the attachment metadata when reviewers change for non-Lando patches.

Relanded with proper reviewer: https://hg.mozilla.org/releases/mozilla-beta/rev/b02693517c6c7cc9dba606bd22a11c0fdf2ad1e2

Comment 22

[Andreea Pavel [:apavel]]

Esr68 uplift: https://hg.mozilla.org/releases/mozilla-esr68/rev/abb620ef4695c289e6c384d4ddb6400b9613ba4c

Comment 23

[Jan de Mooij [:jandem]]

Still had to land on m-c:

https://hg.mozilla.org/integration/autoland/rev/47790856733b3b23cb42ab1b506b3682a01572d5

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:peterv, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 25

[Andrew McCreight [:mccr8]]

Looks like this landed on m-c a day or so ago:
https://hg.mozilla.org/mozilla-central/rev/47790856733b3b23cb42ab1b506b3682a01572d5

Comment 26

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.