Bug 1246061 (CVE-2016-2808)

null-byte written out of bounds using .watch() due to generation count overflow

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dveditz, Assigned: Waldo)

Tracking

({csectype-bounds, csectype-intoverflow, sec-high})

unspecified
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
qe-verify -

Firefox Tracking Flags

(firefox45 wontfix, firefox46+ fixed, firefox47+ fixed, firefox48+ fixed, firefox-esr3846+ fixed, firefox-esr4546+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main46+][adv-esr45.1+][adv-esr38.8+] CESG REF: 54224850 / VULNERABILITY ID: 426732)

Attachments

(12 attachments, 2 obsolete attachments)

389.66 KB, application/pdf
Details
10.58 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.93 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
10.81 KB, patch
luke
: review+
Details | Diff | Splinter Review
11.89 KB, patch
jandem
: review+
Details | Diff | Splinter Review
9.27 KB, patch
jandem
: review+
Details | Diff | Splinter Review
9.99 KB, patch
jandem
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
36.88 KB, patch
abillings
: sec-approval+
Details | Diff | Splinter Review
23.92 KB, patch
Details | Diff | Splinter Review
23.56 KB, patch
Details | Diff | Splinter Review
23.56 KB, patch
Details | Diff | Splinter Review
6.31 KB, patch
luke
: review+
jandem
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
The CESG (https://www.cesg.gov.uk/) sent a vulnerability report to the security alias concerning a bug in the JS engine regarding Object.watch()

CESG REF: 54224850 / VULNERABILITY ID: 426732

Details in the attached PDF

   "There is a way to trick SpiderMonkey into using an invalid hash entry
   by overflowing an unsigned 32-bit integer. This makes it possible to use
   this vulnerability to write a null byte at a relative offset using an
   invalid pointer. This is achieved using Watchers. [...]"

Their PoC runs in a worker and increments the generation count until it wraps around to the same value.
Assignee

Comment 1

3 years ago
Aargh.  If this is the check I think it was, I always thought generation-comparison looked a little smarmy, and that wraparound appeared possible if you were willing to wait long enough.  I semi-assumed we were not caring on the basis of it indeed taking too long to run: "Triggering this null write via the following proof of concept takes about twenty five minutes."  :-(  But maybe 25mins isn't that long if you (say) waited to spring the attack til late at night.  I should have said something here at some point.

I suspect we can just make the generation field (and various other generation fields in other places) uint64_t to fix this.  Should be fun to track down all the various places generation numbers are treated as uint32_t, to avoid a stray truncation making a potential fix ineffective.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Assignee

Comment 2

3 years ago
Sigh, I've been punting this bug too long.  :-(  Here's a probable patch.  I *think* I got all the cases where a generation is manually typed, but I could have missed some still.

Semi-unrelatedly except in concept:

js::ExpandoAndGeneration::generation, used for DOM proxy invalidation, is also a uint32_t similarly vulnerable to wraparound.  It's 32-bit wraparound, not 24-bit wraparound as for HashTable::gen.  That gives 256x for wraparound to occur, taking delay from 24mins to ~6000mins if we assume equally fast steps between increments.  But we *don't*, in that case, have to run all in a single time slice, possibly, so we might still have a problem.  We should probably decide the answer to that at the same time we deal with this issue, as it's something someone auditing our code after this patch lands would find, and might try to attack.
Assignee

Comment 3

3 years ago
bz, comments on the end of comment 2?  I'm inclined to just bump it to 64-bit and be done with worrying, to be honest -- even if wraparound could never happen because of JIT code being discarded by GC, because of event loop turnings meaning *something* makes the problem go away, blah blah blah.  I see no reason to try to be cute here.
Flags: needinfo?(bzbarsky)
Assignee

Comment 4

3 years ago
Looks like my previous patch fixed everything.  This patch atop it makes HashTable::{gen,shift} into a class with mutators/accessors, and it makes HashTable::generator() and all callers deal with an opaque Generator class that *can't* implicitly convert to anything else, and things compile.  Woo!

We could land this (or something similar) if we wanted, but generation() both internally and externally (to the JS engine) is only rarely assigned to anything, so it's probably overkill.  Let's run with just the first patch.
Assignee

Comment 5

3 years ago
Comment on attachment 8723294 [details] [diff] [review]
Possible patch

This increases the opt-build size of js::HashTable by four bytes on 32-bit systems.  On 64-bit systems, it increases the size of js::HashTable by four bytes as well, but it does so from 20 bytes of members to 24 -- so practically we were halfway into a word anyway.  I see no way to avoid either size increase.  (In any case hash table contents size should swamp the hash table's own size, if the user is doing it right.)

Note that generation, as the subsequent attachment demonstrates, is functionally opaque.  We could make it *actually* opaque, but it didn't seem quite worth it.  Feel free to say something if you disagree.
Attachment #8723294 - Flags: review?(luke)
Comment on attachment 8723294 [details] [diff] [review]
Possible patch

Review of attachment 8723294 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, although I wonder if it'd be a bit safer to have generation return a struct or class wrapping the uint64 instead.  That way you couldn't have accidental silent truncation of uint64 to uint32.
Attachment #8723294 - Flags: review?(luke) → review+
> bz, comments on the end of comment 2?  I'm inclined to just bump it to 64-bit and be done with worrying

I agree.  Fwiw, I did talk it over with efaust, and I'm 99% sure wraparound would not be a security issue, just a web-facing behavior correctness one.  But still, no reason to have to worry about it.  Bumping that counter to 64-bit seems fine.
Flags: needinfo?(bzbarsky)
Assignee

Comment 8

3 years ago
(In reply to Luke Wagner [:luke] from comment #6)
> Makes sense, although I wonder if it'd be a bit safer to have generation
> return a struct or class wrapping the uint64 instead.  That way you couldn't
> have accidental silent truncation of uint64 to uint32.

See comment 4 and comment 5?  I would probably do this without even thinking about it (and make js::Generation into Opaque<uint64_t> for extra niceness), but HashTable::gen is part of a bit field, so it can't be directly made into anything else.

So: we both sort of agree this would be nice.  Do we nonetheless want to do it?  The extra patch here is a bit fugly certainly, so I'm on the fence here.  Do you want to make your suggestion more emphatically or just leave things as good enough?  (Particularly as we're not going to be outgrowing uint64_t:56 basically ever.)

No rush on an answer, I want to pull together a patch for comment 7 before landing anything anyway.
Flags: needinfo?(luke)
Oops, sorry for missing that already.  Could the member variable stay a uint64_t bitfield and only use the Opaque<uint64_t> for the return type of generation()?
Flags: needinfo?(luke)
Assignee

Comment 10

3 years ago
That could be done.  I'd still be a little leery about the bit field users, but maybe they're few enough to live with.

It's perhaps worth noting generation is publicly used 1) by watchpoint code, and 2) by UbiNode code only for assertions.  The rest is internal hash table gunk.  So if we can kill off watches, we can remove a bunch of this nonsense from public misuse entirely.
The bitfield is private (well not in HashTable, but by encapsulation of HashTable inside HashMap/Set) so I think this would still be a big improvement.
But yeah, if we want to totally remove generation from the public interface, that sounds even better!
Assignee

Comment 13

3 years ago
To be used in the next patch.  (Or read previous comments where I allude to the addition of this class.)
Attachment #8725803 - Flags: review?(nfroyd)
Assignee

Comment 14

3 years ago
Okay, yeah -- HashTable::gen is only incremented, never really subjected to implicit conversions otherwise -- so adding opacity at the HashTable boundary is totally good enough, you're right.
Attachment #8725805 - Flags: review?(luke)
Assignee

Updated

3 years ago
Attachment #8723354 - Attachment is obsolete: true
Assignee

Comment 15

3 years ago
The generation() removals in the second patch there are because, if we want to get rid of this, it seems best to keep it out of APIs where it's not used to discourage creeping badness, basically.
Comment on attachment 8725803 [details] [diff] [review]
Implement mozilla::Opaque<T>, a comparable class that otherwise-opaquely wraps a T

Review of attachment 8725803 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with documentation nits addressed.

::: mfbt/Opaque.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * An opaque type supporting only comparison operations that wraps the specified
> + * integral type.

Maybe just "An opaque integral type supporting only comparison operators."?

A block comment describing reason(s) you might want to use Opaque<T> instead of T would be splendid.
Attachment #8725803 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #16)
> A block comment describing reason(s) you might want to use Opaque<T> instead
> of T would be splendid.

And of course I should have ended this with "...as the case for it being here is...opaque."
Comment on attachment 8725805 [details] [diff] [review]
Strongly-type consumers of HashTable::gen, using mozilla::Opaque<uint64_t>

Review of attachment 8725805 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8725805 - Flags: review?(luke) → review+
Assignee

Comment 19

3 years ago
jandem, expanding js::ExpandAndGeneration::generation (currently uint32_t) to uint64_t, as mooted in the end of comment 2 and discussed in comment 3 and comment 7, semi-runs up against MacroAssembler deficiencies in its two uses.

The first use:

        ExpandoAndGeneration* expandoAndGeneration = (ExpandoAndGeneration*)expandoVal.toPrivate();
        masm.movePtr(ImmPtr(expandoAndGeneration), tempVal.scratchReg());

        masm.branch32(Assembler::NotEqual,
                      Address(tempVal.scratchReg(),
                              ExpandoAndGeneration::offsetOfGeneration()),
                      Imm32(expandoAndGeneration->generation),
                      &failDOMProxyCheck);

This is addressable (...heh) by adding branch64.  Except at least x86-64 doesn't have cmp r64,imm64, so I'm not sure that would be the "right" MacroAssembler addition to make.

The second use:

        masm.loadPtr(*expandoAndGenerationAddr, tempVal.scratchReg());
        masm.branchPrivatePtr(Assembler::NotEqual, expandoAddr, tempVal.scratchReg(),
                              &failDOMProxyCheck);

        masm.load32(*generationAddr, scratch);
        masm.branch32(Assembler::NotEqual,
                      Address(tempVal.scratchReg(), offsetof(ExpandoAndGeneration, generation)),
                      scratch, &failDOMProxyCheck);

This could be solved by adding load64 and (again) branch64.  On 64-bit this is perfectly optimal.  But on 32-bit, you then have to take an extra register, really for no purpose because you could compare the low bits (and maybe branch), then compare the high bits (and maybe branch).

So, I'm not sure what would be the best MacroAssembler interface additions to address these needs.  Please advise!
Flags: needinfo?(jdemooij)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> This is addressable (...heh) by adding branch64.  Except at least x86-64
> doesn't have cmp r64,imm64, so I'm not sure that would be the "right"
> MacroAssembler addition to make.

For this one, we could add:

  branch64(Condition, const Address&, Imm64, Label*);

On 32-bit platforms, this would end up doing two (branch32) comparisons. On 64-bit platforms, it can forward to branchPtr or something.

> This could be solved by adding load64 and (again) branch64.  On 64-bit this
> is perfectly optimal.  But on 32-bit, you then have to take an extra
> register, really for no purpose because you could compare the low bits (and
> maybe branch), then compare the high bits (and maybe branch).

Hm. We could have:

  branch64(Condition, const Address&, const Address&, Register scratch, Label*);

On 64-bit we'd just load from address1 into scratch and branch. On 32-bit we could do this twice for the low bits and high bits, as you described. Not very pretty, but it gets the job done without requiring extra scratch registers. It should probably assert the scratch register != the base register of one of the addresses.
Flags: needinfo?(jdemooij)
Assignee

Comment 23

3 years ago
Attachment #8728313 - Flags: review?(jdemooij)
Attachment #8728313 - Flags: review?(bzbarsky)
Assignee

Comment 24

3 years ago
The patch making ExpandoAndGeneration::generation a uint64_t is tested *only* to the extent that in a 64-bit build, I ran dom/bindings/test Mochitests and they passed.  I'd appreciate further edumacation as to how to verificate this patch.  :-)
We may or may not have decent test coverage for it.  Worth checking whether any tests got added in the bug that added the ExpandoAndGeneration thing.  If they were, you're probably OK.  If they were not, we can try to come up with some...
Comment on attachment 8728313 [details] [diff] [review]
Make ExpandoAndGeneration::generation a uint64_t

Please leave offsetOfGeneration()'s name as it was, now that you've found all the callers.

r=me
Attachment #8728313 - Flags: review?(bzbarsky) → review+
Attachment #8728311 - Flags: review?(jdemooij) → review+
Comment on attachment 8728312 [details] [diff] [review]
Implement MacroAssembler::branch64(Condition cond, const Address& lhs, const Address& rhs, Register scratch, Label* label)

Review of attachment 8728312 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler.h
@@ +816,5 @@
>  
>      inline void branch64(Condition cond, const Address& lhs, Imm64 val, Label* label) PER_ARCH;
>  
> +    // Compare the value at lhs with the value at rhs.  The scratch register
> +    // *may* be the base of lhs.

This comment doesn't match the |lhs.base != scratch| asserts. But right, we should be careful.
Attachment #8728312 - Flags: review?(jdemooij) → review+
Attachment #8728313 - Flags: review?(jdemooij) → review+
Assignee

Comment 29

3 years ago
Comment on attachment 8733566 [details] [diff] [review]
Fully-concatenated patch, contains no tests

Ugh, I thought I'd made a request already, and I was going to land today.  :-(

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

See comment 0.  This battle is lost.

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

I elided the comment on js::Generation, and I left the existing comments by the generation() methods in HashTable code.  Those changes can be a separate patch to land later.  With those mini-adjustments, I don't think there's anything more that can be done to camouflage the issue.  (Note that because this combines the HashTable generation-changing code and the ExpandoAndGeneration generation-changing code, along with low-level JIT primitives, this is semi-camouflaged already by dint of information overload to the reader.  Sort of.)

* Which older supported branches are affected by this flaw?  If not all supported branches, which bug introduced the flaw?

All of them.

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

Shouldn't be bad.  Will do after this sticks.

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

The important parts of this are pretty small, but it's a moderate amount of change.  I'm somewhat inclined to say either landing now, or maybe somewhat later (the slightly more paranoid option), is the right balance of testing and exposure.  Since I don't have approval to land now, I can wait a bit here.  I think I'd hesitate at landing more than a week after this, tho, if at all possible.
Attachment #8733566 - Flags: sec-approval?
Comment on attachment 8733566 [details] [diff] [review]
Fully-concatenated patch, contains no tests

sec-approval+.

We'll want this on branches.
https://hg.mozilla.org/mozilla-central/rev/967dcb05f347
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Attachment #8733566 - Flags: sec-approval? → sec-approval+
Looks good. Waldo, can you request uplift?
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 33

3 years ago
Posted patch Aurora patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: ancient, at least Mozilla 1.8 (whenever js/src/jsdhash.h was introduced likely)
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: landed in m-i a couple days ago; sadly, 32-bit integer overflow means this isn't automatically-testable
[Risks and why]: not too many, the backport was painless
[String/UUID change made/needed]: N/A
Flags: needinfo?(jwalden+bmo)
Attachment #8734906 - Flags: approval-mozilla-aurora?
Assignee

Comment 34

3 years ago
Posted patch Beta patch (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]: ancient
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: landed in m-i; not auto-testable because of 32-bit wraparound necessity
[Risks and why]: beta backport had slightly more backporting pain -- mostly related to MacroAssembler backporting across some refactoring we've done on trunk; but such issues should be sussed out by a compilation failure pretty easily -- so still fairly low risk, but higher than with other backportings
[String/UUID change made/needed]: N/A
Attachment #8734907 - Flags: approval-mozilla-beta?
Assignee

Comment 35

3 years ago
Posted patch Beta patchSplinter Review
Oops, wrong patch last time -- see previous approval-request comment.
Attachment #8734907 - Attachment is obsolete: true
Attachment #8734907 - Flags: approval-mozilla-beta?
Attachment #8734908 - Flags: approval-mozilla-beta?
Assignee

Comment 36

3 years ago
Posted patch ESR45 patchSplinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: trunk is 48, right?
Risk to taking this patch (and alternatives if risky): mostly that I messed up backporting of MacroAssembler changes, but such mistakes should appear as compilation failures -- so not horribly risky; no alternatives
String or UUID changes made by this patch: N/A
Attachment #8734909 - Flags: approval-mozilla-esr45?
Comment on attachment 8734908 [details] [diff] [review]
Beta patch

May be a bit risky but I think we should take this for beta 6.
Attachment #8734908 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8734906 [details] [diff] [review]
Aurora patch

sec-high, taking this for aurora
Attachment #8734906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 40

3 years ago
Posted patch ESR38 patchSplinter Review
Backporting to ESR38 (shoot me now) is a little different from backporting to 45+.

First, our hash table code has different orderings/sizings of the various HashTable fields around the generation number.  Different sizes of bit fields, other things.  I fiddled stuff into the state it's in on trunk, because I'm not sure it's a good idea to give up efficient packing, and because being *more* like trunk seems likely safer than having a smaller change, that's *less* similar to what we'll test and run with elsewhere.

(There's one additional change, for this stuff: js/src/shell/jsheaptools.cpp needed a modification, because it does some manual generation-comparing.  If I were on trunk I'd make that use relookupForAdd [?, not sure exactly what it is that exists for that, but I'm ~99% sure there's something] to avoid the manual generation-comparing.  But for a backport to an ESR, a smaller change seems best.)

Second, Imm64 doesn't exist in ESR38.  Maybe it's backportable, maybe not.  I dunno.  Given the ExpandoAndGeneration bits of this are a hedge against risk, without certain danger if we don't make the changes, I think we should not bother making ExpandoAndGeneration::generation a uint64_t.  Backporting a JIT feature to address only-theoretical concerns seems unwise to me.

So: r?luke for the modified ESR38 hash table code.  And r?jandem to agree on not backporting ExpandoAndGeneration modifications (or rewriting them to not use Imm64, or backporting Imm64, or some other belabored trick).
Attachment #8734958 - Flags: review?(luke)
Attachment #8734958 - Flags: review?(jdemooij)

Updated

3 years ago
Attachment #8734958 - Flags: review?(luke) → review+
Comment on attachment 8734958 [details] [diff] [review]
ESR38 patch

Review of attachment 8734958 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #40)
> Given the ExpandoAndGeneration bits of this are a hedge against
> risk, without certain danger if we don't make the changes, I think we should
> not bother making ExpandoAndGeneration::generation a uint64_t.  Backporting
> a JIT feature to address only-theoretical concerns seems unwise to me.

Makes sense to me. I'd also be fine with disabling these stubs completely on ESR 38 btw, but that doesn't seem necessary here.
Attachment #8734958 - Flags: review?(jdemooij) → review+
Reporter

Updated

3 years ago
Group: javascript-core-security → core-security-release
The patch is pretty big and I am bit uncomfortable to take in esr
> [Risks and why]: not too many, the backport was painless
Could you detail what it means?
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 44

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #43)
> The patch is pretty big and I am bit uncomfortable to take in esr
> > [Risks and why]: not too many, the backport was painless
> Could you detail what it means?

There are two separate issues fixed by this patch.

===

First: js::HashSet::generation() and js::HashMap::generation() pre-patch are 32-bit integer counters, recording the "generation" of the structure.  If at times T1 and T2 the counter has the same generation value, then the structure hasn't changed -- pointers to keys/values are all valid.  Every increment denotes a new generation.

But it's easy to make generation-incrementing overflow to 0 if generations are 32 bits.  That makes "same value" comparisons unusable.  Malicious code can make this happen with the non-standard Object.prototype.watch method.

This is fixed by the changes to GCHashTable.h, HashTable.h, jsapi.h, jscntxt.h, jswatchpoint.cpp, UbiNodeCensus.cpp, Opaque.h, and moz.build.

Most of these files' changes *remove* exposure of generation, reducing attack surface, unquestionably safe.  The rest make generation an opaque 64-bit integer that can't overflow this millennium.  ;-)  The only interesting changes are to HashTable.h, and they're straightforward -- basically one way to do it.

I don't think this part of the patch has any particular risk.

===

Second: we have a separate "generation" concept used for a JIT optimization, where that generation's another 32-bit integer that might also overflow.

(Theoretically -- we don't know malicious code can trigger this, due to the slow-script dialog [assuming the user doesn't hit Continue a hundred times].  But it's not certain, and the slow-script dialog *is not 100% reliable* last I checked.  Certainly it's not meant as security mechanism.  So we'd really rather fix this than take the risk.)

The changes for this half of the patch involve two classes of change.

First, we require two low-level JIT mechanism for 64-bit integer comparison.  On 64-bit these are easy; on 32-bit they decompose into two 32-bit operations.  All of this reuses existing functionality to do 32-bit or pointer-sized ops.  It changes a bunch of files, but it's quite simple.  And if we were to make a mistake here, a comparison would almost certainly *fail* -- which would trigger safe (but less-fast) behavior.

Second, we have to change a couple places (IonCaches.cpp, SharedIC.cpp) *performing* the aforementioned comparisons, to do 64-bit instead of 32-bit comparisons.  This is again pretty simple -- and again, a mistake almost certainly means safe-but-slower behavior.

So: two sub-parts, each pretty simple (tho one is a larger *number* of simple changes), and a mistake probably means being slow, not unsafe.

===

The first issue *must* be backported, because it's definitely exploitable.  This part of the patch is very similar to the trunk-work with an easy backport.  We have no reason not to take this, and good reason (exploitable!) to take it.

Re the second issue, I meant what I said in the approval request.  The risk of this change is low.  But *if* the backport were complicated, the larger number of changes would mean resulting risk -- too much for a quasi-speculative issue.  It wasn't a complicated backport, so we should take this.
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8734958 [details] [diff] [review]
ESR38 patch

ok, taking it on esr38
Attachment #8734958 - Flags: approval-mozilla-esr38+
Comment on attachment 8734909 [details] [diff] [review]
ESR45 patch

Important issue, taking it.
Should be in 45.1.0
Attachment #8734909 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment 24 makes an attempt to verify the fix. Comment 25 mentions the possible need for more tests. Without an actual PoC or further tests, I won't be able to have this verified otherwise. If someone provides the above and more information, I'm happy to take a look at it. Marking qe-verify-.
Flags: qe-verify-
Whiteboard: CESG REF: 54224850 / VULNERABILITY ID: 426732 → [post-critsmash-triage] CESG REF: 54224850 / VULNERABILITY ID: 426732

Comment 50

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #48)
> https://hg.mozilla.org/releases/mozilla-esr38/rev/fa4efccde9b7

This landed without mfbt/Opaque.h.  Fixed in https://hg.mozilla.org/releases/mozilla-esr38/rev/486740b9903a.
Thanks for that.
Even with that, though, builds were still busted like https://treeherder.mozilla.org/logviewer.html#?job_id=71468&repo=mozilla-esr38

Backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/861f6b83ce1d to fix the builds up.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(ehsan)
Assignee

Comment 52

3 years ago
https://hg.mozilla.org/releases/mozilla-esr38/rev/71f611fd27c7d6cb7d6dab9895c2922948042543 *eyes pulsebot's failure to comment in sec-bugs beadily*

Setting aside missing mfbt/Opaque.h, the subsequent failures were due to the changes noted in the first part of comment 40.  ("It all built locally for me...")

On doing archaeology to determine how trunk differed, I discovered these differences were introduced in bug 1136046, with a somewhat more involved patch than the mini-changes I made here that buggily-half-ported that work.  The extent of that work certainly isn't something I have any desire to try to backport, nor any need to do so.  My adjustments were just to try to be consistent, not because necessity.

So I reverted those bits of change, leaving only the uint32_t->uint64_t change and rearranging of two fields to not pack stupidly.  HashTable might be slightly bigger on ESR38 for lack of the packing I half-attempted, but I think we can live with that.  :-)
Flags: needinfo?(jwalden+bmo)

Comment 53

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #51)
> Thanks for that.
> Even with that, though, builds were still busted like
> https://treeherder.mozilla.org/logviewer.html#?job_id=71468&repo=mozilla-
> esr38
> 
> Backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/861f6b83ce1d
> to fix the builds up.

I doubt there's anything for me to do here. :-)
Flags: needinfo?(ehsan)
Whiteboard: [post-critsmash-triage] CESG REF: 54224850 / VULNERABILITY ID: 426732 → [post-critsmash-triage][adv-main46+][adv-esr45.1+][adv-esr38.8+] CESG REF: 54224850 / VULNERABILITY ID: 426732
Alias: CVE-2016-2808
Depends on: 1266366
Assignee

Comment 54

3 years ago
Landed updated/clearer comments, now that we're a ways past this being fixt everywhere:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7a9ba853405d5f05dd4b62cf83ebc54bbc1968

It'd be nice to land a testcase as well, but as a practical matter it's not really feasible to overflow a 32-bit counter here with the amount of work to do each iteration -- not in short enough time to have a test for it.  So let's be proactive and testsuite- this.
Flags: in-testsuite? → in-testsuite-
Reporter

Updated

3 years ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.