1188234 - nsXULPrototypeElement::Deserialize doesn't bounds check when accessing aNodeInfos

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[q1]

I just saw https://bugzilla.mozilla.org/show_bug.cgi?id=1185593, with the suggestive crash frame:

0 	xul.dll 	nsXULPrototypeElement::Deserialize(nsIObjectInputStream*, nsXULPrototypeDocument*, nsIURI*, nsTArray<nsRefPtr<mozilla::dom::NodeInfo> > const*) 	dom/xul/nsXULElement.cpp

Which resolves to:

nsresult
nsXULPrototypeElement::Deserialize(nsIObjectInputStream* aStream,
                                   nsXULPrototypeDocument* aProtoDoc,
                                   nsIURI* aDocumentURI,
                                   const nsTArray<nsRefPtr<mozilla::dom::NodeInfo>> *aNodeInfos)
{
    NS_PRECONDITION(aNodeInfos, "missing nodeinfo array");

    // Read Node Info
    uint32_t number;
    nsresult rv = aStream->Read32(&number);
>    mNodeInfo = aNodeInfos->ElementAt(number);

Is it really OK to read |number| from the stream and use it as an index into aNodeInfos without checking it?

Comment 1

[Benjamin Smedberg]

It's a trusted stream... I don't see how this can be a security issue.

Comment 2

[q1]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> It's a trusted stream... I don't see how this can be a security issue.

Do you mean "trusted" in the sense that only Mozilla-supplied code or a malfunctioning extension could produce an invalid stream?

Comment 3

[Andrew McCreight [:mccr8]]

Buffer overflows in random chrome code aren't really a security issue, as it could do other much worse things.

Comment 4

[q1]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Buffer overflows in random chrome code aren't really a security issue, as it
> could do other much worse things.

Hmm. That seems true about intentional attacks, but not entirely with respect to bugs in extensions. How much should the framework protect itself from that kind of thing?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to q1 from comment #4)
> Hmm. That seems true about intentional attacks, but not entirely with
> respect to bugs in extensions. How much should the framework protect itself
> from that kind of thing?

Sure, I guess it could be useful to add some more checks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This particular stream is produced by our C++ code; I don't believe extensions have a chance to stomp on it by accident (though a malicious one might be able to mess with things here, I expect).

Comment 7

[q1]

(In reply to Boris Zbarsky [:bz] from comment #6)
> This particular stream is produced by our C++ code; I don't believe
> extensions have a chance to stomp on it by accident (though a malicious one
> might be able to mess with things here, I expect).

What code produces this stream? It does seem that something stomped it, judging by the stack trace, though it's not conclusive.

Comment 8

[Boris Zbarsky [:bzbarsky]]

nsXULPrototypeElement::Serialize and its callers, generally speaking.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Initialize |number|.

There's a bunch of patches here, but it is just a series of small fixes for things I noticed in this function, including the original issue.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc1270fc3343

If the first Read32() fails, |number| will be garbage.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Add bounds checking in nsXULPrototypeElement::Deserialize().

Make sure we don't do an out-of-bounds read if we read out a bogus value.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make allocation of nsXULPrototypeAttribute fallible in nsXULPrototypeElement::Deserialize().

If we read out a bogus value of |mNumAttributes|, it could be
extremely large, causing the allocation of |mAttributes| to fail.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Don't append null to mChildren.

Other code does not expect random nulls in this array, so avoid adding
them, just in case we somehow end up looking at it. This code returns
right after the append so it should be okay.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8642814 [details] [diff] [review]
part 1 - Initialize |number|.

Lovely 13 years old code :/

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8642815 [details] [diff] [review]
part 2 - Add bounds checking in nsXULPrototypeElement::Deserialize().

I would just do aNodeInfos->SafeElementAt(number);

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8642816 [details] [diff] [review]
part 3 - Make allocation of nsXULPrototypeAttribute fallible in nsXULPrototypeElement::Deserialize().

Either this or remove the OOM check. Perhaps this is better.

Comment 16

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #14)
> I would just do aNodeInfos->SafeElementAt(number);

It looked like SafeElementAt() will just crash the browser, and it seems bad to crash in this startup code.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #15)
> Either this or remove the OOM check. Perhaps this is better.

My philosophy here is that the Read32 calls should be able to return any values and we shouldn't crash, just fail.

Comment 18

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #16)
> It looked like SafeElementAt() will just crash the browser, and it seems bad
> to crash in this startup code.

Olli pointed out in IRC that it actually just takes a default argument and returns that, so I'll just do that.

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to q1 from comment #7)
> What code produces this stream? It does seem that something stomped it,
> judging by the stack trace, though it's not conclusive.

This code is run when we are reading the XUL prototype cache during startup. This cache can get mangled in various cases, like if a user uses Windows file restore to go to an earlier version that uses a slightly different format, or maybe if it crashes while writing it is out. We're supposed to detect problems like that and just invalidate the cache, but it doesn't always happen due to other bugs.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d70f26619eca
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ee2ada09b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f9116e9468
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ae99b066a1

Comment 21

[Andrew McCreight [:mccr8]]

Thanks for the reviews!

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d70f26619eca
https://hg.mozilla.org/mozilla-central/rev/36ee2ada09b9
https://hg.mozilla.org/mozilla-central/rev/e7f9116e9468
https://hg.mozilla.org/mozilla-central/rev/04ae99b066a1

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8642814 [details] [diff] [review]
part 1 - Initialize |number|.

The top crash in bug 1185593 seems to have gone away with the first Nightly these patches were in, so something in here must have fixed it. Thanks for the analysis, q1!

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: This seems to have fixed a Windows top crash that was showing up 42 (bug 1185593) and is also affecting 41, so I'm filing for approval for 41, which I think is beta now.
[Describe test coverage new/current, TreeHerder]: I'm not sure. I think our automated testing of this code is bad.
[Risks and why]: Low. This just improves our error handling in a number of cases. There are four patches, but they are simple.
[String/UUID change made/needed]: none

This approval request is for all four patches.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8642814 [details] [diff] [review]
part 1 - Initialize |number|.

The evidence suggests that the topcrash is gone with this fix in Nightly (yes, I confirmed!). Let's uplift to m-b.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8642815 [details] [diff] [review]
part 2 - Add bounds checking in nsXULPrototypeElement::Deserialize().

[Triage Comment]
Beta+

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8642816 [details] [diff] [review]
part 3 - Make allocation of nsXULPrototypeAttribute fallible in nsXULPrototypeElement::Deserialize().

[Triage Comment]
Beta+

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8642817 [details] [diff] [review]
part 4 - Don't append null to mChildren.

[Triage Comment]
Beta+

Comment 28

[q1]

(In reply to Andrew McCreight [:mccr8] from comment #23)
> Comment on attachment 8642814 [details] [diff] [review]
> part 1 - Initialize |number|.
> 
> The top crash in bug 1185593 seems to have gone away with the first Nightly
> these patches were in, so something in here must have fixed it. Thanks for
> the analysis, q1!

You're welcome. I'm always happy to see another bug squashed.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e5423b114251
https://hg.mozilla.org/releases/mozilla-beta/rev/b3d63dc2991e
https://hg.mozilla.org/releases/mozilla-beta/rev/8739aada6815
https://hg.mozilla.org/releases/mozilla-beta/rev/13724d34c866