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

security: mixed content blocking subleties #5808

Open
johnmellor opened this issue May 24, 2020 · 2 comments
Open

security: mixed content blocking subleties #5808

johnmellor opened this issue May 24, 2020 · 2 comments
Labels
bug Something isn't working correctly cli related to cli/ dir

Comments

@johnmellor
Copy link

Many thanks @bartlomieju for adding an initial implementation of mixed content blocking (#1064).

I noticed a few subtle security issues with the logic in pull #5680, compared to how browsers do this which has been gradually refined over the years:

1. Redirect handling

#5680 blocks https modules from importing http urls, but it doesn't seem to prevent https modules from importing https urls that redirect to http urls (or https urls that redirect to http urls that redirect back to https urls).

Browsers block all three (equally dangerous) cases, since the Mixed Content spec's "Insecure Content in Secure Contexts" section notes that user agents must do a mixed content check before every fetch of a subresource, and the Fetch standard handles redirects by triggering a nested fetch and hence an additional mixed content check per redirect.

2. Response handling

Another subtle case is https urls whose TLS encryption is bad. The Fetch standard's "HTTP-network fetch" algorithm recommends

User agents are strongly encouraged to only succeed HTTPS connections with strong security properties and return network errors otherwise.

Presumably that's what Deno would already do if a MITM attacker sends you a self-signed certificate for an https import? (https://badssl.com/ by the Chrome & Firefox security teams might be helpful for manually testing a variety of different kinds of SSL error.)

3. Import protocol

#5680 only blacklists the http protocol as insecure. But,

  1. this unnecessarily blacklists local origins like "http://127.0.0.1" that browsers do not consider to be mixed content (as the connection from the browser to a local server does not go over the network).
  2. the codepath that loads modules might now or in the future support other insecure protocols such as ftp. So it'd be safer to have a whitelist of secure protocols, and block everything else.

You can solve both of these by using the whitelist defined for the Mixed Content spec's a priori authenticated URL algorithm, which roughly allows:

  • any origin with the {'https', 'wss', 'file', 'data'} schemes
  • any origin whose host is localhost - but there are important subtleties here in the Secure Contexts spec's "Is origin potentially trustworthy?" algorithm, since localhost can sometimes hit the network(!).
  • The origin of blob: and filesystem: URLs is the origin of the context in which they were created. Therefore, blobs created in a trustworthy origin will themselves be potentially trustworthy.

4. Referrer protocol

#5680 only blocks mixed content if the referrer's scheme is https. But now or in the future there are likely to be other secure contexts from which mixed content should be blocked, for example if an https module imports a module using a data/blob/filesystem/etc URL (as requested in #5059/#2726/#5683), and that module in turn attempts to import an http module, that should count as mixed content and be blocked.

The Mixed Content spec's "Does settings prohibit mixed security contexts?" algorithm defines this to be based on the context's HTTPS state, which in turn is set by the Fetch standard's "scheme fetch" algorithm when loading the context. Roughly, it will be "modern" (or "deprecated") for resources retrieved over HTTPS, "none" for responses retrieved over FTP, and for the {"about", "blob", "data"} schemes they copy the HTTPS state of the context that made the request - so in the example above the data/blob/filesystem URL would have an HTTPS State of "modern" (or "deprecated") and so mixed content would be blocked.

Unfortunately "scheme fetch" doesn't specify how to calculate HTTPS state for the file scheme:

For now, unfortunate as it is, file URLs are left as an exercise for the reader.

but the safest route would probably be to default to blocking mixed content in them, unless a command line flag like "--unsafe-allow-mixed-content-in-files" is passed (something similar was proposed in #5318). Note that http://127.0.0.1 URLs shouldn't count as mixed content (see Import protocol above), so such a flag wouldn't be necessary for local development, only when importing modules hosted on non-https intranet servers. And it would go a long way toward avoiding users accidentally putting themselves at risk by including http modules into Deno processes they've granted powerful permissions to.

5. Workers

It wasn't clear to me from the code whether #5680 also affects loading Workers from URLs, but presumably the same mixed content blocking should apply there too, and it would be nice to share the code between both places.

@sholladay
Copy link

Insecure HTTP should be unsupported, period. Deno has the opportunity to take a hard stance on this and enforce good security practices to improve the state of the web. Browsers are slowly moving in this direction, even breaking things to a certain degree. But Deno doesn't have to worry about supporting old applications. Please, keep things simple and secure by only supporting HTTPS.

@Lonniebiz
Copy link

Lonniebiz commented May 24, 2020

If you do decide to block mixed protocols on imports, then please also provide a flag that allows you to ignore those blocks.

While I certainly don't want a man in the middle to modify a module I'm importing during its transfer (inserting code that deletes my entire file system), at the same time, I'd appreciate the authority, flexibility and convenience (as the developer) of being able to make that decision for myself and not to have it be made impossible by Deno.

SSL related blocks, in the browser, have been a royal pain for me in intranet apps that use self-sign certificates.

@ry ry added bug Something isn't working correctly cli related to cli/ dir labels May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly cli related to cli/ dir
Projects
None yet
Development

No branches or pull requests

4 participants