Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block subresource requests whose URLs include credentials. #465

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Jan 24, 2017

Hard-coding credentials into subresource requests (e.g.
https://user:pass@host/) is problematic from a security perspective,
as it's allowed folks to brute-force credentials in the past, enables
session fixation attacks for sites using basic auth, and can allow
attackers access to well-known, poorly-coded devices (such as users'
routers). Moreover, the ability to hard-code credentials leads to
inadvertant leakage via XSS on the one hand, and poor development
practice on the other. Sifting through HTTPArchive, for example, yields
a number of credentials for test servers and other internal
architecture.

Usage of the http://user:pass@host/ pattern has declined significantly
in the last few years
; given that low usage, closing this small
security hole seems quite reasonable.


Preview | Diff

Hard-coding credentials into subresource requests (e.g.
`https://user:pass@host/`) is problematic from a security perspective,
as it's allowed folks to brute-force credentials in the past, enables
session fixation attacks for sites using basic auth, and can allow
attackers access to well-known, poorly-coded devices (such as users'
routers). Moreover, the ability to hard-code credentials leads to
inadvertant leakage via XSS on the one hand, and poor development
practice on the other. Sifting through HTTPArchive, for example, yields
a number of credentials for test servers and other internal
architecture.

Usage of the `http://user:pass@host/` pattern has [declined significantly
in the last few years][1]; given that low usage, closing this small
security hole seems quite reasonable.

[1]: https://www.chromestatus.com/metrics/feature/timeline/popularity/532
[2]: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/lx-U_JR2BF0
@mikewest
Copy link
Member Author

Kicking off another discussion, @annevk. Same issue as #464 regarding top-level vs non. I've called that out with an XXX span; we'll remove that if/when we decide that this kind of thing is worth doing.

@annevk
Copy link
Member

annevk commented Jan 24, 2017

If we do this we can simplify https://fetch.spec.whatwg.org/#concept-http-redirect-fetch as well (the "includes credentials" pieces) since CORS requests are always subresource requests.

@mikewest
Copy link
Member Author

I haven't verified this in Edge, but I'm told that MS hasn't supported this for years: https://support.microsoft.com/en-us/help/834489/internet-explorer-does-not-support-user-names-and-passwords-in-web-site-addresses-http-or-https-urls

@annevk
Copy link
Member

annevk commented Jan 24, 2017

Note also that the way the standard does HTTP authentication at the moment is by using this syntax. So if we can no longer use this syntax, that would also have to change somehow. Search for 401 in https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch.

@mikewest
Copy link
Member Author

It's a bit complicated, so I might be misreading, but I don't think the suggested change breaks that bit of the algorithm. By putting the check at the top of main fetch, we're dealing with the URL that comes from a page (or redirect). The implementation detail of HTTP-network-or-cache fetch doesn't call back into main fetch, but calls into itself (in 24.4).

@annevk
Copy link
Member

annevk commented Jan 24, 2017

Fair. What about XMLHttpRequest? https://xhr.spec.whatwg.org/#the-open()-method also uses this and does call into fetch. Or would you stop honoring those parameters?

@mikewest
Copy link
Member Author

I'd suggest blocking those as well; they're included in the Chrome metric noted above, and they have similar properties from a security perspective. Basically, I think basic/digest auth is ~fine as a browser-mediated mechanism of allowing users to sign into sites and maintain state in some way. I think it's significantly less fine when the credentials are controlled by the page.

mikewest added a commit that referenced this pull request Jan 25, 2017
Currently, requests have a 'destination' property, which provides some
clarity about the type of request that's being made. For navigation
requests, however, we need a little more granularity in order to support
suggestions like #464 and #465. In particular, we need to distinguish
between navigation requests that target a new top-level browsing
context, and those that target nested browsing contexts (as the latter
are treated as subresource requests in the context of some policy
decisions).

This patch adds a new 'target browsing context' property to requests
which will be populated from HTML's navigation algorithm in order to
support these kinds of policies.
@annevk
Copy link
Member

annevk commented Feb 8, 2017

@youennf any thoughts from WebKit on this?

@travisleithead any thoughts from Edge?

@mcmanus @valenting any thoughts from Gecko/Necko?

@mikewest
Copy link
Member Author

mikewest commented Feb 9, 2017

@annevk
Copy link
Member

annevk commented Feb 9, 2017

IE/Edge restricts it to HTTP(S) URLs, which is a little different, though I suspect in practice that only affects FTP.

@mikewest
Copy link
Member Author

mikewest commented Feb 9, 2017

IE/Edge restricts it to HTTP(S) URLs, which is a little different, though I suspect in practice that only affects FTP

Indeed. Also, let's kill FTP (in #464). :)

@valenting
Copy link

Firefox changes tracked in bug 1340200

@annevk
Copy link
Member

annevk commented Feb 17, 2017

@mikewest same questions as for the ftp URL PR. Since Mozilla seems tentatively on board and we've waited over a week, I'm happy with doing this. Will you file bugs against Safari and Edge?

@mikewest
Copy link
Member Author

This one I can certainly write tests for, so let's let me do that before we land the spec patch. I plan to land this change in Chrome after the next branch point, and I'm not sure there's much point upstreaming tests before any implementation passes. Perhaps we can put this on hold for a week or three?

I'll file bugs against Safari and Edge to ensure they're aware. Edge shouldn't have much work to do.

@annevk
Copy link
Member

annevk commented Feb 17, 2017

I'm happy to wait three weeks. We'll also need to rebase as this probably conflicts with the FTP change.

@jeronimoyk
Copy link

jeronimoyk commented Apr 24, 2017

What about the case when receiving error/challenge 401 with (WWW-Authenticate: Basic realm=...) for a URL for which the browser already has the credentials cached.
May be I am wrong, but I think that Chrome is using the same erroneous approach when having the cached credentials - after 401 error received, instead of putting the Authorisation header in the GET retry with the cached credentials (base64 encoded), it is composing a new URL with the credentials exposed in it.

@ssp
Copy link

ssp commented May 14, 2017

I found this via the message

[Deprecation] Subresource requests whose URLs contain embedded credentials (e.g. https://user:pass@host/) are blocked. See https://www.chromestatus.com/feature/5669008342777856 for more details.

Chrome 60.0.3095.5 gave me when visiting a site with Basic-authentication. When loading the resources it contains (which do not have authentication information in their URLs in the markup) Chrome will not load them giving the explanation quoted above.

Like @ykraynov I wonder whether this is what you have in mind here or whether Chrome may be overdoing it here.

@magynhard
Copy link

magynhard commented Jun 9, 2017

I'm developing Selenium Tests with Cucumber. We have a test environment with Basic Authentication.

Unfortunately Chrome completes relative URLs on the Website with the full Authentication URL,
e.g.: /page becomes https://user:password@domain.com/page and is blocked by Chrome.
So many links e.g. for CSS are broken.

I expect, after Chrome has been authentificated by the URL, adding only the URL without authentification information to relative links and not to block them, e.g. https://domain.com/page, as it does with the address bar (hiding the auth information).

In my opinion it doesn't make sense, that Chrome completes URLs with information that is not allowed by itself.

If this case is not considered in the specs, other browsers may implement it the same way in the future. I think we need to specify, that after auth, relative links are completed without authentication.

@MikeN123
Copy link

MikeN123 commented Jun 9, 2017

Same issue here. I have a local bookmark https://user:pass@host.blabla and it doesn't work since all subresource requests are blocked (all using relative URL's). Seems like a bug to me.

@magynhard
Copy link

FYI: Relating to the Chrome Bug, I have created an issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=731618

@ShAnsari
Copy link

When can we expect a fix on this? It has messed up all my test cases.

@magynhard
Copy link

Until the fix is released, you can use the switch

--disable-blink-features=BlockCredentialedSubresources

to run your tests.

@jamesjnadeau
Copy link

jamesjnadeau commented Jul 7, 2017

I have a couple log viewer apps that are behind a vpn that I am now not able to access because of this.
I think the premise of

Usage of the http://user:pass@host/ pattern has declined significantly
in the last few years;

is wrong. Just because you're not getting it in your analytics doesn't mean it's not happening. I personally just don't allow you to collect these analytics, and I'd wager a bet there are many more like me.

How about instead of a breaking change, and pushing your opinions on others, you just issue a warning about it when it happens, and allow the warning to be dismissed?

This is a feature people use, and you just took it away without just cause IMHO.

I think the most frustrating part is you page:
https://www.chromestatus.com/feature/5669008342777856

states:

Consensus & Standardization
Firefox: No public signals
Edge: No public signals
Safari: No public signals
Web Developers: No signals

I'm a Web Developer, and part of a company of 100+ others, we want this feature back without a launch flag. Hope that's a clear enough signal for ya.

@annevk
Copy link
Member

annevk commented Jul 7, 2017

This is the wrong place to complain about Chrome's behavior. You want https://bugs.chromium.org/p/chromium/issues/detail?id=731618 most likely.

@jamesjnadeau
Copy link

jamesjnadeau commented Jul 7, 2017

Thanks @annevk! will go complain there as well

Is this the right place to complain about a change to the browser standard? Because I think getting rid of this feature is a mistake.

So to be perfectly clear, if I auth once, all requests to that domain should use that same auth until my browser session/window closes. This is how browsers have worked for ages, please keep it that way.

@annevk
Copy link
Member

annevk commented Jul 7, 2017

Constructive feedback on the proposed change is welcome, yes. And although HTTP authentication and credentials in a URL are related in some cases (e.g., with XMLHttpRequest), they're also different.

@jamesjnadeau
Copy link

jamesjnadeau commented Jul 7, 2017

I guess I'm a little confused as to what the road block is then. So I'm going to write out what I think is going on, maybe you can steer me in the right direction.

Let's say I have a page requested at http://user:pass@same.domain/ with the following html:

<html>
 <head>
   <link rel="stylesheet" href="/a.css">
   <link rel="stylesheet" href="http://user:pass@other.domain/b.css">
   <link rel="stylesheet" href="http://user2:pass2@same.domain/c.css">
   <script>
     // JS code that adds a link to a style sheet
     addStyleLink('/d.css');
     addStyleLink('http:/user:pass@other.domain/e.css');
     addStyleLink('http://user2:pass2@same.domain/f.css');
   </script>
 </head>
....

Under the proposed rule "Block subresource requests whose URLs include credentials", I think the following would happen:

  1. a.css would be requested with user:pass creds - this is currently broken in chrome?
  2. b.css would not use creds to make the request, this is now blocked?
  3. c.css should be requested with user2:pass2 as the creds - or is this blocked?
  4. d.css will be requested with user:pass creds
  5. e.css request is blocked
  6. f.css request is made with user2:pass2 creds

Not sure how this should all play out in what should/should not be blocked from happening.

I'd prefer, and I think you'd step on a lot less peoples toes, if you just notified people they are doing something wrong, or it's a bad practice. At the end of the day, I think you'd get the results you want(steering people away from this practice), but not block peoples legitimate use(in their eyes) of this feature of the URL spec.

@annevk
Copy link
Member

annevk commented Jul 7, 2017

The bad practice is including credentials in URLs. I'm surprised you have code like that, since as far as I know Internet Explorer (and now Edge) has never supported it (at least not from IE6 onward).

@jamesjnadeau
Copy link

I have code like /a.css to the best of my knowledge. I have urls that I hand out to people like http://admin:pass@staging-site.com that allows a need to know access to a site without too much fuss. Often this is behind a vpn, or is an obscure domain name.

I'm just trying to flesh out what exactly is being suggested here, because it's not clear(to me) what you are blocking.

@annevk
Copy link
Member

annevk commented Jul 7, 2017

If your code doesn't embed credentials in URLs it shouldn't be affected.

@mnot
Copy link
Member

mnot commented Jan 11, 2018

From http://httpwg.org/specs/rfc7230.html#http.uri --

The URI generic syntax for authority also includes a deprecated userinfo subcomponent ([RFC3986], Section 3.2.1) for including user authentication information in the URI. Some implementations make use of the userinfo component for internal configuration of authentication information, such as within command invocation options, configuration files, or bookmark lists, even though such usage might expose a user identifier or password. A sender MUST NOT generate the userinfo subcomponent (and its "@" delimiter) when an "http" URI reference is generated within a message as a request target or header field value. Before making use of an "http" URI reference received from an untrusted source, a recipient SHOULD parse for userinfo and treat its presence as an error; it is likely being used to obscure the authority for the sake of phishing attacks.

Background:

@youennf
Copy link
Collaborator

youennf commented Jan 11, 2018

Safari is currently supporting user info for both top resources and sub resources.
Any redirection URL is currently pruned of userinfo.

I am not sure how backward compatible it is to remove support of userinfo for things like XHR.
It seems safe to change the handling of redirections given how inconsistent the ecosystem is.

mthuurne added a commit to boxingbeetle/softfab that referenced this pull request Jul 12, 2019
Some types of artifacts need sandboxing: for example an HTML report
can contain JavaScript and for both security and robustness reasons
we don't want the report's scripts to have access to the Control
Center state.

One of the states a sandboxed artifact is denied access to is the
session cookie. While that is good to protect a user's login from
being abused by malicious scripting in a report, it also means that
the Control Center has no way to decide whether a report is allowed
to load additional resources, since it doesn't know on behalf of
which user the request is made.

To solve this, a temporary URL namespace is created: 'sandbox/<key>',
where the key is a secure random string. This URL exists long enough
for page loading to finish, but not much longer than that: currently
it is set to half a minute. If the sandbox URL has expired, the CC
redirects the request to a non-sandboxed version, which then verifies
the user's permissions and creates a new sandboxed URL. This way,
expired URLs are automatically renewed.

I tried hiding the sandbox key from the browser's location bar using
the "user:pass@" syntax in the sandboxed URL, but modern web browsers
will only accept that syntax directly from the user, not from a site:

  whatwg/fetch#465

A more secure approach would be to serve artifacts from a separate
origin (scheme + hostname + port), but that would significantly
complicate the setup of a Control Center, so I didn't want to require
that. It might be something to implement as an option in the future,
if there are projects with very strict security requirements.
Base automatically changed from master to main January 15, 2021 07:38
@annevk
Copy link
Member

annevk commented Feb 15, 2021

What's the status of this @mikewest? https://wpt.fyi/results/xhr/send-authentication-competing-names-passwords.htm suggests that XMLHttpRequest still works in Chrome.

@mikewest
Copy link
Member Author

I think we had to roll this back for XHR. I'll try to dig up the details.

@mikewest
Copy link
Member Author

I think we landed in a place where we block subresource loads whose URLs contain userinfo (https://codereview.chromium.org/2651943002), unless either:

a) The subresource is being loaded via XHR (https://codereview.chromium.org/2808753003), or
b) The page on which the subresource is loaded contained userinfo, and it matches the userinfo in the subresource (https://chromium-review.googlesource.com/530308).

@ddragana
Copy link

@mikewest your last comment describes Chrome's behavior for URI that contain a userinfo. Can you confirm what is Chrome's behavior if URI does not contain a userinfo?
Firefox will implement this behavior.
Thanks.

@ddragana
Copy link

@mikewest Can you also share with us what the Chrome behavior in case of

@mikewest
Copy link
Member Author

Hi @ddragana! I'm sorry, but I don't understand the question. The change discussed in this PR was only related to URLs that do contain userinfo, with the carveouts described above. What aspect of Chromium's behavior regarding URLs that don't contain userinfo are you interested in understanding?

@ddragana
Copy link

What I am interested in is slightly related to this PR, but not completely. (I did not have a better place to ask the question therefore I asked here)

Some more explanation:
Both Chrome and Firefox do not show authentication prompt dialog for 3rd party images.
I was wondering if Chrome is showing authentication prompt dialog for other type of 3rd party subresources, e.g. video, style, etc.? If the prompts are allowed, have you tried to disallow it the past?

@domenic
Copy link
Member

domenic commented Sep 30, 2022

I found this code in Chromium https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/fetch_parameters.cc;l=94;drc=a432cd59d51281057ba2a2673ca645a9600bb927 which seems to strip usernames and passwords from some requests... looking at the call sites, it appears the list of requests is somewhat random, including mostly CORS subresources, but also prefetches and preloads?

We are trying to determine whether to emulate this pattern for new CORS subresource requests... if this is in specs somewhere and I missed it, please let me know.

@annevk
Copy link
Member

annevk commented Sep 30, 2022

I suspect what you are observing is an alternative implementation of the "use-URL-credentials flag" that ends up being observable in at least service workers and therefore technically is a specification violation. Probably worthy of a new issue.

@MayyaSunil
Copy link

MayyaSunil commented Aug 4, 2023

Hello @annevk, do you see any problem here?
I am implementing the changes required for this in Gecko.
I guess we need to update these tests as they contain sub-resource loads with credentials?
However, I would like to know if you have plans to merge this before I submit the updates for the tests.
Thank you

@annevk
Copy link
Member

annevk commented Aug 17, 2023

This PR needs rebasing and OP needs to be updated to include (and fill out) https://github.com/whatwg/fetch/blob/main/PULL_REQUEST_TEMPLATE.md.

If that's done and there are tests indicating this is what's implemented in Chromium and planned to be implemented by Gecko I don't see a reason not to merge this.

@MayyaSunil
Copy link

MayyaSunil commented Sep 21, 2023

Hello @mikewest , I am trying to match Firefox's behaviour with Chrome for URLs with credentials described in this PR. It would be great if we could merge this PR and standardize the behavior. If you don't have time to do the remaining tasks mentioned above to merge this, I would be happy to do them for you. Kindly let me know. Thanks

@mikewest
Copy link
Member Author

@MayyaSunil I would be thrilled for you to pick this up and get it landed. It's been a few years since I looked at any of this, but I can certainly help track down behavioral differences if you come across them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet