Bug 1348168 (CVE-2017-5428)

integer overflow in createImageBitmap() overload accepting ArrayBuffer and ArrayBufferView arguments (pwn2own 2017)

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: rforbes, Assigned: Ehsan)

Tracking

({csectype-intoverflow, regression, sec-critical})

unspecified
mozilla55
csectype-intoverflow, regression, sec-critical
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52blocking verified, firefox-esr5252+ verified, firefox53blocking verified, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [keep hidden while bug 1348894 is unfixed])

Attachments

(13 attachments, 2 obsolete attachments)

17.68 KB, text/html
Details
56 bytes, text/html
Details
56 bytes, text/html
Details
4.78 KB, text/plain
Details
7.56 KB, application/javascript
Details
11.02 KB, text/plain
Details
10.45 KB, text/plain
Details
3.23 KB, patch
mccr8
: review-
Details | Diff | Splinter Review
4.69 KB, text/html
Details
6.72 KB, patch
decoder
: feedback+
dbaron
: feedback-
Details | Diff | Splinter Review
1.47 KB, patch
Details | Diff | Splinter Review
3.49 KB, patch
bzbarsky
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
1.92 KB, text/plain
dveditz
: feedback+
Details
(Reporter)

Description

2 years ago
Created attachment 8848313 [details]
magic.html

this is the vuln we got from pwn2own.  more later.
(Reporter)

Updated

2 years ago
Group: core-security
(Reporter)

Comment 1

2 years ago
Created attachment 8848314 [details]
exploit stuff
(Reporter)

Updated

2 years ago
Component: JavaScript Engine → Graphics
(Reporter)

Comment 2

2 years ago
Created attachment 8848315 [details]
index.html
Component: Graphics → JavaScript Engine
Component: Graphics
(Reporter)

Comment 3

2 years ago
Created attachment 8848316 [details]
Firefox.md
(Reporter)

Comment 4

2 years ago
Created attachment 8848317 [details]
long.min.js
Attachment #8848316 - Attachment mime type: application/x-genesis-rom → text/plain
Created attachment 8848325 [details]
AddressSanitizer Trace (52.0.1, release ASan build)
Looking at Firefox.md, the source code region seems to have been added in bug 1141979:

[FoxEye] Extend ImageBitmap with interfaces to access its underlying image data


Specifically the CreateImageFromBufferSourceRawData function at:

https://hg.mozilla.org/mozilla-central/rev/0282afe01d9e#l1.39

and the following lines, possibly:

+    // Create a layers::Image and set data.
+    if (aFormat == ImageBitmapFormat::YUV444P ||
+        aFormat == ImageBitmapFormat::YUV422P ||
+        aFormat == ImageBitmapFormat::YUV420P) {
+      RefPtr<layers::PlanarYCbCrImage> image =
+        new layers::RecyclingPlanarYCbCrImage(new layers::BufferRecycleBin());
+
+      if (NS_WARN_IF(!image)) {
+        return nullptr;
+      }
+
+      // Set Data.
+      if (NS_WARN_IF(!image->CopyData(data))) {
+        return nullptr;
+      }
+
+      return image.forget();
+    } else {

The Firefox.md file mentions: "Nothing is checked before the data is passed into `RecyclingPlanarYCbCrImage::CopyData`."
Steps to reproduce for developers:

1. Download files "magic.html", "index.html" and "long.min.js" from this bug into the same directory.
2. Create a file "shellcode" with any content (otherwise the XHR in magic.html fails).
3. Launch the browser, pointing to "index.html".

Reproduces cleanly on AddressSanitizer builds (https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer).
(Assignee)

Comment 8

2 years ago
I'm writing a patch from the description, and trying to reproduce at the same time.
Assignee: nobody → ehsan
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
tracking-firefox52: --- → blocking
tracking-firefox53: --- → blocking
tracking-firefox54: --- → +
tracking-firefox55: --- → +
tracking-firefox-esr52: --- → 52+
Keywords: csectype-intoverflow, sec-critical
Summary: pwn2own vulnerability → createImageBitmap integer overflow (pwn2own 2017)
(Assignee)

Updated

2 years ago
See Also: → bug 1279569
Created attachment 8848328 [details]
AddressSanitizer Trace (mozilla-central tip, hgrev 312f53856798, debug)
(Assignee)

Comment 10

2 years ago
Created attachment 8848336 [details] [diff] [review]
Avoid in place const_casts
Attachment #8848336 - Flags: review?(continuation)
Alias: CVE-2017-5428
Comment on attachment 8848336 [details] [diff] [review]
Avoid in place const_casts

> CreateImageFromBufferSourceRawData(const uint8_t*aBufferData,
>                                    uint32_t aBufferLength,
>                                    mozilla::dom::ImageBitmapFormat aFormat,
>                                    const Sequence<ChannelPixelLayout>& aLayout)
> {
>   MOZ_ASSERT(aBufferData);
>   MOZ_ASSERT(aBufferLength > 0);
>@@ -1957,24 +1969,33 @@ CreateImageFromBufferSourceRawData(const uint8_t*aBufferData,
>     // Prepare the PlanarYCbCrData.
>     const ChannelPixelLayout& yLayout = aLayout[0];
>     const ChannelPixelLayout& uLayout = aFormat != ImageBitmapFormat::YUV420SP_NV21 ? aLayout[1] : aLayout[2];
>     const ChannelPixelLayout& vLayout = aFormat != ImageBitmapFormat::YUV420SP_NV21 ? aLayout[2] : aLayout[1];
> 
>     layers::PlanarYCbCrData data;
> 
>     // Luminance buffer

Is the code above this ok?

    const uint8_t* srcBufferPtr = aBufferData;
    uint8_t* dstBufferPtr = dstMap.GetData();

    for (int i = 0; i < srcSize.height; ++i) {
      memcpy(dstBufferPtr, srcBufferPtr, srcStride);
      srcBufferPtr += srcStride;
      dstBufferPtr += dstMap.GetStride();
    }
and such.

(ok, that might just read random data)
Created attachment 8848339 [details]
neutered version of magic.html

I removed the shellcode bits.  This crashes for me locally, and should crash when loaded directly from bugzilla (since I changed the link to point to bugzilla's long.min.js file attached above).
Comment on attachment 8848336 [details] [diff] [review]
Avoid in place const_casts

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

In IRC, patch was found to be insufficient. Presumably some of the math inside CopyData or CopyPlane.
Attachment #8848336 - Flags: review?(continuation) → review-
Created attachment 8848340 [details] [diff] [review]
I was worried about integer overflows here...

(This doesn't fix the crashes I can reproduce using the above, though, but it was the first thing I looked at given the description in Firefox.md (attachment 8848316 [details]), and seems like it may well be bad given the way CopyData is called from the function above.)
Created attachment 8848345 [details] [diff] [review]
bitmap-stride.patch

So some form of validation needs to be done on inputs here.

Likely similar problems are nearby. We should audit around here.
Comment on attachment 8848345 [details] [diff] [review]
bitmap-stride.patch

Confirmed that this patch fixes the particular crash we were seeing on mozilla-central ASan build.
Attachment #8848345 - Flags: feedback+
Created attachment 8848347 [details] [diff] [review]
patch that does directly fix integer overflows

The reason my previous patch didn't fix the exploit is that the result of this nice size_t calculation got implicitly converted to uint32_t both when calling AllocateBuffer and when assigning to mBufferSize.  Using uint32_t from the beginning does make the exploit no longer crash.
Attachment #8848340 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
While people were looking at this code on IRC, it seemed like the more we looked at it, the more security holes we found.  We decides that it's best to disable the API that bug 1141979 added altogether.
Of course we shouldn't ever have exposed that method to the web :/ I don't know what happened but http://searchfox.org/mozilla-central/source/dom/webidl/WindowOrWorkerGlobalScope.webidl#78-81 isn't done.
(Assignee)

Comment 20

2 years ago
Created attachment 8848348 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review
Attachment #8848348 - Flags: review?(jgilbert)
(Assignee)

Updated

2 years ago
Attachment #8848348 - Flags: review?(bugs)
Comment on attachment 8848348 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review

So can we now have extended attributes there? If so, r+, if not, add the check to c++.
Attachment #8848348 - Flags: review?(bugs) → review+
(Assignee)

Comment 22

2 years ago
Comment on attachment 8848348 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review

The patch doesn't build!
Attachment #8848348 - Attachment is obsolete: true
Attachment #8848348 - Flags: review?(jgilbert)
Right, you can't do this via IDL, because the IDL annotations control whether the function is exposed in JS, but there is only _one_ JS function for all the overloads...

It looks like https://bug1141979.bmoattachments.org/attachment.cgi?id=8686383 (from bug 1141979) simply never got checked in?  :(
I gave Ehsan the mozconfig to make an ASan build for fix verification later on, as I'm signing off now.
(Assignee)

Comment 25

2 years ago
Created attachment 8848355 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review
Attachment #8848355 - Flags: review?(bzbarsky)
Comment on attachment 8848355 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review

>+  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
>+  MOZ_ASSERT(workerPrivate);
>+  JSContext* cx = workerPrivate->GetJSContext();
>+  MOZ_ASSERT(cx);

  JSContext* cx = GetCurrentThreadJSContext();

And please file a followup to make this stuff all saner.  For example, have the bindings pass in the JSContext so we stop doing the "pass null on mainthread because we peeked at the impl" thing, and have a one-arg version of ImageBitmap::ExtensionsEnabled that just takes the cx, called from these two callsites and the two-arg version.   But again, followup for that.

r=me
Attachment #8848355 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 27

2 years ago
(In reply to Christian Holler (:decoder) from comment #24)
> I gave Ehsan the mozconfig to make an ASan build for fix verification later
> on, as I'm signing off now.

I'm making a local ASan build to do a final verification to be 100% sure.
(Assignee)

Comment 28

2 years ago
Comment on attachment 8848355 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Impossible.  We are removing the API the exploit can be written based on.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes.  They point to the entry point for the vulnerability.

Which older supported branches are affected by this flaw? All, including ESR52 bug not ESR45.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should be backportable easily.

How likely is this patch to cause regressions; how much testing does it need? This API was accidentally exposed to the Web, so there is web compat risk here.  dveditz said someone from product has OK'ed the removal.

We should work on adding this back ASAP after fixing the code properly.
Attachment #8848355 - Flags: sec-approval?
Blocks: 1141979
Group: core-security → gfx-core-security
Keywords: regression
Comment on attachment 8848355 [details] [diff] [review]
Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review

[Triage Comment]
sec-approval+ and a=dveditz for all the branches
Attachment #8848355 - Flags: sec-approval?
Attachment #8848355 - Flags: sec-approval+
Attachment #8848355 - Flags: approval-mozilla-release+
Attachment #8848355 - Flags: approval-mozilla-esr52+
Attachment #8848355 - Flags: approval-mozilla-beta+
Attachment #8848355 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 30

2 years ago
(In reply to :Ehsan Akhgari from comment #27)
> (In reply to Christian Holler (:decoder) from comment #24)
> > I gave Ehsan the mozconfig to make an ASan build for fix verification later
> > on, as I'm signing off now.
> 
> I'm making a local ASan build to do a final verification to be 100% sure.

I did that. "failed to find corrupted array" reloaded many times. no crash. landing now. dinner after!
Fwiw, I got crashes on dbaron's testcase without the patch, no crashes with.  Exceptions verified thrown instead with the patch.

I've just done two try pushes:

1)  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f37932b94f0f1145407f1a880e55de00e67a7ecf -- just Ehsan's patch (the version I was reviewing).

2)  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6d814a7c654e5e9a650a1936bef05f03a29dcb6 -- same thing but with our tests that set the pref changed to not set it, to verify that they would fail that way.
(Assignee)

Updated

2 years ago
Summary: createImageBitmap integer overflow (pwn2own 2017) → integer overflow in createImageBitmap() overload accepting ArrayBuffer and ArrayBufferView arguments (pwn2own 2017)
(Assignee)

Comment 34

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #23)
> It looks like
> https://bug1141979.bmoattachments.org/attachment.cgi?id=8686383 (from bug
> 1141979) simply never got checked in?  :(

This is what led to the fiasco: bug 1141979 comment 134.  :-(
Comment on attachment 8848345 [details] [diff] [review]
bitmap-stride.patch

>+static bool
>+IsLayoutValid(const ChannelPixelLayout& layout, uint32_t availBytes)
>+{
>+  if (!layout.mWidth && !layout.mHeight)
>+    return true;
>+
>+  const auto bytesPerDataType = GetBytesPerPixelValue(layout.mDataType);
>+  const auto bytesPerPixelStride = CheckedInt<uint32_t>(bytesPerDataType) + layout.mSkip;
>+
>+  const auto bytesPerRow = bytesPerPixelStride*(layout.mWidth-1) + bytesPerDataType;
>+  if (!bytesPerRow.isValid() || bytesPerRow.value() > layout.mStride)
>+    return false;
>+
>+  const auto bytesNeeded = CheckedInt<uint32_t>(layout.mOffset) +
>+                           CheckedInt<uint32_t>(layout.mStride)*(layout.mHeight-1) +
>+                           bytesPerRow;
>+  return bytesNeeded.isValid() && bytesNeeded.value() <= availBytes;
>+}

So, a few comments on this checking function relative to what RecyclingPlanarYCbCrImage::CopyData and its static helper CopyPlane do.

In CopyPlane, skip is the amount to skip, in the source only, between uint8_t units.  IsLayoutValid assumes that skip is only added after every bytesPerDataType bytes, but CopyPlane adds it every byte.  This means that CopyPlane could copy past the stride in ways that IsLayoutValid wouldn't detect.  It's not clear to me which the skip is supposed to be, although I have a sneaking suspicion that IsLayoutValid is right and CopyPlane is wrong.

CopyData also stores the skip values in its mData, despite CopyPlane *not* doing the skipping in its own buffer, only the source buffer.  I'm not sure if anyone uses that, but if they do, it'll be wrong.  (That's largely independent of IsLayoutValid.)

IsLayoutValid should also test bytesPerPixelStride.isValid(), to protect against layout.mSkip of 0xffffffff or similar.
Attachment #8848345 - Flags: feedback-
CopyPlane is wrong then, based on the spec document.

This whole thing is a mess. We should definitely cauterize this out. This needs a minor rewrite before we can even think about turning this back on.

I'll see how far I get tomorrow.
However, we should not waste time trying to find a minimal fix for this gaping hole of an entrypoint.
When I was poking at this stuff and trying to verify the fix on workers, I ran into more lack of input validation.  Filed that as bug 1348219.
And for those following along, the worker test doesn't fail on the second try run in comment 31 because of bug 1348215.  That doesn't affect the correctness of this fix, though.
(In reply to Jeff Gilbert [:jgilbert] from comment #36)
> This whole thing is a mess. We should definitely cauterize this out. This
> needs a minor rewrite before we can even think about turning this back on.

We should also make sure our fuzzing people are covering this API and have tested it before turning it on again.
Should you post an "intent to unship" for this API?
Flags: needinfo?(ehsan)
Comment on attachment 8848347 [details] [diff] [review]
patch that does directly fix integer overflows

Fwiw, this is exactly the spot that first caught my attention when I looked at the code yesterday and I tried to write an assert for it to catch the overflow there (but it didn't work on first attempt).

Imho we should make it impossible to have such multiplications in the code that are both unchecked and unannotated. Either the developer explicitly wants overflow to happen (and should express that with some kind of annotation), or overflow checking must happen. This could probably be enforced statically.
Component: Graphics → DOM
Group: gfx-core-security → dom-core-security
We're not unshipping createImageBitmap() function itself, just a non-standard overload we added. Pretty sure we never did an intent to ship on that part: it was supposed to be hidden behind an experimental pref in the first place.
That is very true, since even the comment in .webidl said that was supposed to happen, to disable the experimental code behind a pref.
(In reply to Andrew McCreight [:mccr8] from comment #39)
> (In reply to Jeff Gilbert [:jgilbert] from comment #36)
> > This whole thing is a mess. We should definitely cauterize this out. This
> > needs a minor rewrite before we can even think about turning this back on.
> 
> We should also make sure our fuzzing people are covering this API and have
> tested it before turning it on again.

I think fuzzing can help here, but I also believe (as I said in comment 41) that the kind of integer overflow exploited here could probably be detected statically if we enforce that all intended overflows are annotated somehow by developers. I guess it should be possible to do this in e.g. clang with an additional pass or in their static analysis plugin. What I'm unsure about is the way of annotating. I know you can make annotations on a function level, not sure if that would be sufficient. In the end, I would like all code that does multiplications like this to simply not compile anymore if you don't annotate or use the checked API.
Group: dom-core-security → gfx-core-security
Component: DOM → Graphics
(Assignee)

Comment 45

2 years ago
Created attachment 8848515 [details]
intent to unship email

No matter what we call it, we _did_ unship an API, we really should just admit to the mistake that we made IMHO.  This is the email I'm planning to send out.  What do you think, Daniel?  I obviously don't want to leak out anything security sensitive.  It seems the fix also isn't live (not even merged to central yet???)
Flags: needinfo?(ehsan)
Attachment #8848515 - Flags: feedback?(dveditz)
(Assignee)

Comment 46

2 years ago
(In reply to Christian Holler (:decoder) from comment #44)
> (In reply to Andrew McCreight [:mccr8] from comment #39)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #36)
> > > This whole thing is a mess. We should definitely cauterize this out. This
> > > needs a minor rewrite before we can even think about turning this back on.
> > 
> > We should also make sure our fuzzing people are covering this API and have
> > tested it before turning it on again.
> 
> I think fuzzing can help here

Fuzzing should be a *requirement* for an API that accepts a user buffer like this.  The problem is that when the intention is to not ship an API, we have nothing to catch you if you screw something up and ship it by accident.  We have some rudimentary protection against this in the form of test_interfaces.html and friends, and this clearly shows that we need something more sophisticated.

, but I also believe (as I said in comment 41)
> that the kind of integer overflow exploited here could probably be detected
> statically if we enforce that all intended overflows are annotated somehow
> by developers.

That's not really practical.  Bug 1279569, however, is, and it would have caught this.

FTR (and not to sound bitter) we keep talking about this but we never act on this.  I've been saying for years that there are whole classes of security vulnerabilities that we could *completely* eliminate from our code forever into the future using static analysis, but we need a concentrated effort around it.  Often times because you can't land the analysis (to prevent more broken code from being checked in, and also to show a recipe for people to find security holes in Firefox) and often times the analysis find gazillions of issues.  I am sounding like a broken record but I keep saying this.  We need to focus on this more seriously.  :-)
(In reply to :Ehsan Akhgari from comment #46)

> 
> That's not really practical.  Bug 1279569, however, is, and it would have
> caught this.

I can't see that bug, can you Cc me please?

> 
> FTR (and not to sound bitter) we keep talking about this but we never act on
> this.  I've been saying for years that there are whole classes of security
> vulnerabilities that we could *completely* eliminate from our code forever
> into the future using static analysis, but we need a concentrated effort
> around it.  Often times because you can't land the analysis (to prevent more
> broken code from being checked in, and also to show a recipe for people to
> find security holes in Firefox) and often times the analysis find gazillions
> of issues.  I am sounding like a broken record but I keep saying this.  We
> need to focus on this more seriously.  :-)


I am 100% with you and I feel exactly the same way about it. I started doing the UBSan work years ago, then I encountered the signed integer overflow problem. But we never got anywhere with it because e.g. layout/ just does it (tm) and while we discussed various ways to solve this situation (which in all cases involves developers), we never solved it. That is just frustrating. If static analysis here isn't practical, then maybe adding overflow checks at runtime is (clang can do this already for both signed and unsigned overflows). The problem here is and has always been performance. But maybe we can exclude some performance critical code and make the other behavior at least the default. That would cause runtime aborts. Instead of that, we could also truncate for stability reasons I guess (i.e. leave it at max/min)? I think the major problem is that integer overflow is almost never intended, so trying to shift the default behavior seems like the right approach to me, even if that is complicated.

Still curious about your proposal in bug 1279569 :)
(Assignee)

Comment 48

2 years ago
(In reply to Christian Holler (:decoder) from comment #47)
> (In reply to :Ehsan Akhgari from comment #46)
> 
> > 
> > That's not really practical.  Bug 1279569, however, is, and it would have
> > caught this.
> 
> I can't see that bug, can you Cc me please?

Done.

> I am 100% with you and I feel exactly the same way about it. I started doing
> the UBSan work years ago, then I encountered the signed integer overflow
> problem. But we never got anywhere with it because e.g. layout/ just does it
> (tm) and while we discussed various ways to solve this situation (which in
> all cases involves developers), we never solved it. That is just
> frustrating. If static analysis here isn't practical, then maybe adding
> overflow checks at runtime is (clang can do this already for both signed and
> unsigned overflows). The problem here is and has always been performance.
> But maybe we can exclude some performance critical code and make the other
> behavior at least the default. That would cause runtime aborts. Instead of
> that, we could also truncate for stability reasons I guess (i.e. leave it at
> max/min)? I think the major problem is that integer overflow is almost never
> intended, so trying to shift the default behavior seems like the right
> approach to me, even if that is complicated.

Very simplistically speaking, this is a trade-off between programmer convenience and runtime efficiency as you note.  The basic idea in bug 1279569 is to start to treat some arithmetic expressions that we know can have disastrous consequences in the case of an overflow as requiring overflow checks statically in order to hopefully come up with a good trade-off here.

> Still curious about your proposal in bug 1279569 :)

FWIW I'm not claiming _I_ had a good proposal there, see bug 1279569 comment 28.  :-)  Now I think it makes a lot of sense to treat some variable names especially.  This bug was definitely a good learning experience.
Comment on attachment 8848515 [details]
intent to unship email

I think we need to be clearer that we are _not_ disabling the standard createImageBitmap overloads.  Maybe in the first paragraph, replace "no other engine ships this API" with "no other engine ships this overload of createImageBitmap" and "unship the API" with "unship the non-standard overload"?

And maybe emphasize that this overload was never release noted, is not documented anywhere, and would throw in any other browser, and hence the chances of anyone using it are extremely low?
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #49)
> And maybe emphasize that this overload was never release noted, is not
> documented anywhere, and would throw in any other browser, and hence the

"not documented anywhere" is probably too strong, given that there's a spec for it at https://w3c.github.io/mediacapture-worker/#imagebitmapfactories-interface-extensions .  (Contrast with the spec for the primary createImageBitmap at https://html.spec.whatwg.org/multipage/webappapis.html#images-2 .)
That's fair.  Not documented on MDN, or other places that are actually documentation for this stuff that people read.  ;)

That spec needs a lot of work, by the way.  :(
Group: gfx-core-security → core-security-release
(Assignee)

Comment 52

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #49)
> Comment on attachment 8848515 [details]
> intent to unship email
> 
> I think we need to be clearer that we are _not_ disabling the standard
> createImageBitmap overloads.  Maybe in the first paragraph, replace "no
> other engine ships this API" with "no other engine ships this overload of
> createImageBitmap" and "unship the API" with "unship the non-standard
> overload"?

Fair, will do.

> And maybe emphasize that this overload was never release noted, is not
> documented anywhere, and would throw in any other browser, and hence the
> chances of anyone using it are extremely low?

Will note, not documented on MDN per dbaron.
Comment on attachment 8848515 [details]
intent to unship email

f+ with nits? :-)

I'm with Boris that you should be clearer in the sentence in the first paragraph that we're not unshipping the entire API. You get more specific later, but let's not alarm people at the top.

that is:
As to the best of our knowledge no other engine ships this API and we never intended to ship it and never publicized the fact that we did, we decided that the best course of action, as unusual as this would be, is to immediately unship the API.

"this API" could mean the entire function, while you intend it to mean only one overloaded version. So something like "no other engine ships this variant of createImageBitmap... unship this variant of the API."

Would really like not to send this until after the release is available.
Attachment #8848515 - Flags: feedback?(dveditz) → feedback+
https://hg.mozilla.org/mozilla-central/rev/ac0658d7d958
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 55

2 years ago
OK, I also don't want to send it before the release is available either.

Do we have a rough estimate on when that would be?  Thanks!
Reproduced the crash using the POC's that have been attached using the STR from comment#7. I've also reproduced the issue using the POC from comment#12. Used the following build to reproduce:
* build uses: fx52, buildid: 20170302120751, changeset: 44d6a57ab554

Used the following two builds on the three platforms listed below:
* fx52.0.1, buildid: 20170316213829, changeset: 2f2b4a119565
* fx52.0.1esr, buildid: 20170316213902, changeset: 4af7cd795eee

Platforms used:

* macOS 10.12.3 x64 - PASSED (couldn't reproduce the crash)
* Win 10 Pro x64 VM - PASSED (couldn't reproduce the crash)
* Win 8.1 x64 VM - PASSED (couldn't reproduce the crash)

Test Cases used:

* opened the POC's that have been attached in several different tabs, windows and ensured that fx doesn't crash via CreateImageFromBufferSourceRawData
** ensured that "failed to find corrupted array" is being displayed when the POC loads within the tab
** attempted opening the POC via several different methods (File -> Open, Drag & Drop, clicking on the POC and opening it in the default browser)

:mwobensmith is going through verifications for linux.
Confirmed crash using dbaron's POC on Linux Ubuntu 14.04 and Android ARM, Fx52 release.

Verified no crash using Fx52.0.1 (Linux/Android) and Fx52.0.1esr (Linux).

Sometimes the crash took several page refreshes on affected builds to reproduce, so I did this many times on fixed builds to make sure it no longer happens.

On fixed builds, we see page text indicating "failed to find corrupted array".
status-firefox52: fixed → verified
status-firefox-esr52: fixed → verified
Reproduced the crash using the POC's that have been attached using the STR from comment#7. I've also reproduced the issue using the POC from comment#12. Used the following build to reproduce:
* build uses: fx52, buildid: 20170302120751, changeset: 44d6a57ab554

Using the same process/STR as described in comment#56, I went through verification using the following build:
* fx53.0b3, buildid: 20170316045436, changeset: c385ff88725d

Platforms Used:

* macOS 10.12.3 x64 - PASSED (couldn't reproduce the crash)
* Win 10 Pro x64 VM - PASSED (couldn't reproduce the crash)
* Win 8.1 x64 VM - PASSED (couldn't reproduce the crash)
* Ubuntu 16.04.2 LTS x64 VM - PASSED (couldn't reproduce the crash)
status-firefox53: fixed → verified
(Assignee)

Comment 59

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #26)
> Comment on attachment 8848355 [details] [diff] [review]
> Disable Mozilla custom ImageBitmap extensions that didn't go through proper
> API review
> 
> >+  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> >+  MOZ_ASSERT(workerPrivate);
> >+  JSContext* cx = workerPrivate->GetJSContext();
> >+  MOZ_ASSERT(cx);
> 
>   JSContext* cx = GetCurrentThreadJSContext();
> 
> And please file a followup to make this stuff all saner.  For example, have
> the bindings pass in the JSContext so we stop doing the "pass null on
> mainthread because we peeked at the impl" thing, and have a one-arg version
> of ImageBitmap::ExtensionsEnabled that just takes the cx, called from these
> two callsites and the two-arg version.   But again, followup for that.

Filed bug 1348452 and bug 1348453.
(Assignee)

Comment 60

2 years ago
With the advisory being public and the release live, I sent out the email.
I filed bug 1348894 to land the integer overflow fix in comment 17 (with a previous non-working version in comment 14) since I *believe* that code is used elsewhere.
See Also: → bug 1348894
Whiteboard: [keep hidden while bug 1348894 is unfixed]
Group: core-security-release
See Also: → bug 1414547

Updated

11 months ago
See Also: → bug 1428947
You need to log in before you can comment on or make changes to this bug.