Closed
Bug 334700
Opened 18 years ago
Closed 12 years ago
Possible null pointer dereference in LogHeaders (nsHttpTransaction.cpp)
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: kherron+mozilla, Assigned: mcmanus)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file)
1.75 KB,
patch
|
jduell.mcbugs
:
review+
Biesinger
:
review+
|
Details | Diff | Splinter Review |
This was found through a coverity scan of the source tree. Please refer to the sample URL. If logging is turned on, nsHttpTransaction can call a function |LogHeaders| to log request and reply headers in a transaction. LogHeaders contains code to shroud credentials in the Authorization header: 107 if (PL_strcasestr(buf.get(), "authorization: ") != nsnull) { 108 char *p = PL_strchr(PL_strchr(buf.get(), ' ')+1, ' '); 109 while (*++p) *p = '*'; 110 } For the reply headers in particular, I assume |LogHeaders| receives non-normalized headers including possible syntax errors. If the authorization header doesn't contain least two spaces, the outer |PL_strchr| call will return null, leading to a null pointer dereference on the next line. Less important, if the header contains two or more spaces following the colon, then this code will fail to shroud the credentials field. Also, maybe it should shroud the proxy-authentication header as well?
Assignee | ||
Comment 1•12 years ago
|
||
6 years. blegh. anyhow, yes - that's a null deref on "authorization: foo"; but only if NSPR logging is turned on so its not a general purpose vulnerability. I've fixed that and added proxy-authorization to the screen - but its not a great use of time to add a proper tokenizer in this bug for other formulations. patch forthcoming
Assignee | ||
Comment 2•12 years ago
|
||
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #658056 -
Flags: review?(cbiesinger)
Comment 3•12 years ago
|
||
Comment on attachment 658056 [details] [diff] [review] patch 0 Review of attachment 658056 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +60,5 @@ > + while ((endOfLine = PL_strstr(lineStart, "\r\n"))) { > + buf.Assign(lineStart, endOfLine - lineStart); > + if (PL_strcasestr(buf.get(), "authorization: ") || > + PL_strcasestr(buf.get(), "proxy-authorization: ")) { > + char *p = PL_strchr(PL_strchr(buf.get(), ' ') + 1, ' '); still has issue that we don't mask the credentials if the auth header is followed by two spaces? We could trawl forward with a while (isspace()) followed by while (!isspace()) to get past it w/o that flaw?
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Jason Duell (:jduell) from comment #3) > > still has issue that we don't mask the credentials if the auth header is > followed by two spaces? that's what I was, too obliquely, referring to by "not a great use of time to add a proper tokenizer in this bug" in comment 1. The crash is the bit I want to fix up here. The failure mode is actually that we obfuscate too much (starting at the second space, not the second token), not too little - right? This obfuscation stuff is pretty uninteresting anyhow when all the real information is in the cookie. bug 788471 filed for the leftover.
Updated•12 years ago
|
Attachment #658056 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e518505ef2
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/35e518505ef2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 7•12 years ago
|
||
Comment on attachment 658056 [details] [diff] [review] patch 0 Review of attachment 658056 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +59,5 @@ > + char *endOfLine; > + while ((endOfLine = PL_strstr(lineStart, "\r\n"))) { > + buf.Assign(lineStart, endOfLine - lineStart); > + if (PL_strcasestr(buf.get(), "authorization: ") || > + PL_strcasestr(buf.get(), "proxy-authorization: ")) { no real need for this line since the first strcasestr already covers it
Attachment #658056 -
Flags: review+
You need to log in
before you can comment on or make changes to this bug.
Description
•