Carve out an exception for embedded credentials in XHR.

Carve out an exception for embedded credentials in XHR.

As discussed in https://crbug.com/707761, the security justification for
restricting username/password in XHR is weaker than I thought it was.
I'd still _like_ to remove developer-controlled usernames and passwords
from the platform, but I was incorrect to point to them as an actual
vulnerability, given the way basic/digest auth actually works (requiring
CORS-same-originness, and handshaking through a 401 response).

So, this patch limits the previous restrictions against embedded
credentials to non-XHR use cases. That will make SAP happy, and should
resolve the other complaints this change has generated.

BUG=707761, 708131, 504300
Review-Url: https://codereview.chromium.org/2808753003
Cr-Commit-Position: refs/heads/master@{#464019}
Committed: https://chromium.googlesource.com/chromium/src/+/fd04d4a0b5f4a35c4acd66a0b35773deb33e8bb6

[Mike West, 2017-04-10 14:57:51]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 14:58:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 15:36:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 15:36:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2017-04-11 08:10:51]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 08:11:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:50:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:50:52]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-04-11 11:31:42]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2017-04-11 11:31:43]

WDYT about carving out XHR, Jochen?

[Mike West, 2017-04-11 11:32:16]

mkwst@chromium.org changed reviewers:
+ foolip@chromium.org, rbyers@chromium.org

[Mike West, 2017-04-11 11:32:16]

+other API OWNER folks who approved the initial intent.

[Mike West, 2017-04-11 11:35:52]

Description was changed from

==========
Carve out an exception for embedded credentials in XHR.

BUG=707761,504300
==========

to

==========
Carve out an exception for embedded credentials in XHR.

As discussed in https://crbug.com/707761, the security justification for
restricting username/password in XHR is weaker than I thought it was.
I'd still _like_ to remove developer-controlled usernames and passwords
from the platform, but I was incorrect to point to them as an actual
vulnerability, given the way basic/digest auth actually works (requiring
CORS-same-originness, and handshaking through a 401 response).

So, this patch limits the previous restrictions against embedded
credentials to non-XHR use cases. That will make SAP happy, and should
resolve the other complaints this change has generated.

BUG=707761,708131,504300
==========

[jochen (gone - plz use gerrit), 2017-04-11 11:36:34]

what happened to the idea of forcing a preflight?

[Mike West, 2017-04-11 12:27:40]

On 2017/04/11 at 11:36:34, jochen wrote:
> what happened to the idea of forcing a preflight?

This doesn't force a preflight, but instead relies on the existing basic auth
behavior, which is similar.

Scenario 1:
1.  `evil.com` fires an XHR at `https://user:pass@bank.com/`.
2.  Blink sends a request to `https://bank.com/` (no `Authorization` header)
3.  `https://bank.com/` responds with a 200.
4.  Done, no `Authorization` header is sent.

Scenario 2:
1.  `evil.com` fires an XHR at `https://user:pass@bank.com/`.
2.  Blink sends a request to `https://bank.com/` (no `Authorization` header)
3.  `https://bank.com/` responds with a 401, without `Allow-Access-From-Origin`.
4.  Done, no `Authorization` header is sent.

Scenario 3:
1.  `evil.com` fires an XHR at `https://user:pass@bank.com/`.
2.  Blink sends a request to `https://bank.com/` without an `Authorization`
header.
3.  `https://bank.com/` responds with a 401, with a valid
`Allow-Access-From-Origin`.
4.  Blink sends a new request to `https://bank.com/` with an `Authorization`
header.
5.  Done.

I'd misread step 17 and step 23 of
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch, basically. We only
send the authorization header if there's a preexisting authentication value (in
which case the embedded username/password aren't actually used) or if there's
already been a 401 response that request authentication.

[Mike West, 2017-04-11 12:28:54]

On 2017/04/11 at 12:27:40, Mike West wrote:
> On 2017/04/11 at 11:36:34, jochen wrote:
> > what happened to the idea of forcing a preflight?
> 
> This doesn't force a preflight, but instead relies on the existing basic auth
behavior, which is similar.
> 
> Scenario 1:
> 1.  `evil.com` fires an XHR at `https://user:pass@bank.com/`.
> 2.  Blink sends a request to `https://bank.com/` (no `Authorization` header)
> 3.  `https://bank.com/` responds with a 200.
> 4.  Done, no `Authorization` header is sent.
> 
> Scenario 2:
> 1.  `evil.com` fires an XHR at `https://user:pass@bank.com/`.
> 2.  Blink sends a request to `https://bank.com/` (no `Authorization` header)
> 3.  `https://bank.com/` responds with a 401, without
`Allow-Access-From-Origin`.
> 4.  Done, no `Authorization` header is sent.
> 
> Scenario 3:
> 1.  `evil.com` fires an XHR at `https://user:pass@bank.com/`.
> 2.  Blink sends a request to `https://bank.com/` without an `Authorization`
header.
> 3.  `https://bank.com/` responds with a 401, with a valid
`Allow-Access-From-Origin`.
> 4.  Blink sends a new request to `https://bank.com/` with an `Authorization`
header.
> 5.  Done.
> 
> I'd misread step 17 and step 23 of
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch, basically. We only
send the authorization header if there's a preexisting authentication value (in
which case the embedded username/password aren't actually used) or if there's
already been a 401 response that request authentication.

(Why don't I add a test to lock this in?)

[jochen (gone - plz use gerrit), 2017-04-11 12:31:49]

that means that you could still use this to brute force router passwords?

[foolip, 2017-04-11 14:12:53]

Leaving this to jochen@, please poke me if I'm needed.

[Mike West, 2017-04-12 05:01:42]

On 2017/04/11 at 12:31:49, jochen wrote:
> that means that you could still use this to brute force router passwords?

Only if the router returns a 401 response requiring authentication, and includes
`Access-Control-Allow-Origin: https://evil.com/` in the response.

[jochen (gone - plz use gerrit), 2017-04-12 09:26:20]

I asked on the bug whether it was possible to restrict this to https... let's
give them a day to answer

[jochen (gone - plz use gerrit), 2017-04-12 10:28:58]

guess they don't do https :/

oh well, lgtm

[Mike West, 2017-04-12 11:25:00]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-04-12 11:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2808753003/#ps40001
(title: "Rebaseline.")

[commit-bot: I haz the power, 2017-04-12 11:25:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 14:38:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491996300507250,
"parent_rev": "53cb7fcf9db050cac655e47ff85d02daac7dbb22", "commit_rev":
"fd04d4a0b5f4a35c4acd66a0b35773deb33e8bb6"}

[commit-bot: I haz the power, 2017-04-12 14:39:26]

Description was changed from

==========
Carve out an exception for embedded credentials in XHR.

As discussed in https://crbug.com/707761, the security justification for
restricting username/password in XHR is weaker than I thought it was.
I'd still _like_ to remove developer-controlled usernames and passwords
from the platform, but I was incorrect to point to them as an actual
vulnerability, given the way basic/digest auth actually works (requiring
CORS-same-originness, and handshaking through a 401 response).

So, this patch limits the previous restrictions against embedded
credentials to non-XHR use cases. That will make SAP happy, and should
resolve the other complaints this change has generated.

BUG=707761,708131,504300
==========

to

==========
Carve out an exception for embedded credentials in XHR.

As discussed in https://crbug.com/707761, the security justification for
restricting username/password in XHR is weaker than I thought it was.
I'd still _like_ to remove developer-controlled usernames and passwords
from the platform, but I was incorrect to point to them as an actual
vulnerability, given the way basic/digest auth actually works (requiring
CORS-same-originness, and handshaking through a 401 response).

So, this patch limits the previous restrictions against embedded
credentials to non-XHR use cases. That will make SAP happy, and should
resolve the other complaints this change has generated.

BUG=707761,708131,504300

Review-Url: https://codereview.chromium.org/2808753003
Cr-Commit-Position: refs/heads/master@{#464019}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd04d4a0b5f4a35c4acd66a0b357...
==========

[commit-bot: I haz the power, 2017-04-12 14:39:31]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fd04d4a0b5f4a35c4acd66a0b357...