Closed Bug 1188234 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: XUL, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: q1, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, csectype-uninitialized)

Attachments

(4 files)

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?
It's a trusted stream... I don't see how this can be a security issue.
(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?
Buffer overflows in random chrome code aren't really a security issue, as it could do other much worse things.
Group: core-security
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1185593
(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?
(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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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).
(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.
nsXULPrototypeElement::Serialize and its callers, generally speaking.
Blocks: 1185593
Assignee: nobody → continuation
Keywords: csectype-bounds
Summary: Possible security-related cause of bug 1185593 → nsXULPrototypeElement::Deserialize doesn't bounds check when accessing aNodeInfos
Component: DOM → XUL
Blocks: 1190639
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.
Attachment #8642814 - Flags: review?(bugs)
Make sure we don't do an out-of-bounds read if we read out a bogus value.
Attachment #8642815 - Flags: review?(bugs)
If we read out a bogus value of |mNumAttributes|, it could be
extremely large, causing the allocation of |mAttributes| to fail.
Attachment #8642816 - Flags: review?(bugs)
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.
Attachment #8642817 - Flags: review?(bugs)
Comment on attachment 8642814 [details] [diff] [review]
part 1 - Initialize |number|.

Lovely 13 years old code :/
Attachment #8642814 - Flags: review?(bugs) → review+
Comment on attachment 8642815 [details] [diff] [review]
part 2 - Add bounds checking in nsXULPrototypeElement::Deserialize().

I would just do aNodeInfos->SafeElementAt(number);
Attachment #8642815 - Flags: review?(bugs) → review+
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.
Attachment #8642816 - Flags: review?(bugs) → review+
(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.
(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.
Attachment #8642817 - Flags: review?(bugs) → review+
(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.
(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.
Thanks for the reviews!
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.
Attachment #8642814 - Flags: approval-mozilla-beta?
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.
Attachment #8642814 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8642815 [details] [diff] [review]
part 2 - Add bounds checking in nsXULPrototypeElement::Deserialize().

[Triage Comment]
Beta+
Attachment #8642815 - Flags: approval-mozilla-beta+
Comment on attachment 8642816 [details] [diff] [review]
part 3 - Make allocation of nsXULPrototypeAttribute fallible in nsXULPrototypeElement::Deserialize().

[Triage Comment]
Beta+
Attachment #8642816 - Flags: approval-mozilla-beta+
Comment on attachment 8642817 [details] [diff] [review]
part 4 - Don't append null to mChildren.

[Triage Comment]
Beta+
Attachment #8642817 - Flags: approval-mozilla-beta+
(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.
You need to log in before you can comment on or make changes to this bug.