Closed Bug 828958 Opened 12 years ago Closed 12 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: 12 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: