1243463 - Make nsTArrayHeader::sEmptyHdr const

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[q1]

If a bug lets an attacker cause FF to write beyond the end of an empty nsTArray, she can overwrite static variables inside xul.dll. This is very dangerous, because the exact identity and order of those variables is fixed by the build, and is thus known to the attacker.

Putting nsTArrayHeader::sEmptyHdr into readonly memory would much reduce the risk posed by this kind of bug.

I raised this issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1232069#c2 (a bug that allows exactly this kind of write-beyond-end), but think that it deserves its own bug.  Andrew McCreight also raised a related issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1240838 .

I marked this bug as closed because it might suggest targets to attackers.

Comment 1

[q1]

Some amount of memory (perhaps a few pages?) following &nsTArrayHeader::sEmptyHdr should also be readonly, because an empty nsTArray indexes the byte just following nsTArrayHeader::sEmptyHdr.

Comment 2

[Nathan Froyd [:froydnj]]

Do we need this now that we bounds-check array accesses?

Comment 3

[q1]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Do we need this now that we bounds-check array accesses?

That doesn't help if a caller uses nsTArray::Elements() to get a pointer to the first element, then overruns the array.

Comment 4

[q1]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Do we need this now that we bounds-check array accesses?

Ack! What I mean is that the bounds-check doesn't help if a caller uses nsTArray::Elements() to get a pointer to the first element, then overruns the array.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: Bug 1243463 - make `sEmptyTArrayHeader` const; r=mccr8

We fixed what I think is the lone instance of writing into the (empty)
header in SetLength because it was causing TSan violations, so we should
be clear to make this const. This change is not terribly effective on its
own (cf. the const_cast required to make this work at all), but in the
next patch, we can rig up sEmptyTArrayHeader to be surrounded with "guard
pages" and make rogue accesses off the array header a little more protected.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: Bug 1243463 - move `sEmptyTArrayHeader` definition to assembly; r=mccr8

Depends on D88657

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: Bug 1243463 - add padding around sEmptyTArrayHeader; r=mccr8

Depends on D88658

Comment 8

[Andrew McCreight [:mccr8]]

Bug 1240838 is related.

Comment 9

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

https://hg.mozilla.org/integration/autoland/rev/ac566cd8ff453656c996a3b792579e370a8b7a68
https://hg.mozilla.org/mozilla-central/rev/ac566cd8ff45

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:nika, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Andrew McCreight [:mccr8]]

Tom, why did you reopen this issue? If there's some remaining hardening that could be done here, perhaps you could file a new bug for it? Thanks.

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

We did not resolve this issue fully - we made the object const, but not read-only. I will open a new bug for that task.