Closed Bug 1682068 Opened 5 months ago Closed 4 months ago

ImageBitmap::CreateInternal can use unstable typed array data

Categories

(Core :: Canvas: 2D, defect)

defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox-esr78 85+ fixed
firefox84 --- wontfix
firefox85 + fixed
firefox86 + fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main85+r][adv-esr78.7+r][sec-survey])

Attachments

(1 file)

dom::Uint8ClampedArray and friends do not keep pointed-to data alive, nor do they handle the case where the memory might be moved elsewhere during a GC. They really probably shouldn't be used outside of dom bindings, but we should probably figure out what we want to do before banning their usage.

For keeping the data alive, RootedSpiderMonkeyInterface<Uint8ClampedArray> is sufficient. For preventing the memory from being moved, well... we don't really have anything.

If I am reading the code correctly:

  • ImageData has an unrooted JSObject* that is unwrappable to a typed array
  • that object gets passed to dom::Uint8ClampedArray::Init(), which stores the JSObject* in a couple of unrooted member variables
  • note that thus far, the lack of rooting or anything is not a problem; we're not doing anything that could GC
  • the data pointer is extracted from the dom::Uint8ClampedArray
  • CreateImageFromRawData does a bunch of stuff that appears to boil down to copying the data. Copied data is safe from the marauding GC.
  • alternatively, CreateImageFromRawDataInMainThreadSyncTask could store the data pointer in a task that gets dispatched to the event loop. Before this task runs, something could GC, which could free and/or invalidate the data. (Once the task runs, I think we copy via CreateImageFromRawData and we're safe again.)

The rooting (ie, keeping the data alive) is probably not a huge issue; something probably hangs onto the object. But if we run low on memory and do a compacting GC, then if the data is stored directly in the JSObject structure, it will be moved and any access to the old pointer is a UAF. (Depending on how you create the ArrayBuffer, up to 96 bytes on 64-bit can be stored inline.)

Note that I found this by adding a JS::AutoCheckCannotGC member to dom::TypedArray_base, which revealed what is behind bug 1666285. (I mean, we already knew because ASAN told us. I was just able to get the same info from the rooting hazard static analysis.) In addition to solving the specific problem here, I would like to put something in place to catch this sort of problem statically.

The easiest fix would be to eagerly copy out the data if it could move. But then the static analysis would still complain, because it wouldn't know that it didn't need to worry about the dom::Uint8ClampedArray anymore.

I don't have an answer yet. I was looking at an approach where I would create another typed array holder class that always forced its JSObject to relocate any inline data to a fixed malloc buffer. But (1) that requires a JSContext* and I'm not sure how easy that is to provide, and (2) it's yet another weird messy typed array holder class.

At the moment, I'm thinking that I can leave the JS::AutoCheckCannotGC member, use the conditional eager copy described above, and add a JS::AutoSuppressGCAnalysis local to have it ignore the problem. This would still flag new instances of the problem that require a similar fix.

Group: core-security → gfx-core-security
Keywords: sec-high
Depends on: 1684123
Assignee: nobody → sphink
Status: NEW → ASSIGNED

Comment on attachment 9194616 [details]
Bug 1682068 - Fix rooting hazard in ImageBitmap::CreateInternal by avoiding movable data

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Very unlikely to happen. This is not a UAF; it's only reading from freed memory and so can only be used as an information leak.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: I'm guessing this will apply cleanly, though I'm not sure how often this code mutates.
  • How likely is this patch to cause regressions; how much testing does it need?: It's restricted to a very local area, and I expect it gets exercised fairly well by the tests already. (The actual problematic scenario does not, since it's unlikely that we would do a compacting GC at just the wrong point, but the revised code will trigger much more commonly.)

Main risk is a perf regression.

Attachment #9194616 - Flags: sec-approval?

Comment on attachment 9194616 [details]
Bug 1682068 - Fix rooting hazard in ImageBitmap::CreateInternal by avoiding movable data

Approved to land and request uplift

Attachment #9194616 - Flags: sec-approval? → sec-approval+

Backed out bug 1684123 and bug 1682068 for build bustage:

https://hg.mozilla.org/integration/autoland/rev/d59ed2dfcc645c959387d56ce9f04a892ee0110c

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=K8rJjbIXQ5-04X5gzzk_7g.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=e6df68a131a394198f98d63993de6954989eaec2
Failure log: https://treeherder.mozilla.org/logviewer?job_id=326612322&repo=autoland

[task 2021-01-13T18:25:55.982Z] 18:25:55 INFO - In file included from Unified_cpp_dom_canvas0.cpp:92:
[task 2021-01-13T18:25:55.982Z] 18:25:55 ERROR - /builds/worker/checkouts/gecko/dom/canvas/ImageBitmap.cpp:895:9: error: no member named 'Reset' in 'mozilla::dom::RootedSpiderMonkeyInterface<mozilla::dom::TypedArray<unsigned char, &js::UnwrapUint8ClampedArray, &JS_GetUint8ClampedArrayData, &js::GetUint8ClampedArrayLengthAndData, &JS_NewUint8ClampedArray>>'
[task 2021-01-13T18:25:55.982Z] 18:25:55 INFO - array.Reset();
[task 2021-01-13T18:25:55.982Z] 18:25:55 INFO - ~~~~~ ^
[task 2021-01-13T18:25:55.982Z] 18:25:55 INFO - 1 error generated.

Flags: needinfo?(sphink)

Oops, this depends on the patches in bug 1680607. Which I have marked as being blocked by this bug. I'll invert the relationship and push again.

Flags: needinfo?(sphink)
Depends on: 1686532

Changing blocking relationships after splitting bug 1680607.

(re-landing with dependent patches)

Hopefully I got it right this time.

Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

Please request uplift ASAP, today is the last 85 beta build.

Flags: needinfo?(sphink)

Comment on attachment 9194616 [details]
Bug 1682068 - Fix rooting hazard in ImageBitmap::CreateInternal by avoiding movable data

Beta/Release Uplift Approval Request

  • User impact if declined: rare crashes (this has been hit in automation), UAF, very difficult to exploit
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1684123, Bug 1686532
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Main risk would be a perf regression, which I expect will be very small.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: (see above)
  • Fix Landed on Version: 86
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): (see above)
  • String or UUID changes made by this patch: none
Flags: needinfo?(sphink)
Attachment #9194616 - Flags: approval-mozilla-esr78?
Attachment #9194616 - Flags: approval-mozilla-beta?

Comment on attachment 9194616 [details]
Bug 1682068 - Fix rooting hazard in ImageBitmap::CreateInternal by avoiding movable data

Approved for 85.0b9 and 78.7esr.

Attachment #9194616 - Flags: approval-mozilla-esr78?
Attachment #9194616 - Flags: approval-mozilla-esr78+
Attachment #9194616 - Flags: approval-mozilla-beta?
Attachment #9194616 - Flags: approval-mozilla-beta+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [adv-main85+r]
Whiteboard: [adv-main85+r] → [adv-main85+r][adv-esr78.7+r]

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.

Flags: needinfo?(sphink)
Whiteboard: [adv-main85+r][adv-esr78.7+r] → [adv-main85+r][adv-esr78.7+r][sec-survey]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #15)

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.

Done.

Flags: needinfo?(sphink)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.