Closed Bug 334700 Opened 18 years ago Closed 12 years ago

Possible null pointer dereference in LogHeaders (nsHttpTransaction.cpp)

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: kherron+mozilla, Assigned: mcmanus)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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?
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
Attached patch patch 0Splinter Review
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #658056 - Flags: review?(cbiesinger)
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?
Blocks: 788471
(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.
Attachment #658056 - Flags: review?(cbiesinger) → review+
https://hg.mozilla.org/mozilla-central/rev/35e518505ef2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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.

Attachment

General

Created:
Updated:
Size: