Closed Bug 1808632 (CVE-2023-25733) Opened 2 years ago Closed 2 years ago

Potential null pointer dereference in TaskbarPreviewCallback::Done() caused by failure to check return value

Categories

(Core :: Widget: Win32, defect, P2)

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
110 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox108 --- wontfix
firefox109 --- wontfix
firefox110 --- fixed

People

(Reporter: mozillabugs, Assigned: rkraesig)

Details

(Keywords: csectype-undefined, reporter-external, sec-low, Whiteboard: [win:stability][adv-main110+])

Attachments

(2 files)

TaskbarPreviewCallback::Done() (widget/windows/TaskbarPreview.cpp) doesn't check the return value from gfx::SourceSurfaceSkia::Map(). I think this call cannot actually fail, but if it did, later lines would dereference a null pointer. The bug wouldn't cause use of an uninitialized pointer because the pointer in question is contained in a gfx::DataSourceSurface::MappedSurface, which uses default member initializers (yay C++11!). The bug is on line 359 (trunk):

321: NS_IMETHODIMP
322: TaskbarPreviewCallback::Done(nsISupports* aCanvas, bool aDrawBorder) {
...
358:   gfx::DataSourceSurface::MappedSurface sourceMap;
359:   srcSurface->Map(gfx::DataSourceSurface::READ, &sourceMap);
360:   mozilla::gfx::CopySurfaceDataToPackedArray(
361:       sourceMap.mData, imageSurface->Data(), srcSurface->GetSize(),
362:       sourceMap.mStride, BytesPerPixel(srcSurface->GetFormat()));
363:   srcSurface->Unmap();

I have filed this as a security bug in case I missed something that would cause use of an uninitialized pointer in some circumstance.

Group: core-security → gfx-core-security

Ray, could you take a look at this and prioritize?

Severity: -- → S2
Flags: needinfo?(rkraesig)
Priority: -- → P2
Whiteboard: [win:stability]

Properly test for and handle errors in target-surface creation and
mapping.

(Additionally, perform some drive-by cleanup/modernization.)

Assignee: nobody → rkraesig
Status: NEW → ASSIGNED

I also don't think that call could actually fail.

If it did, though, I do see a way to get uninitialized pointer use out of it: given enough inlining and constraint propagation during LTO, it's possible that the compiler could see that this pointer is accessed unconditionally, and so elide the initialization to NULL on the theory that if it doesn't get set elsewhere it's undefined behavior anyway. (I think it's very unlikely that any compiler would actually do enough LTO to see that, but let's not find out the hard way.)

Adding :cmartin for review purposes.

Flags: needinfo?(rkraesig)

Comment on attachment 9310950 [details]
Bug 1808632 - TaskbarPreviewCallback::Done(): modernization and error-checking r?cmartin

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: As noted in the bug comments, I don't think it's actually possible.
  • 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?: trivial (probably identical patch)
  • How likely is this patch to cause regressions; how much testing does it need?: Respectively: a) not very; b) "I ran it locally and exercised the code and it didn't crash".
  • Is Android affected?: No
Attachment #9310950 - Flags: sec-approval?

Given your comments, I'll mark this sec-low, and then it doesn't need sec-approval. Thanks for the quick investigation and fix.

Attachment #9310950 - Flags: sec-approval?
Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
Whiteboard: [win:stability] → [win:stability][adv-main110+]
Alias: CVE-2023-25733
Group: core-security-release

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: