Undefined behavior in CompositorD3D11::EndFrame is latent bug

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: q1, Assigned: jnicol)

Tracking

({sec-other})

39 Branch
mozilla43
Unspecified
Windows
sec-other
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main43-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
CompositorD3D11::EndFrame (\gfx\layers\d3d11\CompositorD3D11.cpp) invokes undefined behavior by dereferencing a past-the-end iterator to an std::vector. At present, this seems to have no ill effects, but nothing guarantees that that situation will continue:

1150:      params.DirtyRectsCount = mInvalidRegion.GetNumRects();
1151:      std::vector<RECT> rects;
1152:      rects.reserve(params.DirtyRectsCount);
1153:
1154:      nsIntRegionRectIterator iter(mInvalidRegion);
1155:      const nsIntRect* r;
1156:      uint32_t i = 0;
1157:      while ((r = iter.Next()) != nullptr) {
1158:        RECT rect;
1159:        rect.left = r->x;
1160:        rect.top = r->y;
1161         rect.bottom = r->YMost();
1162:        rect.right = r->XMost();
1163:
1164:        rects.push_back(rect);
1165:      }
1166:
1167:      params.pDirtyRects = &rects.front();
1168:      chain->Present1(presentInterval, mDisableSequenceForNextFrame ? DXGI_PRESENT_DO_NOT_SEQUENCE : 0, &params);

The problem is that params.DirtyRectsCount sometimes == 0. This occurs when, for example, creating a new window. (You can verify this by setting a conditional breakpoint on line 1150, then pressing ctrl/n). Control passes to line 1167, which takes &rects.front(). But rects.front() is undefined here, because it's the same as *rects.begin(), and rects.begin()==rects.end().

In the STL implementation that gets built into FF 39.0 when building with VS2012, the expression |&rects.front()| happens to have the value NULL, which is coincidentally appropriate for the call to IDXGISwapChain1::Present1 on line 1168. However, MS specifically calls out vector::front() as undefined for an empty vector. https://msdn.microsoft.com/en-us/library/0z70c7a5.aspx .
Assignee: nobody → jnicol
OS: Unspecified → Windows
(Reporter)

Comment 1

3 years ago
Just to clarify, |container.front()| for an empty container is undefined behavior in all STL implementations, since it dereferences the non-dereferenceable iterator |container.end()|. I used the example of Windows because I've built that version of FF.
Keywords: sec-other
(Assignee)

Comment 2

3 years ago
Created attachment 8647020 [details] [diff] [review]
Patch v1

Only set params.pDirtyRects if the vector is not empty. If the vector is empty then params.pDirtyRects will be null due to the PodZero(&params) call above.

Also might as well use vector::data() instead of vector::front().
(Assignee)

Updated

3 years ago
Attachment #8647020 - Flags: review?(bgirard)
Comment on attachment 8647020 [details] [diff] [review]
Patch v1

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1108,5 @@
>        }
>  
> +      if (!rects.empty()) {
> +        params.pDirtyRects = rects.data();
> +      }

Looks good, personally I would prefer to have |else { params.pDirtyRects = nullptr; }|. It would be easy to forget this if we removed the PodZero call above.
Attachment #8647020 - Flags: review?(bgirard) → review+
(Assignee)

Comment 4

3 years ago
Created attachment 8647474 [details] [diff] [review]
Patch v2

Explicitly set to nullptr as per Benoit's suggestion.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6df50c3803a

I'm not sure what the next steps are here. Usually I'd set checkin-needed but Milan said this needs a security review first? Is there a flag I set for that?
(Assignee)

Updated

3 years ago
Attachment #8647020 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Comment on attachment 8647474 [details] [diff] [review]
Patch v2

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not easily

> 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?

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They will be the same patch, possibly need to be adjusted for line numbers, but that will be trivial.

> How likely is this patch to cause regressions; how much testing does it need?

Not likely at all.
Attachment #8647474 - Flags: sec-approval?
Comment on attachment 8647474 [details] [diff] [review]
Patch v2

As a sec-other, this doesn't need sec-approval. Just check it into trunk.
Attachment #8647474 - Flags: sec-approval?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 8

3 years ago
Created attachment 8649281 [details] [diff] [review]
Patch v3

The commit that caused the conflict with this also fixed the problem - we no longer even use std::vector here.

We could still explicity set pDirtyRects to nullptr but I'm not sure this is necessary. The docs say "You can set this member to NULL if DirtyRectsCount is 0", not "you must...". - https://msdn.microsoft.com/en-us/library/windows/desktop/hh404522(v=vs.85).aspx

Benoit: what do you think? should we commit or abandon this?
Attachment #8649281 - Flags: review?(bgirard)
(Assignee)

Updated

3 years ago
Attachment #8647474 - Attachment is obsolete: true
Comment on attachment 8649281 [details] [diff] [review]
Patch v3

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

IMO I'd rather have this be nullptr than stack address. You've already written the patch anyways. But like you said I agree that it's probably not really needed.
Attachment #8649281 - Flags: review?(bgirard) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e2d43bdbaee
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main43-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.