Last Comment Bug 806031 - (CVE-2013-0748) [FIX] XBL.__proto__.toString is ugly and reveals address space layout
(CVE-2013-0748)
: [FIX] XBL.__proto__.toString is ugly and reveals address space layout
Status: RESOLVED FIXED
[adv-main18+][adv-esr17+][adv-esr10+]
: sec-high, testcase
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 806034
  Show dependency treegraph
 
Reported: 2012-10-26 18:45 PDT by Jesse Ruderman
Modified: 2013-04-30 18:40 PDT (History)
11 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
18+
fixed
18+
fixed
fixed


Attachments
testcase (320 bytes, text/html)
2012-10-26 18:45 PDT, Jesse Ruderman
no flags Details
minimal patch? (6.85 KB, patch)
2012-12-10 04:46 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
less silly assert (6.83 KB, patch)
2012-12-10 05:41 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
abillings: sec‑approval+
Details | Diff | Review
esr17 (6.77 KB, patch)
2012-12-14 17:54 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
akeybl: approval‑mozilla‑esr17+
Details | Diff | Review
esr10 (6.72 KB, patch)
2012-12-21 04:50 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Jesse Ruderman 2012-10-26 18:45:08 PDT
Created attachment 675776 [details]
testcase

Got:
[object chrome://xbl-marquee/content/xbl-marquee.xml#marquee a646314]

Expected in opt, don't reveal address space info:
[object chrome://xbl-marquee/content/xbl-marquee.xml#marquee]

Expected in debug, format it properly:
[object chrome://xbl-marquee/content/xbl-marquee.xml#marquee @ 0x0a646314]

I can't find the code that's doing this.  But here's an example of code doing it right:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNativeJSOps.cpp#74
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-26 20:22:57 PDT
The relevant code is nsXBLBinding::DoInitJSClass which does:

1372       // We need to create a unique classname based on aClassName and
1373       // parent_proto.  Append a space (an invalid URI character) to ensure that
1374       // we don't have accidental collisions with the case when parent_proto is
1375       // null and aClassName ends in some bizarre numbers (yeah, it's unlikely).
1376       jsid parent_proto_id;
1377       if (!::JS_GetObjectId(cx, parent_proto, &parent_proto_id)) {
1378         // Probably OOM
1379         return NS_ERROR_OUT_OF_MEMORY;
1380       }
1381 
1382       // One space, maybe "0x", at most 16 chars (on a 64-bit system) of long,
1383       // and a null-terminator (which PR_snprintf ensures is there even if the
1384       // string representation of what we're printing does not fit in the buffer
1385       // provided).
1386       char buf[20];
1387       PR_snprintf(buf, sizeof(buf), " %lx", parent_proto_id);
1388       className.Append(buf);

and then uses that as the .name of the JSClass, so it gets picked up by toString.

JS_GetObjectId seems to use the object pointer. :(
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-26 20:24:02 PDT
What we really want is a fast one-way hash of the parent_proto_id... ;)
Comment 3 Jesse Ruderman 2012-10-29 16:44:58 PDT
(In reply to Boris Zbarsky (:bz) from comment #2)
> What we really want is a fast one-way hash of the parent_proto_id... ;)

Does the result of the hash need to be guaranteed unique?
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-29 17:13:31 PDT
I suspect so, yes.  At least on a per-window basis...

But we have a hashtable with those strings as keys (which is why they need to be unique).  We could just do a lookup after hashing and if we get a hit iterate the hash, no?
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-29 18:47:01 PDT
What we did for the XSLT generate-id() function (which also needs to generate per-document unique identifiers) is that we simply output

((uintptr_t)node) - ((uintptr_t)node->GetOwnerDoc->GetWindow())

Would that work here?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-29 18:51:24 PDT
Hmm.  This hashtable is cross-window, but there might be no real hits in it across windows because the protos would be different...

Looking at this more, we might be able to just not store this string the JSClass .name at all, and put it directly on nsXBLJSClass instead.
Comment 7 [On PTO until 6/29] 2012-10-31 10:18:58 PDT
Can someone suggest a security rating for this issue?
Comment 8 Jesse Ruderman 2012-10-31 16:03:07 PDT
Hi Al. dveditz and I have been arguing in about that (e.g. in bug 806034).
Comment 9 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-11-21 10:51:35 PST
Well, I'll mark this sec-high, and somebody can change it if they want.
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-05 08:30:38 PST
IMO sec-high should be assigned to someone. Taking.
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-10 01:33:55 PST
So to keep the old behavior as much as possible, couldn't we store
binding name + 64bit id as class name, and store the current binding name + parent_proto_id
in nsXBLService::gClassTable.

I think also comment 6 hints about that.
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-10 04:46:23 PST
Created attachment 690347 [details] [diff] [review]
minimal patch?

To not reveal address space, reveal only an id.
Boris, what do you think of this conservative approach?
nsXBLBinding::DoInitJSClass could certainly be cleaned up a bit, but
I tried to keep the changes minimal. I explicitly decided to not have any
special classname for DEBUG.

Also, as far as I see PR_snprintf usage is currently wrong on 64bit.
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-10 05:18:11 PST
oh, fun. Static assert fires on 32bit.
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-10 05:39:51 PST
Ah, silly me. that assertion is just wrong.
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-10 05:41:51 PST
Created attachment 690354 [details] [diff] [review]
less silly assert
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-10 05:59:25 PST
Comment on attachment 690354 [details] [diff] [review]
less silly assert

r=me
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-10 06:11:13 PST
Comment on attachment 690354 [details] [diff] [review]
less silly assert

[Security approval request comment]
How easily can the security issue be deduced from the patch? The issue perhaps easily, but it isn't
a security problem without having some other security bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not really. The comment will about having better toString for XBL classes.

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply to all the branches with some minor merging. The code is _old_.

How likely is this patch to cause regressions; how much testing does it need?
I tried to take the most conservative approach - I don't expect regressions.
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-12 07:27:42 PST
https://hg.mozilla.org/mozilla-central/rev/718145f0d606
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-12 07:39:23 PST
Comment on attachment 690354 [details] [diff] [review]
less silly assert

[Approval Request Comment]
Bug caused by (feature/regressing bug #): No bug, but XBL class sharing from year 2000
User impact if declined: See bug 806034
Testing completed (on m-c, etc.): just landed m-c 
Risk to taking this patch (and alternatives if risky): Shouldn't be too risky
String or UUID changes made by this patch: NA
Comment 20 Alex Keybl [:akeybl] 2012-12-12 12:10:41 PST
Comment on attachment 690354 [details] [diff] [review]
less silly assert

This will first land in beta 5, which should be manageable.
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-14 17:54:38 PST
Created attachment 692532 [details] [diff] [review]
esr17


See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-18 17:07:47 PST
Comment on attachment 692532 [details] [diff] [review]
esr17

approving for uplift to go along with ff18 (please land once this is also uplifted to mozilla-beta).
Comment 23 Alex Keybl [:akeybl] 2012-12-19 13:54:43 PST
Comment on attachment 690354 [details] [diff] [review]
less silly assert

Since this didn't make it into beta 5, we'll review the necessity of the fix for this internally reported bug during tomorrow's security triage.
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-12-19 14:22:09 PST
Alex, I'd already landed this on all the branches and mid-aired with you updating the bug. How would you like me to proceed?

https://hg.mozilla.org/releases/mozilla-aurora/rev/e65cdbf38911
https://hg.mozilla.org/releases/mozilla-beta/rev/82e323659c9d
https://hg.mozilla.org/releases/mozilla-esr17/rev/f579c9a6d8f2
Comment 25 Alex Keybl [:akeybl] 2012-12-20 16:43:16 PST
Comment on attachment 690354 [details] [diff] [review]
less silly assert

We'll leave this one in the build, given the fact that it's low risk and is exposed on pre-release branches. Fingers crossed :)
Comment 26 Alex Keybl [:akeybl] 2012-12-20 16:43:51 PST
We still need a patch for ESR10 here. Olli?
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-20 16:45:17 PST
Uh, I can't keep track on my security bugs :/
I'll write esr10 patch tomorrow.
Comment 28 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-21 04:50:44 PST
Created attachment 694801 [details] [diff] [review]
esr10

I was told try may not work too well with esr10 anymore, so crossing fingers.
The only changes to esr17 patch are merging + one nullptr -> nsnull.
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-12-21 16:57:37 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/82e323659c9d
Comment 30 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-22 12:06:57 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/02cd40b93741

Note You need to log in before you can comment on or make changes to this bug.