Closed Bug 1533522 Opened 2 years ago Closed 1 year ago

WebGL assumes that GL_OUT_OF_MEMORY leaves resources at their previous sizes

Categories

(Core :: Canvas: WebGL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 68+ fixed
firefox-esr68 --- fixed
firefox67 --- wontfix
firefox68 + fixed
firefox69 + fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: csectype-disclosure, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main68+][adv-esr60.8+])

Attachments

(2 files)

This is definitely not what the spec guarantees.

GL_OUT_OF_MEMORY is allowed to basically ruin your day, but we assume that drivers aren't malicious, and try to recover as safely as possible. However, we should not assume that GL preserved the old resource while trying to resize it to the new size.

See Also: → CVE-2019-11693
Severity: normal → minor
Priority: -- → P1
Group: core-security → gfx-core-security
Keywords: sec-audit
Assignee: nobody → jgilbert

Probably sec-medium:
Drivers probably leave the old resource intact, but at least some drivers probably won't. To prevent data leaks (reading memory re-alloc'd elsewhere), we should forbid access into these resources just in case.

Flags: needinfo?(dveditz)

Comment on attachment 9072075 [details]
Bug 1533522 - Truncate Buffer/Texture on GL_OOM.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: sec-vector would be medium-difficulty, actual exploit would be harder, and is unlikely to be reliable.
  • 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?: 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?: This should rebase cleanly.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause bad regressions.
Attachment #9072075 - Flags: sec-approval?

Comment on attachment 9072075 [details]
Bug 1533522 - Truncate Buffer/Texture on GL_OOM.

This doesn't need sec-approval to land (just criticals and highs do) so please feel free to land it.

Attachment #9072075 - Flags: sec-approval?
Type: enhancement → defect
Flags: needinfo?(dveditz)
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Please nominate this for Beta approval when you get a chance. Not sure if it's worth taking on ESR60 given the severity and where we are in its life cycle, but feel free to nominate it for there too if you think it's something we should consider taking.

Flags: needinfo?(jgilbert)

I want to let this bake for a bit first.

We're about to build 68 rc, is this ready for uplift now or should it ride the trains to 69?

I think we're good!

Flags: needinfo?(jgilbert)

Comment on attachment 9072075 [details]
Bug 1533522 - Truncate Buffer/Texture on GL_OOM.

Beta/Release Uplift Approval Request

  • User impact if declined: sec-moderate, but pretty straight-forward patch.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk, pretty simple patch.
  • String changes made/needed: none
Attachment #9072075 - Flags: approval-mozilla-beta?

Comment on attachment 9072075 [details]
Bug 1533522 - Truncate Buffer/Texture on GL_OOM.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate, but simple patch.
  • User impact if declined: There miiight be a way to build an exploit if there's a driver which frees the old resource memory before failing to allocate the new resource.
  • Fix Landed on Version: 69
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk, simple patch, simple fix. This code is unlikely to run in most situations anyway.
  • String or UUID changes made by this patch: none
Attachment #9072075 - Flags: approval-mozilla-esr60?

Comment on attachment 9072075 [details]
Bug 1533522 - Truncate Buffer/Texture on GL_OOM.

webgl sec fix, approved for 68 rc1

Attachment #9072075 - Flags: approval-mozilla-release+
Attachment #9072075 - Flags: approval-mozilla-esr68+
Attachment #9072075 - Flags: approval-mozilla-beta?

Comment on attachment 9072075 [details]
Bug 1533522 - Truncate Buffer/Texture on GL_OOM.

Approved for 60.8esr also.

Attachment #9072075 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main68+][adv-esr60.8+]

Sorry if I'm missing something obvious but it seems the esr-60 port has a bug:

https://hg.mozilla.org/releases/mozilla-esr60/rev/d815b20659daed17b1783c38a1f136222469d963#l2.12

/dom/canvas/WebGLRenderbuffer.cpp

+    if (error != LOCAL_GL_OUT_OF_MEMORY) {
+      // Truncate.
+      mSamples = 0;
+      mFormat = nullptr;
+      mWidth = 0;
+      mHeight = 0;
+      mImageDataStatus = WebGLImageDataStatus::NoImageData;
+
+      InvalidateStatusOfAttachedFBs(funcName);
+    }

You're truncating if the error is NOT an OOM? Comparable patches for the other branches truncate when it IS an OOM there.

Flags: needinfo?(jgilbert)

Yeah, that's wrong, oops. Thanks!

Flags: needinfo?(jgilbert)

Comment on attachment 9081078 [details]
Bug 1533522 - Truncate Renderbuffer on GL_OOM.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: The backport patch got this conditional backwards.
  • User impact if declined: On unexpected error (just about never), it'll truncate the buffer (probably fine), instead of on OUT_OF_MEMORY, which was the goal of this bug.
  • Fix Landed on Version: 69
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Very low risk, see "User impact".
  • String or UUID changes made by this patch: none
Attachment #9081078 - Flags: approval-mozilla-esr60?

Comment on attachment 9081078 [details]
Bug 1533522 - Truncate Renderbuffer on GL_OOM.

Follow-up fix for ESR60 to correct a mistake made during rebasing which causes the landed patch to have reversed logic. Approved for 60.9esr.

Attachment #9081078 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Thanks!

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