<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Assignee

Comment 4

•

1 year ago

Yup, looking into this.

Flags: needinfo?(eitan)

Assignee

Comment 5

•

1 year ago

area[shape=poly] provides a flattened list of x/y coordinates in the coords attribute. We are hitting this because the number of coordinates provided is uneven and we have no fail-safe for that. Although this is a layout bug, a11y seems to be the sole consumer of nsImageMap::GetBoundsForAreaContent which triggers this.

Assignee

Updated

•

1 year ago

Assignee: nobody → eitan

Assignee

Comment 6

•

1 year ago

Attached file Bug 1878211 - Process pairs in coord list in PolyArea. r?dholbert — Details

Assignee

Updated

•

1 year ago

Component: Disability Access APIs → Layout: Images, Video, and HTML Frames

Andrew McCreight [:mccr8]

Comment 7

•

1 year ago

Looking in pernosco and the patch, it looks like the issue here is a mismatch between the counts here where we allocate precisely mNumCoords entries in mCoords:
https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/layout/generic/nsImageMap.cpp#181,216-217

UniquePtr<nscoord[]> value_list = MakeUnique<nscoord[]>(cnt);
...
mNumCoords = cnt;
mCoords = std::move(value_list);

...vs. here where the i+1 indexing operation potentially is an access of mCoords[mNumCoords], just off the end of the array, if mNumCoords is odd:
https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/layout/generic/nsImageMap.cpp#491,496-498

void PolyArea::GetRect(nsIFrame* aFrame, nsRect& aRect) {
...
    for (int32_t i = 2; i < mNumCoords; i += 2) {
      xtmp = nsPresContext::CSSPixelsToAppUnits(mCoords[i]);
      ytmp = nsPresContext::CSSPixelsToAppUnits(mCoords[i + 1]);

Comment 8

•

1 year ago

Any chance we could change mCoords to be an nsTArray which is bounds checked, here or in a followup? It doesn't look like there are TOO many uses of nsCoord[] so maybe all of that could be cleaned up. I don't know if this is performance sensitive enough for those bounds checks to matter.

Comment 9

•

1 year ago

This code here is definitely not performance-sensitive where a nsTArray bounds-check would matter.

That conversion makes sense; let's do that in a followup, so that the patch here can be trivial and hence uplifted confidently (I suspect we'll want to land this on all branches -- including ESR -- at once, when we're at a good time for sec-bug-landing).

Phabricator Automation

Updated

•

1 year ago

Attachment #9378352 - Attachment description: Bug 1878211 - Round down uneven coord attribute for PolyArea areas. r?dholbert → Bug 1878211 - Round down uneven coord list when processing PolyArea. r?dholbert

Ryan VanderMeulen [:RyanVM]

Updated

•

1 year ago

status-firefox122: --- → affected

status-firefox123: --- → affected

status-firefox-esr115: --- → affected

Updated

•

1 year ago

status-firefox122: affected → wontfix

tracking-firefox123: --- → +

tracking-firefox124: --- → +

tracking-firefox-esr115: --- → 123+

Phabricator Automation

Updated

•

1 year ago

Attachment #9378352 - Attachment description: Bug 1878211 - Round down uneven coord list when processing PolyArea. r?dholbert → Bug 1878211 - Process pairs in coord list in PolyArea. r?dholbert

Assignee

Comment 10

•

1 year ago

Comment on attachment 9378352 [details]
Bug 1878211 - Process pairs in coord list in PolyArea. r?dholbert

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not hard. A web author just needs to pass an uneven coord list and wait for someone with a11y enabled.
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?: easy
How likely is this patch to cause regressions; how much testing does it need?: this is a very simple patch
Is Android affected?: Yes

Attachment #9378352 - Flags: sec-approval?

Comment 11

•

1 year ago

(In reply to Eitan Isaacson [:eeejay] from comment #10)

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: easy

slightly more detail on this^: the existing patch applies perfectly to ESR115 (I just checked), so the patch should uplift trivially with no manual rebasing needed.

(This code hasn't changed much in ages.)

Comment 12

•

1 year ago

•

Edited

Attached file testcase 1 (crashes ASAN builds) — Details

This testcase crashes in ASAN builds, if you:
(1) run the build with environmental variable GNOME_ACCESSIBILITY=1
...or:
(2) Prompt the testcase to draw a focus ring by pressing "Tab" after you load it

These two different approaches are crashing via the two different codepaths that are fixed in eeejay's patch here.

Comment 13

•

1 year ago

•

Edited

(In reply to Daniel Holbert [:dholbert] from comment #12)

(2) Prompt the testcase to draw a focus ring by pressing "Tab" after you load it

Note, I tried to automate the focus-ring action in this testcase, but that apparently doesn't work due to another bug with our image map handling. I filed bug 1878753 on that.

(With that fixed, we could create a better automated test for this sec issue here. In the meantime, an automated testcase here would probably need to use SpecialPowers or EventUtils or somesuch to synthesize a Tab keyboard-event.)

[EDIT Feb 8th: Looks like bug 1878753 is being fixed rather quickly, so we can morph this into a relatively-simple automated test, when we're ready to land tests here. Hooray!]

Comment 14

•

1 year ago

•

Edited

I verified that eeejay's attached patch fixes the crash, testing a local ASAN debug+opt build on my attached testcase 1 with steps in comment 12.

In a stock ASAN debug+opt build, both approaches (1) and (2) from comment 12 instantly crash for me, with this fatal asan error:

==384424==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x504000113e34 at pc 0x7f39116ae4e8 bp 0x7ffdec65ac50 sp 0x7ffdec65ac48
READ of size 4 at 0x504000113e34 thread T0 (file:// Content)

When I apply eeejay's patch and rebuild and repeat the test, I don't crash.

Updated

•

1 year ago

Keywords: testcase

Tom Ritter [:tjr]

Comment 15

•

1 year ago

Comment on attachment 9378352 [details]
Bug 1878211 - Process pairs in coord list in PolyArea. r?dholbert

Approved to request uplift and land

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

Pulsebot

Comment 16

•

1 year ago

Pushed by rvandermeulen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/52b668d19d28 Process pairs in coord list in PolyArea. r=dholbert

Comment 17

•

1 year ago

[ni=eeejay as a reminder to request uplift soonish]

Flags: needinfo?(eitan)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Assignee

Comment 18

•

1 year ago

Keeping the NI to remember. I'm going to wait 24 hours after this lands in Central and we didn't break precious image maps for everyone.

Comment 19

•

1 year ago

https://hg.mozilla.org/mozilla-central/rev/52b668d19d28

Group: layout-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 1 year ago

status-firefox124: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 124 Branch

Comment 20

•

1 year ago

There was no uplift request for beta/release/esr and we are in RC week, can it wait for 124 / esr 115.9 or should I already plan a RC2 for an uplift? The patch grafts cleanly to mozilla-release and esr.

Assignee

Comment 21

•

1 year ago

Comment on attachment 9378352 [details]
Bug 1878211 - Process pairs in coord list in PolyArea. r?dholbert

Beta/Release Uplift Approval Request

User impact if declined: Potential UAF in badly authored HTML.
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: Yes
If yes, steps to reproduce: Execute attached test case in comment #12.
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): It is a very small change that just assures an array is even.
String changes made/needed:
Is Android affected?: Yes

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Potential UAF in badly authored HTML.
Fix Landed on Version: 124
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): It is a very small change that just assures an array is even.

Flags: needinfo?(eitan)

Attachment #9378352 - Flags: approval-mozilla-esr115?

Attachment #9378352 - Flags: approval-mozilla-beta?

Assignee

Updated

•

1 year ago

Flags: qe-verify+

https://hg.mozilla.org/releases/mozilla-release/rev/d01773ef60f2

Comment 22

•

1 year ago

Comment on attachment 9378352 [details]
Bug 1878211 - Process pairs in coord list in PolyArea. r?dholbert

Approved for 123.0RC + ESR, thanks

Attachment #9378352 - Flags: approval-mozilla-release+

Attachment #9378352 - Flags: approval-mozilla-esr115?

Attachment #9378352 - Flags: approval-mozilla-esr115+

Attachment #9378352 - Flags: approval-mozilla-beta?

Pulsebot

Comment 23

•

1 year ago

uplift

https://hg.mozilla.org/releases/mozilla-esr115/rev/17fcbdf64266

Updated

•

1 year ago

status-firefox123: affected → fixed

Pulsebot

Comment 24

•

1 year ago

uplift