Heap buffer over read [@nsTextFrame::GetRenderedText]

RESOLVED FIXED in Firefox -esr52

Status

()

P2
critical
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: tsmith, Assigned: dholbert)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla60
crash, csectype-bounds, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr5259+ fixed, firefox53 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59+ fixed, firefox60+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main59+][adv-esr52.7+])

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8819349 [details]
test_case.html

The attached test case does not consistently reproduce the issue.

==63475==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000753a94 at pc 0x7ff599af39a1 bp 0x7ffcd30f3470 sp 0x7ffcd30f3468
READ of size 1 at 0x602000753a94 thread T0
    #0 0x7ff599af39a0 in CharAt /home/worker/workspace/build/src/dom/base/nsTextFragment.h:204:68
    #1 0x7ff599af39a0 in nsTextFrame::GetRenderedText(unsigned int, unsigned int, nsIFrame::TextOffsetType, nsIFrame::TrailingWhitespace) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:9917
    #2 0x7ff59a8203dc in RenderedToContentOffset /home/worker/workspace/build/src/accessible/generic/HyperTextAccessible.cpp:2004:33
    #3 0x7ff59a8203dc in mozilla::a11y::HyperTextAccessible::GetBoundsInFrame(nsIFrame*, unsigned int, unsigned int) /home/worker/workspace/build/src/accessible/generic/HyperTextAccessible.cpp:108
    #4 0x7ff59a82bced in mozilla::a11y::HyperTextAccessible::TextBounds(int, int, unsigned int) /home/worker/workspace/build/src/accessible/generic/HyperTextAccessible.cpp:1280:32
    #5 0x7ff59a755200 in getRangeExtentsCB(_AtkText*, int, int, AtkCoordType, _AtkTextRectangle*) /home/worker/workspace/build/src/accessible/atk/nsMaiInterfaceText.cpp:406:12
...
see log.txt
(Reporter)

Comment 1

2 years ago
Created attachment 8819350 [details]
log.txt
Created attachment 8819481 [details]
An unrelated shutdown crash with the same testcase

I can't reproduce the reported crash but I got this one instead when quitting
Firefox with 4 tabs running the attached testcase.
It looks like this might be an a11y issue?  (feel free to bounce it back
if it's a bug in Layout)
Component: Layout → Disability Access APIs
Did you have any luck reproducing this?
Flags: needinfo?(surkov.alexander)
(Reporter)

Updated

2 years ago
Keywords: sec-high
(In reply to Mats Palmgren (:mats) from comment #4)
> Did you have any luck reproducing this?

no luck with VoiceOver running, any particular steps I can follow?
Flags: needinfo?(surkov.alexander)
Tyson, is there any specific STR to follow?
Flags: needinfo?(twsmith)
(Reporter)

Comment 7

2 years ago
Created attachment 8826426 [details]
test_case.html
Attachment #8819349 - Attachment is obsolete: true
Flags: needinfo?(twsmith)
(Reporter)

Comment 8

2 years ago
I just updated the test case and it is much more reliable on my machine.

STR:
1) Launch screen reader
2) Launch test case

I am running Ubuntu 16.04 using an ASan opt build.

I hope that helps :)
Are your able to reproduce the issue with the updated test case?
Flags: needinfo?(surkov.alexander)
no luck on Ubuntu 16.04.1, .mozconfig:

ac_add_options --disable-optimize
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

orca screen reader is running
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #10)
> no luck on Ubuntu 16.04.1, .mozconfig:
> 
> ac_add_options --disable-optimize
> mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@
> 
> orca screen reader is running

That's not an ASAN config, is it? ASAN (or Valgrind) is often necessary to catch these subtle memory abuse bugs, short of spending a lot of time creating a testcase that's far closer to an exploit. Much better use of everyone's time to use sanitizer builds.
Flags: needinfo?(surkov.alexander)
ok, what lines should be added/removed to/from my config?
Flags: needinfo?(surkov.alexander)
Any luck testing with ASAN?
You can just download builds as linked in comment 13.
Flags: needinfo?(surkov.alexander)
no luck

* Ubuntu 16.04.1 LTS running in VMWareFusion
* debug [1] and optimized builds [2]: about Nightly -> 54.0a1 (2017-02-15) (64-bit)
* loading url [3], keeping running for a while, no crash

[1]https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-asan-debug/artifacts/public/build/target.tar.bz2
[2] https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2
[3] data:text/html,<script>function done(){try{document.body.bgColor='FEFFFE';}catch(e){}setTimeout(function(){window.location.reload()}, Math.random()*100);}setTimeout(done, Math.random()*200);</script><textarea autofocus readonly form='z'></textarea><meter contenteditable='true'></meter><style></style>A<s/>//
Flags: needinfo?(surkov.alexander)
universal access -> screen reader -> on of course before I start the browsers
(Reporter)

Comment 17

2 years ago
I verified I can reproduce this on the latest ASan opt build however it does seem less reliable. I test with e10s enabled and disabled and I could reproduce it in both cases but it was more reliable with e10s disabled.

Another trick I came across to make this more reliable was to specify the file on the command line when opening Firefox.
(Reporter)

Comment 18

2 years ago
Created attachment 8837700 [details]
test_case.html

v3 trying to increase reliability
Attachment #8826426 - Attachment is obsolete: true
No luck to reproduce it, I tried on debug asan build with e10s turned off to run the test from command line:
./firefox --new-tab file:///home/huh/asan/test.html

Anyway, it makes sense to get attention from layout folks. I realize it may be a bad timing to poke a frame tree and that's probably the case, but I'm not sure whether this kind of query just shouldn't crash or a11y has to make sure to not call at bad timing at all, or both.
(Reporter)

Comment 20

2 years ago
(In reply to alexander :surkov from comment #19)
> No luck to reproduce it, I tried on debug asan build with e10s turned off to
> run the test from command line:
> ./firefox --new-tab file:///home/huh/asan/test.html

Be sure the browser is closed before running that command and use an ASan opt build (not debug)
> * Ubuntu 16.04.1 LTS running in VMWareFusion

You might also want to try a non-WM installation, in case that makes a difference.
That also enables you to use 'rr' more effectively to debug it once you have
a crash.
Assigning to Alexander to continue testing, as suggested by Tyson and Mats in comment 20 and comment 21.
Assignee: nobody → surkov.alexander
Alex any luck?
Flags: needinfo?(surkov.alexander)
(In reply to David Bolter [:davidb] from comment #23)
> Alex any luck?

nope, it seems I keep running into bug 1333715 for ASAN build.
Flags: needinfo?(surkov.alexander)
Trevor do you think you could catch this in rr?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Daniel Veditz [:dveditz] from comment #11)
> (In reply to alexander :surkov from comment #10)
> > no luck on Ubuntu 16.04.1, .mozconfig:
> > 
> > ac_add_options --disable-optimize
> > mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@
> > 
> > orca screen reader is running
> 
> That's not an ASAN config, is it? ASAN (or Valgrind) is often necessary to
> catch these subtle memory abuse bugs, short of spending a lot of time
> creating a testcase that's far closer to an exploit. Much better use of
> everyone's time to use sanitizer builds.

there is an assert to catch the oob read in nsTextFragment::CharAt() but its only a NS_ASSERTION() we should probably upgrade that to a stronger assert.
(In reply to David Bolter [:davidb] from comment #25)
> Trevor do you think you could catch this in rr?

yeah, it wasn't hard to catch this with rr.
Flags: needinfo?(tbsaunde+mozbugs)
Ok, so the problem is a11y calling GetRenderedText(1, 2, offsets in rendered text) on a text frame where mContent->mText is "A\n".  The stack is being called from ipc while waiting on a ipc message from the message manager sent by js, so I'm pretty sure this is an a11y bug.

I'm not really sure what's going on with GetRendered text, but it seems like its gotten trimmedOffsets and the iterator out of sync.  We first call TransformChars() for offsets 1, 2 in the text, but then we take a second trip through the loop at line 9957 because trimmedOffsets = { 1, 2} so GetEnd() returns 3, while iter.getOriginalOffsets() returns 2.
Flags: needinfo?(mats)
> because trimmedOffsets = { 1, 2} so GetEnd() returns 3

That doesn't seem right when the content is "A\n".
Why was the trimmedOffsets length set to 2?
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #28)
> > because trimmedOffsets = { 1, 2} so GetEnd() returns 3
> 
> That doesn't seem right when the content is "A\n".
> Why was the trimmedOffsets length set to 2?


That's a good question. Who's going to continue working on this? tbsaunde or surkov (who's still assigned)?
(In reply to Frederik Braun [:freddyb] from comment #29)
> (In reply to Mats Palmgren (:mats) from comment #28)
> > > because trimmedOffsets = { 1, 2} so GetEnd() returns 3
> > 
> > That doesn't seem right when the content is "A\n".
> > Why was the trimmedOffsets length set to 2?
> 
> 
> That's a good question. Who's going to continue working on this? tbsaunde or
> surkov (who's still assigned)?
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(surkov.alexander)
is anyone able to reproduce it nowadays? I tried a local nightly debug asan build and no luck.
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(surkov.alexander)
Should we try to find out which patch fixed this?
please reopen if you are able reproduce it

(In reply to Frederik Braun [:freddyb] from comment #32)
> Should we try to find out which patch fixed this?

it'd be nice of course, I'll leave the decision on it to Daniel.
Status: NEW → RESOLVED
Last Resolved: a year ago
Priority: -- → P1
Resolution: --- → WORKSFORME
(Reporter)

Comment 34

a year ago
I just reproduce this with a Linux ASan opt m-c build
Changeset: 32d9d1e81cc607320a36391845917f645f7a7f72
Build ID: 20170725154251

Some builds seem to repro faster than others.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Reporter)

Comment 35

a year ago
Created attachment 8889952 [details]
test_case.html

Update the test case. Hopefully this makes it a bit more reproducible.
Attachment #8837700 - Attachment is obsolete: true
Eitan, ni? in case if you are keen to take a look at this one.
Flags: needinfo?(eitan)
Tried with the build Tyson recommended, had it run for a while with no luck.
Flags: needinfo?(eitan)
(Reporter)

Comment 38

a year ago
Reproduced again with a m-c Linux ASan opt from earlier today.
BuildID=20170803132126
SourceStamp=fa1da3c0b200abbd9cfab3cab19962824314044e

Eitan, this seems to be fairly reliable using ffpuppet. With e10s disabled I see a crash in ~15 seconds, with e10s enabled < 1 minute. Here is the command I use:
python ffpuppet.py ~/workspace/browsers/m-c-1501766486-asan-opt/firefox -d -u test_case.html

Other things to note:
If I drag and drop the test case I don't get a crash.
Screen reader *NEEDS* to be enabled it does not repro with only GNOME_ACCESSIBILITY=1
I cannot repro with an ASan debug build (even one with the exact m-c sourcestamp).
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Priority: P1 → P2
Just NIing Eitan in case he missed comment 38. It seems like we should try to solve the reproducibility mystery. (Thanks for poking me about this bug RyanVM). This is sec-high. Eitan maybe you can steal this from Alex?
Flags: needinfo?(eitan)
Keywords: stale-bug
Blorg. I still can't reproduce this.

Tyson, when you say you need a screen reader, do you mean starting Orca, and having it run in the background?
Flags: needinfo?(eitan)
Flags: needinfo?(twsmith)
(Reporter)

Comment 42

a year ago
(In reply to Eitan Isaacson [:eeejay] from comment #41)
> Blorg. I still can't reproduce this.
> 
> Tyson, when you say you need a screen reader, do you mean starting Orca, and
> having it run in the background?

On ubuntu "universal access > screen reader > on" is what I'm doing.

I just repro'd on a new taskcluster build[1] with ffpuppet[2]. 

python -m ffpuppet ~/workspace/browsers/firefox/firefox -p prefs.js -d -u test_case.html

[1] https://tools.taskcluster.net/index/artifacts/gecko.v2.mozilla-central.latest.firefox/linux64-asan-opt
[2] https://github.com/MozillaSecurity/ffpuppet
Flags: needinfo?(twsmith)
(Reporter)

Comment 43

a year ago
Created attachment 8911869 [details]
prefs.js

Here is a prefs.js file that disables e10s. It seems to make it easier to repro.
Eitan, do you have more luck reproducing this bug thanks to the additional information provided by Tyson?
Flags: needinfo?(eitan)
(In reply to Stephanie Ouillon [:arroway] from comment #44)
> Eitan, do you have more luck reproducing this bug thanks to the additional
> information provided by Tyson?

Eitan is on PTO this week, so I can answer the question on this behalf I guess. Eitan was able to reproduce the crash reliably on this machine. The problem lies in the discrepancy between frame tree and accessible tree, in particular because HyperTextAccessible::mOffsets is not in sync with nsIFrame::GetRenderedText.

So the bug is on track.
Flags: needinfo?(eitan)
Has Regression Range: --- → no
status-firefox53: affected → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → fix-optional
status-firefox58: --- → affected
status-firefox-esr52: --- → affected
It's probably not fair to own the bug since Eitan is doing most of the work, so let's change assignee to Eitan for now. Let's adjust it later if we need to.
Assignee: surkov.alexander → eitan
I managed to reproduce this reliably with a python script that calls the right at-spi methods. I'll do more digging next week.
Created attachment 8928026 [details] [diff] [review]
Mochitest reproducing crash.

You still need an ASAN build to see this.
I attached a reliable mochitest for reproducing this with an ASAN build. Seems pretty straightforward. Alex know the text interfaces better, so I'm handing this off to him.
Assignee: eitan → surkov.alexander
Alex, this is a sec-high, which means that we'd ideally like this fixed in a 1-2 week timeframe.
Can you bump its priority in your list of acive bugs?

Thanks!
Flags: needinfo?(surkov.alexander)
(In reply to Eitan Isaacson [:eeejay] from comment #49)
> I attached a reliable mochitest for reproducing this with an ASAN build.
> Seems pretty straightforward. Alex know the text interfaces better, so I'm
> handing this off to him.

Before we utilize my ascribed text stuff knowledge :) could you remind what were your observations? Is it because HyperTextAccessible::mOffsets gets out of sync with GetRenderedText, correct? Do you have any logs left showing what was happening there?
Flags: needinfo?(surkov.alexander) → needinfo?(eitan)
(In reply to alexander :surkov from comment #51)
> (In reply to Eitan Isaacson [:eeejay] from comment #49)
> > I attached a reliable mochitest for reproducing this with an ASAN build.
> > Seems pretty straightforward. Alex know the text interfaces better, so I'm
> > handing this off to him.
> 
> Before we utilize my ascribed text stuff knowledge :) could you remind what
> were your observations? Is it because HyperTextAccessible::mOffsets gets out
> of sync with GetRenderedText, correct? Do you have any logs left showing
> what was happening there?

Sorry. I don't.
Flags: needinfo?(eitan)
I created a try server build [1], which is essentially Eitan's mochitest plus some diagnostic assertions.

It appears that a11y feeds wrong arguments into nsIFrame::GetRenderedText, which grows out of a discrepancy between real text and a11y cached text offsets I believe. 

However the problem is only caught at nsTextFragment::CharAt right before the crash. GetRenderedText's input arg assertions fail to catch the problem early. Also nsIFrame::GetRenderedText method should probably have some sanity checks to catch scenarios like this and return early without crashing.

Daniel, what is your thinking on this?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9e08b8cd58c352c50da135e8bb949ec344a816&selectedJob=159853708
Flags: needinfo?(dholbert)
(Assignee)

Comment 54

10 months ago
I'm taking a look with rr (making slow progress, as nsIFrame::GetRenderedText and its a11y callers are new to me).

It's not yet clear to me that a11y is doing anything wrong here.  And it does look like nsIFrame::GetRenderedText is documented as being fault-tolerant, so I agree that this seems to be a bug in its impl somewhere.

Here's what I've got so far: when we fail, we're working with the nsTextFrame for the string "A\n  "  (4 characters long -- A, newline, space, space), and we fail because GetRenderedText is calling nsTextFragment::CharAt(4) -- i.e. reading just past the end of this text fragment.

This happens in a call to nsTextFrame::GetRenderedText() with aStartOffset=1, aEndOffset=2.  That function makes the problematic CharAt(4) call here:
> 10161      if (isSkipped) {
> 10162        for (int32_t i = 0; i < runLength; ++i) {
> 10163          char16_t ch = textFrag->CharAt(iter.GetOriginalOffset() + i);
> 10164          if (ch == CH_SHY) {

At this point, have the following variable values:
 - iter.GetOriginalOffset() == 2
 - i == 2
    (so we pass in 2+2=4 to CharAt()).
 - runLength is 3, which comes from trimmedOffsets.GetEnd() - iter.GetOriginalOffset() which is 5 - 2 = 3.

I think "trimmedOffsets" might be the thing that's wrong here (and in particular, I think its "end" is too large by 1). Specifically, I'm suspicious of this code in nsTextFrame::GetRenderedText:
> 10145    trimmedOffsets.mStart = std::max<uint32_t>(trimmedOffsets.mStart,
> 10146        startOffset);
> 10147    trimmedOffsets.mLength = std::min<uint32_t>(trimmedOffsets.GetEnd(),
> 10148        endOffset) - trimmedOffsets.mStart;

Before this code, trimmedOffsets has mStart = 0, mLength = 4, and its GetEnd() returns 4.
After this code, trimmedOffsets has mStart = 1, mLength *still* 4, and its GetEnd() returns 5.

I suspect the developer writing this code *intended* to be clamping the start and the end independently (making them strictly narrower), and didn't realize that line 10145's mStart tweak was effectively *increasing* trimmedOffsets.GetEnd(), which influences the result of GetEnd() on line 10147.

I'll bet we probably want to capture GetEnd() in a local variable up-front, before we do any of this clamping, and use that original value at line 10147 rather than the (possibly-changed) GetEnd() value.

I'm a bit surprised that this hasn't caused trouble before, though...
Flags: needinfo?(dholbert)
(Assignee)

Comment 55

10 months ago
Created attachment 8949893 [details] [diff] [review]
fix v1

This patch avoids the crash in the mochitest for me. Does it seem to solve this bug for you, surkov?
Attachment #8949893 - Flags: review?(mats)
Attachment #8949893 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 56

10 months ago
I tested the attached patch by running:
  ./mach mochitest accessible/tests/mochitest/text/
...with the try run's mochitest+asserts patch (from comment 53) applied as well.

All tests passed.  So I don't think I broke anything -- but let me know if there are any other a11y testcases that are likely to exercise GetRenderedText() that I should run as well...
(In reply to Daniel Holbert [:dholbert] from comment #54)
> I suspect the developer writing this code *intended* to be clamping the
> start and the end independently (making them strictly narrower), and didn't
> realize that line 10145's mStart tweak was effectively *increasing*
> trimmedOffsets.GetEnd(), which influences the result of GetEnd() on line
> 10147.

Yeah, I'm pretty sure that side-effect on GetEnd() was unintended.

Updated

10 months ago
Attachment #8949893 - Flags: review?(mats) → review+

Updated

10 months ago
Assignee: surkov.alexander → dholbert
Component: Disability Access APIs → Layout: Text
(Assignee)

Comment 58

10 months ago
Comment on attachment 8949893 [details] [diff] [review]
fix v1

Requesting sec-approval for landing. (Though I'd probably like to wait for a sanity-check from surkov before landing as well.)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not too easily. Given how nontrivial it was for us to find a reliably reproducing testcase, combined with the need for a11y tools [or mochitest-harness-only APIs] to activate this codepath, I think this is pretty obscure. Also worth noting: I don't think this is an "execute arbitrary code" type of exploit -- rather, it's a "read a few bytes of uninitialized memory beyond the end of an array" type exploit.  It lets us call this CharAt() API with inappropriately large (out of bounds) aIndex values:
https://dxr.mozilla.org/mozilla-central/rev/aac7218d86242042f836d6258ff46dc7e4d62df2/dom/base/nsTextFragment.h#229,232

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment kind of hints at it. Perhaps I should soften it to something vaguer, like:
 Bug 1324042: Fix trimmedOffsets arithmetic in GetRenderedText(). r=mats

Which older supported branches are affected by this flaw?
All supported branches (back as far as Firefox 45) have the buggy lines of code, so I'd assume that all supported branches are affected.

If not all supported branches, which bug introduced the flaw?
This code is from bug 264412 (which landed in Firefox 45) -- here's the changeset where it was added:
https://hg.mozilla.org/mozilla-central/rev/95346f49d048#l20.192

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch is tiny and the affected code hasn't changed in a few years, so it should backport cleanly without needing any modifications.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions, I think. If it passes automated a11y tests (as it seems to on my machine), I'm fairly confident that it doesn't need further testing.
Attachment #8949893 - Flags: sec-approval?
sec-approval+ for trunk.
I'd like the patch nominated for beta and ESR52 as well.
status-firefox57: fix-optional → wontfix
status-firefox58: affected → wontfix
status-firefox59: --- → affected
status-firefox60: --- → affected
tracking-firefox59: --- → +
tracking-firefox60: --- → +
tracking-firefox-esr52: --- → 59+
Attachment #8949893 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 60

10 months ago
Comment on attachment 8949893 [details] [diff] [review]
fix v1

[ESR52 Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
It's sg:high.

User impact if declined:
Possible data leakage and crashes, from reading past the end of a buffer.

Fix Landed on Version:
Hasn't landed yet, but approved to land on 60.

Risk to taking this patch (and alternatives if risky):
Low risk. Only affects a fraction of our users (those with a11y tools), and the fix itself is quite targeted & makes us more correct about adjusting the end of a buffer.

String or UUID changes made by this patch:
None.
Attachment #8949893 - Flags: approval-mozilla-esr52?
(Assignee)

Comment 61

10 months ago
Comment on attachment 8949893 [details] [diff] [review]
fix v1

Beta Approval Request Comment
[Feature/Bug causing the regression]: bug 264412
[User impact if declined]: Possible data leakage and crashes, from reading past the end of a buffer.
[Is this code covered by automated tests?]: We do have an automated test to verify the fix, which can land once users have received the fix. I don't know as much about existing a11y test coverage, but I'm also not super-concerned given the surgical nature of this fix.
[Has the fix been verified in Nightly?]: Only in local builds. Hasn't quite landed in Nightly yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not particularly risky.
[Why is the change risky/not risky?]: The fix is quite targeted & makes us more correct/conservative about adjusting the end of a buffer.
[String changes made/needed]: None.
Attachment #8949893 - Flags: approval-mozilla-beta?
(Assignee)

Comment 62

10 months ago
(In reply to Daniel Holbert [:dholbert] from comment #58)
> Requesting sec-approval for landing. (Though I'd probably like to wait for a
> sanity-check from surkov before landing as well.)

sanity-check still welcome -- I went ahead and landed on inbound, though, since this has sec-approval+ now, and since the beta/esr approval questionnaires seem to depend on that happening before approval is granted.

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf54020043bd09c162530907b2a1091a10f4f92

> The check-in comment kind of hints at it. Perhaps I should soften it to
> something vaguer, like:
>  Bug 1324042: Fix trimmedOffsets arithmetic in GetRenderedText(). r=mats

(I adjusted to use this ^^ commit message language before landing, too, BTW.)
(Assignee)

Updated

10 months ago
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/fbf54020043b
Status: REOPENED → RESOLVED
Last Resolved: a year ago10 months ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8949893 [details] [diff] [review]
fix v1

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

a11y mochitest passes (https://treeherder.mozilla.org/#/jobs?repo=try&revision=10e51cc603642d1018a58abfc2df22407d3da5da&selectedJob=161718295).

what are practices in mochitest landing that reveals a sec bug?
Attachment #8949893 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 65

10 months ago
(In reply to alexander :surkov from comment #64)
> a11y mochitest passes
> (https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=10e51cc603642d1018a58abfc2df22407d3da5da&selectedJob=1
> 61718295).

Great! Yeah, it passed locally for me, too.

> what are practices in mochitest landing that reveals a sec bug?

General practice is to avoid 0-daying ourselves -- i.e. wait until the fix has reached users before making the test public.  So, we wouldn't land the test (or post it anywhere public) until the patch reaches all affected/supported releases, & users have had time to receive the upgrade.

(This technically means we should've avoided try runs with the testcase here. So, since we had a few of those, we already sort-of published the test, as of comment 53 and 64.  However, that's not too bad in this case, because even if bad guys notice that Try run, they probably can't do too much harm -- if they can even figure out how to turn it into exploit code, they have a limited window during which they can maybe inspect a few bytes past the end of a given nsTArray, and they won't be able to do remote code execution or anything like that.)
(In reply to Daniel Holbert [:dholbert] from comment #65)

> General practice is to avoid 0-daying ourselves -- i.e. wait until the fix
> has reached users before making the test public.  So, we wouldn't land the
> test (or post it anywhere public) until the patch reaches all
> affected/supported releases, & users have had time to receive the upgrade.
> 
> (This technically means we should've avoided try runs with the testcase
> here. So, since we had a few of those, we already sort-of published the
> test, as of comment 53 and 64.

you're right, for some reason I missed the point that try server is a way to publish things. I wasn't able to make a local build that reveals the problem, so I used the try server to check stuff. I wish we had an option to make protected try runs.
Attachment #8949893 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: layout-core-security → core-security-release
Comment on attachment 8949893 [details] [diff] [review]
fix v1

Approved for ESR 52.7.0
Attachment #8949893 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 69

10 months ago
uplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/ac743923f81d
status-firefox-esr52: affected → fixed
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+][adv-esr52.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.