Closed Bug 828958 Opened 11 years ago Closed 11 years ago

audit uses of DeserializeURI

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Project Flags:
blocking-b2g tef+
Tracking Flags:
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)

v2
1.98 KB, patch
bent.mozilla
: review+
milan
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
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 v2 — — Splinter 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
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → affected
tracking-b2g18: --- → ?
https://hg.mozilla.org/mozilla-central/rev/a3582dcf09df
Status: ASSIGNED → RESOLVED
Closed: 11 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
status-b2g18-v1.0.0: affected → fixed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: