761479 - XMLHttpRequest with mozAnon=true should still send explicitly passed username + password

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Philipp von Weitershausen [:philikon]]

In bug 692677 we added the mozAnon flag to XHR which tells the User Agent not to pass cookies and other auth information along with the request. We use the nsIRequest::LOAD_ANONYMOUS flag for that. However, an explicit username and password as passed into the xhr.open() method isn't sent along either, but it really should.

Comment 1

[James Lal [:lightsofapollo]]

This is going to be very important for all the protocols the email/calendar app intend to use with this api.

Comment 2

[Philipp von Weitershausen [:philikon]]

Alright, I think I finally have a grasp of what's going on:

(1) The username, password parameters are equivalent to using http://username:password/... in the URI, as is evident from nsXMLHttpRequest::Open(): https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1888

(2) The URI credentials are used in nsHttpChannelAuthProvider::SetAuthorizationHeader(): https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#1199

(3) However, that is skipped if the channel has the nsIRequest::LOAD_ANONYMOUS load flag set: https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#176

While I don't feel inclined to mess with Necko too much, it seems error-prone to manually set the 'Authorization' header in nsXMLHttpRequest or something like that. My suggestion would be to invent a new flag, e.g. nsIRequest::LOAD_ANONYMOUS_WITH_URI_IDENTITY that we can check for in nsHttpChannelAuthProvider::AddAuthorizationHeaders().

Thoughts?

Comment 3

[Philipp von Weitershausen [:philikon]]

CCing bz for some guidance.

Comment 4

[Boris Zbarsky [:bzbarsky]]

What's the actual desired behavior here?  Why are we trying to send some kinds of authentication information but not others?

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Boris Zbarsky (:bz) from comment #4)
> What's the actual desired behavior here?  Why are we trying to send some
> kinds of authentication information but not others?

Basically, yes. The behaviour that we expect (Sicking and I, at least) would be that for the following request

  xhr = new XmlHttpRequest({mozAnon: true});
  xhr.open("GET", url, true, username, password)

the server would still receive the explicitly passed username and password values, whereas any username + password values from the auth cache as well as cookies would not be sent along.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Would user certificates be sent?  What about other authentication mechanisms that XHR doesn't really expose at the moment but might in the future?

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Would user certificates be sent?

I would not expect them to be sent. But I do not know whether they will. I should check and maybe also write a unit test for this.

> What about other authentication mechanisms
> that XHR doesn't really expose at the moment but might in the future?

If they're explicitly set on the XHR, I would expect them to be sent. AFAIUI, mozAnon = true tells the User Agent something like "don't add anything *you* know about this host from previous encounters, but let *me* add stuff explicitly".

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, then it seems like we need a new request flag value to indicate that...

Comment 9

[Andreas Gal :gal]

How are we doing here?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Removing this as a blocker. If someone thinks it should be a blocker, please let me know exactly how we are planning on using this in a v1 app.

As far as I can tell we wouldn't be using this for the email app since it's not using XHR at all. And I'm not aware that calendar stuff is commonly behind http-auth based security.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 1

I don't know if this is the right way to fix it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 661767 [details] [diff] [review]
patch 1

This is so not my thing.  Let's try Honza, I guess?

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 661767 [details] [diff] [review]
patch 1

This is a first sight feedback only:
- nsHttpChannelAuthProvider::GetCredentialsFromURI duplicates some code we already have
- this probably won't work for NTLM or any state-full auth scheme

I'll do a proper review, latest the next week.  

If you have a new version of the patch, please don't obsolete this one (it would kill my splinter comments).  Thanks.

Comment 14

[Honza Bambas (:mayhemer)]

How urgent is this bug to get fixed?

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: WIP1

A bit different approach.  Basic tests are passing but this one needs a new, more specific test yet.

Comment 16

[Andrea Marchesini [:baku]]

can you use the mochitest from my patch?

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini (:baku) from comment #16)
> can you use the mochitest from my patch?

I did, my patch didn't pass (got stuck on a prompt).  I'll look after the cause.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> (In reply to Andrea Marchesini (:baku) from comment #16)
> > can you use the mochitest from my patch?
> 
> I did, my patch didn't pass (got stuck on a prompt).  I'll look after the
> cause.

So, I retried and now it passes (maybe it was just a rebuild problem).  I pushed the patch to try as well along with patch for bug 776171:

https://tbpl.mozilla.org/?tree=Try&rev=3c489d152066

We'll see..

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v2 + test by Andrea Marchesini

- check for the ANON flag moved down to nsHttpChannelAuthProvider::GetCredentialsForChallenge
- includes a test from Andrea that I r+'ed

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 669529 [details] [diff] [review]
v2 + test by Andrea Marchesini

r=honzab for the test parts.

Comment 21

[Honza Bambas (:mayhemer)]

Try passes. https://tbpl.mozilla.org/?tree=Try&rev=04b88dc7eba1

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v3 + updated test originally by Andrea Marchesini

I found an issue with my patch.  When there are cached credentials with the same username but different password then those are prioritized over the credentials in the URL.  When LOAD_ANONYMOUS is set we have to bypass this cache override to prevent leak.  Though, we still need to use cache here to make state-full schemes work (NTLM and Digest).

This is an updated patch, changes:
- the "taking identity from auth cache" block is not executed also when LOAD_ANONYMOUS is engaged
- test updated to include check for this code path

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 673901 [details] [diff] [review]
v3 + updated test originally by Andrea Marchesini

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a526f4d7b8

(try: https://tbpl.mozilla.org/?tree=Try&rev=d79b4fabfdc0)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1a526f4d7b8