Closed Bug 1506665 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-use-after-free [@ mozilla::layers::CopyableCanvasRenderer::Initialize] with READ of size 8

Categories

(Core :: Canvas: WebGL, defect)

x86_64
Windows
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla67
Tracking Status
firefox-esr60 66+ verified
firefox64 --- wontfix
firefox65 + wontfix
firefox66 + verified
firefox67 + verified

People

(Reporter: decoder, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main66+][adv-esr60.6+])

Attachments

(7 files, 1 obsolete file)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 65.0a1-20181102220121-https://hg.mozilla.org/mozilla-central/rev/e05552012e193e1effac4c01dd149c6e6b51f30b.

For detailed crash information, see attachment.
Flags: sec-bounty?
Jeff, could you suggest someone who should take a look here?
Flags: needinfo?(jgilbert)
Group: core-security → gfx-core-security
I feel like we've hit this before.
I think this is bug 1446853.
Flags: needinfo?(jgilbert)
See Also: → 1446853
Attached file testcase.html
(In reply to Aral Yaman from comment #5)
> Created attachment 9025361 [details]
> testcase.html

Thanks a lot for providing the testcase! Unfortunately, it seems like we have this already on file, including a testcase, in bug 1446853. I very much appreciate your efforts though to help resolve this bug :)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1446853
Is this really a dupe? The use-after-free is detected at the same point, and the object was created the same, but the free-ing stack is nothing alike. Maybe this should be a "depends on" at least, if not entirely separate.

(Also the testcase here is simpler than the one in bug 1446853 and might make debugging easier.)
Flags: needinfo?(choller)
Forwarding this question to :jgilbert. Jeff, is it expected that the "free'd" stack here is different to the one in bug 1446853 or could this still be a different bug?
Status: RESOLVED → REOPENED
Flags: needinfo?(choller) → needinfo?(jgilbert)
Resolution: DUPLICATE → ---
We can leave this open and see, for now.
Flags: needinfo?(jgilbert)
Assignee: nobody → jgilbert
Any news on this issue ?
I have no access to Bug 1446853 (https://bugzilla.mozilla.org/show_bug.cgi?id=1446853) so for me it is not possible to know if this issue is a dupe or not ...
I tested the attached testcase.html today and it still crashes the latest Firefox Nightly 65.0a1 (2018-11-27) (64-bit).
Thanks!
Are there any activities on this bug ?
Hi 

The attached testcase.html still crashes the latest Firefox Nightly 66.0a1 (2018-12-13) (64-bit)
You may have to enamble Pageheap for reproducing the crash!
Jeff, what's the status here?
Flags: needinfo?(jgilbert)

I tested the testcase.html againg on my Windows 10 PC and it still crashes the latest Firefox Nightly 66.0a1 (2019-01-07)(64-bit).
I also submitted the crash report:
https://crash-stats.mozilla.org/report/index/3de995cc-66f0-4778-86b4-a4d7d0190108

Is this bug not going to be fixed ?

Will take a quick stab tomorrow.

Flags: needinfo?(jgilbert)

This doesn't seem to crash without PageHeap enabled: I haven't installed it, and I can't repro with the attached testcase.

Ok, I bet I see it:
https://hg.mozilla.org/mozilla-central/annotate/aac6849c6ff0eb4981b569988a094f04e42f14dd/dom/canvas/WebGLContext.cpp#l1195

CanvasInitializeData data;
...
data.mGLContext = gl;
data.mSize = DrawingBufferSize(); // This can destroy gl!
...
aRenderer->Initialize(data); // Tries to call data.mGLContext->AddRef(), UAF.

Comment on attachment 9035833 [details]
Bug 1506665 - CanvasInitializeData should use RefPtrs. -

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Relatively hard.

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?: 59+

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?: Very easy.

How likely is this patch to cause regressions; how much testing does it need?: Very unlikely to cause regressions.

Attachment #9035833 - Flags: sec-approval?

I tested the testcase.html with the firefox version inside target.zip an it still crashes...

Flags: needinfo?(aral.yaman)
Flags: needinfo?(jgilbert)

Well, setting pageheap on globally makes my machine fail to start.

Flags: needinfo?(jgilbert)

(In reply to Jeff Gilbert [:jgilbert] from comment #25)

Well, setting pageheap on globally makes my machine fail to start.

I am not sure what the need for pageheap here is, we do have ASan builds for Windows and Linux and they can both also be built on try or locally. If you need help doing so, please let me know :)

Flags: needinfo?(jgilbert)

I never have had a smooth experience with getting asan builds running, and building takes a while, so trying to get pageheap running to trigger the failure (as reported) was worth trying.

However, I can't get that to work, so now I'll try with asan builds.

Flags: needinfo?(jgilbert)

Comment on attachment 9035833 [details]
Bug 1506665 - CanvasInitializeData should use RefPtrs. -

Clearing sec-approval request until we know that the patch fixes the issue.

Attachment #9035833 - Flags: sec-approval?

This isn't going to make Fx65 at this point with the RC coming next week :(

Any News about this bug ?
Firefox Nightly 67.0a1 (2019-01-31) (64-bit) still crashes.

(In reply to Aral Yaman from comment #24)

I tested the testcase.html with the firefox version inside target.zip an it still crashes...

Can you give me a crash report or at least a crash stack for where this crashes?

No one else can seem to reliably reproduce this.

Flags: needinfo?(aral.yaman)

I posted already a crash report:
https://crash-stats.mozilla.org/report/index/3de995cc-66f0-4778-86b4-a4d7d0190108

To reproduce the crash it is important that page heap is enabled.

Hope this helps otherwise let me know if I should provode more information

Flags: needinfo?(aral.yaman)

That's not the crash report with target.zip from the link I posted, is it?

I cannot enable page heap, so you need to do this test for me.

Flags: needinfo?(aral.yaman)

No it was a previous crash report.

Now I submitted a crash report from target.zip
See here: https://crash-stats.mozilla.org/report/index/a2f8a6d1-80f2-4b5f-9681-7f2500190201

Flags: needinfo?(aral.yaman)

The crash report looks not very useful so I tried to get some more information in WinDbg and attached the log file firefox-debug_3c78_2019-02-01_08-05-19-073.log

But I think I have a problem for getting sybol files.

Do you think this log can help you more or could you give me an instruction how I can get the symbol file information ?

I followed the instructions from here: https://developer.mozilla.org/en-US/docs/Mozilla/How_to_get_a_stacktrace_with_WinDbg

Flags: needinfo?(jgilbert)

I could reliably reproduce the crash like the following STR on my Win10 pc.

[1] Open attachment 9025361 [details]
[2] Reload the tab.
[3] Move to another tab.
[4] Move back to the tab of attachment 9025361 [details]

Repeat [2]-[4] if the crash did not happen.

I could reproduce the crash with local build. I am going to look into it tomorrow.

When the crash happened, WebGLContext::InitializeCanvasRenderer() destroyed GLContextEGL under WebGLContext::DrawingBufferSize(). The DrawingBufferSize() calls WebGLContext::EnsureDefaultFB(), but it failed to allocate backbuffer. Then GLContextEGL was destroyed.

Before calling the DrawingBufferSize(), CanvasInitializeData::mGLContext stored raw pointer. Then DrawingBufferSize() call destroyed the GLContextEGL, and mGLContext became stale pointer.

https://searchfox.org/mozilla-central/rev/b5299cdebb16d39a027d7bd782dafc2ad64d0bad/dom/canvas/WebGLContext.cpp#1262

Assignee: jgilbert → sotaro.ikeda.g
Flags: needinfo?(jgilbert)

When WebGLContext::SetDimensions() was called, if mRequestedSize was different from size of DefaultFB and GLContext already existed, the SetDimensions() just cleared mDefaultFB, the mDefaultFB was allocaed again when it became necessary next time. Then DrawingBufferSize() tried to allocate DefaultFB in WebGLContext::InitializeCanvasRenderer() as in Comment 39.

I confirmed that attachment 9043509 [details] addressed the problem of comment 0 and comment 1. Though there were still crash that were caused by oom. The oom crash is not related to this bug.

Crash happened with attachment 9035833 [details].

Crash happened in ShareableCanvasRenderer::Initialize(). Null pointer access happened since gl::GLScreenBuffer* screen = mGLContext->Screen() was nullptr. mGLContext was destroyed because of EnsureDefaultFB() failure, it caused the nullptr.

Ok, cool!
Can you fix that too?

I would rather update our layers code to handle a destroyed GLContext, rather than patching just the issue in ShareableCanvasRenderer::Initialize().

(In reply to Jeff Gilbert [:jgilbert] from comment #44)

I would rather update our layers code to handle a destroyed GLContext, rather than patching just the issue in ShareableCanvasRenderer::Initialize().

Yea, patching in ShareableCanvasRenderer::Initialize() is not good. I am going to update the patch, but it should not happen by change of WebGLContext::InitializeCanvasRenderer().

Duplicate of this bug: 1527664

Are we still waiting on an updated patch?

Flags: needinfo?(sotaro.ikeda.g)

I already updated the patch. Can you comment to the patch?

Flags: needinfo?(sotaro.ikeda.g)
Attachment #9043509 - Attachment description: Bug 1506665 - Check if EnsureDefaultFB() succeeds in WebGLContext::InitializeCanvasRenderer() → Bug 1506665 - Handle GLContext failure
Attachment #9043509 - Attachment description: Bug 1506665 - Handle GLContext failure → Bug 1506665 - Handle GLContext failure by large sized webgl canvas

attachment 9043509 [details] also addressed content process crash.

Comment on attachment 9043509 [details]
Bug 1506665 - Add more GLContext failure handling

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: It seems not easy, but it might be possible to construct.
  • 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?: Firefox 59
  • If not all supported branches, which bug introduced the flaw?: Bug 1427668
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: It is not difficult. Risk is same to the current path.
  • How likely is this patch to cause regressions; how much testing does it need?: I locally tested with Bug 1506665 Comment 37. The patch addressed Tab crashes for me. There might still exist a route to cause nullptr GLContext access.
    Testing with Bug 1506665 Comment 37 seems necessary.
Attachment #9043509 - Attachment description: Bug 1506665 - Handle GLContext failure by large sized webgl canvas → Bug 1506665 - Check if EnsureDefaultFB() succeeds in WebGLContext::InitializeCanvasRenderer()
Attachment #9043509 - Flags: sec-approval?
Attachment #9043509 - Attachment description: Bug 1506665 - Check if EnsureDefaultFB() succeeds in WebGLContext::InitializeCanvasRenderer() → Bug 1506665 - Add more GLContext failure
Attachment #9043509 - Attachment description: Bug 1506665 - Add more GLContext failure → Bug 1506665 - Add more GLContext failure handling
Blocks: 1427668
Attachment #9035833 - Attachment is obsolete: true

Comment on attachment 9043509 [details]
Bug 1506665 - Add more GLContext failure handling

sec-approval+ for trunk. We'll want this patched on all affected branches so we'll want Beta and ESR60 patches made and nominated.

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

On beta, patch for m-c(attachment 9043509 [details]) could be applied. attachment 9046963 [details] [diff] [review] is for esr60.

Comment on attachment 9043509 [details]
Bug 1506665 - Add more GLContext failure handling

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1427668
  • User impact if declined: Tab could crash in a STR like Bug 1506665 Comment 37.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: The step is in Bug 1506665 Comment 37.
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The patch fixes UAF bug. But there might still exist a route to cause nullptr GLContext access. Though I addressed some nullptr GLContext accesses.

I nominate the patch to Beta and ESR60 before landing to m-c because it was requested by Bug 1506665 Comment 53.

  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Tab could crash in a STR like Bug 1506665 Comment 37.
  • User impact if declined: Tab could crash in a STR like Bug 1506665 Comment 37.
  • Fix Landed on Version: Not yet landed to m-c
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The patch fixes UAF bug. But there might still exist a route to cause nullptr GLContext access. Though I addressed some nullptr GLContext accesses.

I nominate the patch to Beta and ESR60 before landing to m-c because it was requested by Bug 1506665 Comment 53.

  • String or UUID changes made by this patch: None
Attachment #9043509 - Flags: approval-mozilla-esr60?
Attachment #9043509 - Flags: approval-mozilla-beta?

Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmps6sb8i\npatching file gfx/layers/CanvasRenderer.h\nHunk #1 FAILED at 27\n1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/CanvasRenderer.h.rej\nabort: patch failed to apply', '')

Flags: needinfo?(sotaro.ikeda.g)

It was caused by Bug 1530471. I am going to update the patch.

Attachment #9043509 - Attachment description: Bug 1506665 - Add more GLContext failure handling → Bug 1506665 - Add more GLContext failure handling

I updated the patch.

Flags: needinfo?(sotaro.ikeda.g)
Group: gfx-core-security → core-security-release
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
QA Whiteboard: [qa-triaged]

Issue was reproduced with Nightly 67.0a1 (2019-01-31)(64-bit) on Windows 10 Pro x64.
Verified as fixed with Nightly 67.oa1 (2019-03-03).

Status: RESOLVED → VERIFIED

Awarding a bug bounty for this report.

Aral: can you confirm that this fixes the problem for you? This patch should be in ASAN nightlies now.

Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(aral.yaman)

I tested with the latest Nightly 67.0a1 (2019-03-04) and can confirm that it does not crash anymore!

Flags: needinfo?(aral.yaman)

Comment on attachment 9043509 [details]
Bug 1506665 - Add more GLContext failure handling

Fixes a WebGL sec issue. Verified by QA and the reporter. Approved for 66.0b14 and 60.6esr.

Attachment #9043509 - Attachment description: Bug 1506665 - Add more GLContext failure handling → Bug 1506665 - Add more GLContext failure handling
Attachment #9043509 - Flags: approval-mozilla-esr60?
Attachment #9043509 - Flags: approval-mozilla-beta?
Attachment #9047877 - Flags: approval-mozilla-beta+
Attachment #9046963 - Flags: approval-mozilla-esr60+

Verified the issue as fixed with 66.0b14 on Windows 10x64.

Whiteboard: [adv-main66+][adv-esr60.6+]

The issue is verified as fixed with 60.6esr on Windows 10x64.

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