1324042 - Heap buffer over read [@nsTextFrame::GetRenderedText]

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: 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

Comment 1

[Tyson Smith [:tsmith]]

Attachment: log.txt

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Mats Palmgren (inactive)]

It looks like this might be an a11y issue?  (feel free to bounce it back
if it's a bug in Layout)

Comment 4

[Mats Palmgren (inactive)]

Did you have any luck reproducing this?

Comment 5

[alexander :surkov (:asurkov)]

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

Comment 6

[Mats Palmgren (inactive)]

Tyson, is there any specific STR to follow?

Comment 7

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 8

[Tyson Smith [:tsmith]]

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 :)

Comment 9

[Stephanie Ouillon [:arroway] (needinfo me)]

Are your able to reproduce the issue with the updated test case?

Comment 10

[alexander :surkov (:asurkov)]

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

Comment 11

[Daniel Veditz [:dveditz]]

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

Comment 12

[alexander :surkov (:asurkov)]

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

Comment 13

[Tyson Smith [:tsmith]]

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

Comment 14

[Frederik Braun [:freddy]]

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

Comment 15

[alexander :surkov (:asurkov)]

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

Comment 16

[alexander :surkov (:asurkov)]

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

Comment 17

[Tyson Smith [:tsmith]]

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.

Comment 18

[Tyson Smith [:tsmith]]

Attachment: test_case.html

v3 trying to increase reliability

Comment 19

[alexander :surkov (:asurkov)]

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.

Comment 20

[Tyson Smith [:tsmith]]

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

Comment 21

[Mats Palmgren (inactive)]

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

Comment 22

[Frederik Braun [:freddy]]

Assigning to Alexander to continue testing, as suggested by Tyson and Mats in comment 20 and comment 21.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Alex any luck?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #23)
> Alex any luck?

nope, it seems I keep running into bug 1333715 for ASAN build.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Trevor do you think you could catch this in rr?

Comment 26

[Trevor Saunders (:tbsaunde)]

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

Comment 27

[Trevor Saunders (:tbsaunde)]

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.

Comment 28

[Mats Palmgren (inactive)]

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

Comment 29

[Frederik Braun [:freddy]]

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

Comment 30

[Frederik Braun [:freddy]]

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

Comment 31

[alexander :surkov (:asurkov)]

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

Comment 32

[Frederik Braun [:freddy]]

Should we try to find out which patch fixed this?

Comment 33

[alexander :surkov (:asurkov)]

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.

Comment 34

[Tyson Smith [:tsmith]]

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.

Comment 35

[Tyson Smith [:tsmith]]

Attachment: test_case.html

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

Comment 36

[alexander :surkov (:asurkov)]

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

Comment 37

[Eitan Isaacson [:eeejay]]

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

Comment 38

[Tyson Smith [:tsmith]]

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

Comment 39

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

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.

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 41

[Eitan Isaacson [:eeejay]]

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?

Comment 42

[Tyson Smith [:tsmith]]

(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

Comment 43

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Here is a prefs.js file that disables e10s. It seems to make it easier to repro.

Comment 44

[Stephanie Ouillon [:arroway] (needinfo me)]

Eitan, do you have more luck reproducing this bug thanks to the additional information provided by Tyson?

Comment 45

[alexander :surkov (:asurkov)]

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

Comment 46

[alexander :surkov (:asurkov)]

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.

Comment 47

[Eitan Isaacson [:eeejay]]

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

Comment 48

[Eitan Isaacson [:eeejay]]

Attachment: Mochitest reproducing crash.

You still need an ASAN build to see this.

Comment 49

[Eitan Isaacson [:eeejay]]

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.

Comment 50

[Frederik Braun [:freddy]]

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!

Comment 51

[alexander :surkov (:asurkov)]

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

Comment 52

[Eitan Isaacson [:eeejay]]

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

Comment 53

[alexander :surkov (:asurkov)]

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

Comment 54

[Daniel Holbert [:dholbert]]

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

Comment 55

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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

Comment 56

[Daniel Holbert [:dholbert]]

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

Comment 57

[Mats Palmgren (inactive)]

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

[Daniel Holbert [:dholbert]]

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.

Comment 59

[Al Billings [:abillings - ex-MoCo]]

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

Comment 60

[Daniel Holbert [:dholbert]]

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 61

[Daniel Holbert [:dholbert]]

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.

Comment 62

[Daniel Holbert [:dholbert]]

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

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

https://hg.mozilla.org/mozilla-central/rev/fbf54020043b

Comment 64

[alexander :surkov (:asurkov)]

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?

Comment 65

[Daniel Holbert [:dholbert]]

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

Comment 66

[alexander :surkov (:asurkov)]

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

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

https://hg.mozilla.org/releases/mozilla-beta/rev/bec50ba1fb12

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8949893 [details] [diff] [review]
fix v1

Approved for ESR 52.7.0

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ac743923f81d