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

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)

Did you have any luck reproducing this?

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

Tyson, is there any specific STR to follow?

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?

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

(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.

ok, what lines should be added/removed to/from my config?

This should help. https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer

Any luck testing with ASAN? You can just download builds as linked in comment 13.

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

universal access -> screen reader -> on of course before I start the browsers

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.

v3 trying to increase reliability

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.

(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.

Alex any luck?

(In reply to David Bolter [:davidb] from comment #23) > Alex any luck? nope, it seems I keep running into bug 1333715 for ASAN build.

Trevor do you think you could catch this in rr?

(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.

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.

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

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

is anyone able to reproduce it nowadays? I tried a local nightly debug asan build and no luck.

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.

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.

Update the test case. Hopefully this makes it a bit more reproducible.

Eitan, ni? in case if you are keen to take a look at this one.

Tried with the build Tyson recommended, had it run for a while with no luck.

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.

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?

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?

(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

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?

(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.

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.

I managed to reproduce this reliably with a python script that calls the right at-spi methods. I'll do more digging next week.

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.

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!

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

(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.

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

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

This patch avoids the crash in the mochitest for me. Does it seem to solve this bug for you, surkov?

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.

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.

sec-approval+ for trunk. I'd like the patch nominated for beta and ESR52 as well.

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.

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.

(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.)

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?

(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.

Comment on attachment 8949893 [details] [diff] [review] fix v1 Approved for ESR 52.7.0