Closed Bug 828958 Opened 7 years ago Closed 7 years ago

audit uses of DeserializeURI

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high)

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
Make sure there's no way child process can cause parent to crash by passing a null URI to IPDL.
Attachment #700353 - Flags: review?(bent.mozilla)
Attached patch v2Splinter Review
meet my friend hg qref
Assignee: nobody → jduell.mcbugs
Attachment #700353 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #700353 - Flags: review?(bent.mozilla)
Attachment #700354 - Flags: review?(bent.mozilla)
Comment on attachment 700354 [details] [diff] [review]
v2

Review of attachment 700354 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +133,5 @@
>    nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  if (!uri) {
> +    // URIParams does MOZ_ASSERT if null, but we need to protect opt builds from
> +    // null deref here.
> +    return SendFailedAsyncOpen(NS_ERROR_ILLEGAL_VALUE);

Let's just return false here.
Attachment #700354 - Flags: review?(bent.mozilla) → review+
Comment on attachment 700354 [details] [diff] [review]
v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3582dcf09df

[Approval Request Comment]
User impact if declined: compromised child process could make parent crash 
Risk to taking this patch (and alternatives if risky): none: 2 safe one liners
String or UUID changes made by this patch: no
Attachment #700354 - Flags: approval-mozilla-b2g18?
Comment on attachment 700354 [details] [diff] [review]
v2

Looks like safe and necessary.
Attachment #700354 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
I'm assuming approval-b2g+ means v1.0.1 only unless otherwise specified.  Do we want this on 1.0.0 too?

  https://hg.mozilla.org/releases/mozilla-b2g18/rev/b8a407f4e857
https://hg.mozilla.org/mozilla-central/rev/a3582dcf09df
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
>> Do we want this on 1.0.0 too?
>
> tracking-b2g18: 	+ 

I'm not clear if that's a "yes".  Doesn't sound like it?
blocking-b2g: --- → tef?
This is low risk, is an sec-high issue, and will prevent child processes from crashing the parent. Let's take it on v1.0.0 as well.
blocking-b2g: tef? → tef+
Keywords: sec-high
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/5253c6b83f3e

Also updated fix on 1.0.1 to use exact same logic (return false instead of return SendFailedAsyncOpen) just to keep branches in sync:

  https://hg.mozilla.org/releases/mozilla-b2g18/rev/c6a4a3d1ec69
You need to log in before you can comment on or make changes to this bug.