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

Reconsider enforcing the use of HTTPS for module download #5318

Closed
vdeturckheim opened this issue May 14, 2020 · 13 comments
Closed

Reconsider enforcing the use of HTTPS for module download #5318

vdeturckheim opened this issue May 14, 2020 · 13 comments

Comments

@vdeturckheim
Copy link

vdeturckheim commented May 14, 2020

Deno is marketed as “A secure JavaScript and TypeScript runtime” and the philosophy behind the flags disabling syscalls seems to be “Deno default behavior prevents you from shooting yourself in the foot security wise”.

However, by default, Deno accepts to download libraries using http only. In other words, https is not enforced. There are two issues about this (#1063 and #1064).
The first one, has been dismissed with the argument that: [Deno] “should follow the browser conventions here - using a script tag to import http is allowed.” The other one is still open but has not had any activity in a couple months.

As long as a Deno process instantiates a module with an http import, a “man in the middle attack” is possible:

It means it is possible to tamper with the code of the module that is downloaded. Even if Deno has implemented integrity checks for modules, there are still some key attack vectors.

As I understand, there are still two cases when integrity checks are not enforced: The first download of a lib and the update of a lib. Moreover, even in the cases where integrity checks are enforced, someone could just prevent you from downloading this module, or do something even nastier: send garbage for an infinite time.
In this last case (according to a test I ran on 1.0.0-rc3), Deno will just be stuck forever downloading the module while seeing its RSS increase until something stops it (user or OS). In this case, any dependency (or sub dependency) could selectively prevent Deno apps from installing , reaching actually a Denial of Service case. (Service being, having Deno running the code).

Another attack: someone could eavesdrop on the module that is being downloaded. Eventually, Deno users will use authentication to download certain modules. Over http, the credentials will be transmitted in clear. It is noticeable that the popular (and very cool) package manager @yarnpkg has been having an issue with this (see CVE-2019-5448). It is very possible that someone will actually fill a CVE against Deno on that exact topic at one point.

All of that can be introduced by importing a library in https that would actually import another one using http only.

Overall, It’s pretty easy to fix. It should just “by default, disallow http for module resolution”. Maybe a flag could be used to re-enable it, following what I understand is the philosophy of the project regarding security flags. This would be a breaking change however.

I saw that following the prototype pollution events in the JavaScript world, it was decided to remove Object.prototype.__proto__ (see #4341). Actually, most browsers still support __proto__ (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto).

Here, the Deno team did the right thing, giving up browser compatibility to increase the default security level of the runtime. This is exactly the opposite of the decision taken regarding downloading modules using http only.

IMO, this defeats Deno being “Secure” if browser compatibility is more important than security. Deno is not a web browser. Also, web browsers have benefited from a lot of features regarding security hardening. For instance, security headers https://owasp.org/www-project-secure-headers/. Most of this would not make sense in Deno but they are here to update these behaviors.

(edit) Summary for ease of archieving:

  • current version of the project might be holding an instance of CWE-310 and/or CWE-311 out of similarity with CVE-2019-5448
  • current version of the project might be holding an instance of CWE-770
@pimterry
Copy link

pimterry commented May 14, 2020

Just to repeat the point touched on at the end of #1063: the current behaviour is not the same as standard browser behaviour or conventions.

Browsers will allow you to import HTTP scripts only from HTTP pages, where your entire context is already insecure. HTTPS pages cannot import insecure HTTP scripts due to mixed content rules, because that would be a security risk, and it's only allowed in HTTP pages because it's moot there anyway.

Given that you treat the script context in Deno as initially secure & trusted, importing content should behave like a browser import from a secure context imo, and should reject plain HTTP requests.

This is especially true given that browsers are rapidly moving towards wholesale deprecation of plain HTTP anyway:

@vdeturckheim vdeturckheim changed the title Reconsidering enforcing the use of HTTPS for module download Reconsider enforcing the use of HTTPS for module download May 14, 2020
@Ciantic
Copy link

Ciantic commented May 14, 2020

Many browser does not allow mixing http and https scripts, especially in https site. I think this problem is analogous to mixed content in browsers.

There could be a flag "--allow-mixed-content" or something, and by default it would not allow peer-dependencies to import http-urls.

@nayeemrmn
Copy link
Collaborator

How is this not a duplicate of #1064? You could have added input there.

@vdeturckheim
Copy link
Author

vdeturckheim commented May 14, 2020

@nayeemrmn #1064 is about "Prevent modules imported through https to internally import http modules". Here this is about disabling http altogether by default for module import even for first level modules. At process level.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented May 14, 2020

@vdeturckheim 🤷‍♂️ An https module importing an http one is an actual problem. Specifying an http main module is opting in at CLI. I don't know how this is pointing out an inconsistency with the security flags which exist specifically to opt in to potentially shooting yourself somewhere.

There is a reason #1064 talks about specifically about downgrading to http even though the reasoning is exactly the same.

@vdeturckheim
Copy link
Author

vdeturckheim commented May 14, 2020

@nayeemrmn

I don't know how this is pointing out an inconsistency with the security flags which exist specifically to opt in to potentially shooting yourself somewhere.

well, importing something in http is shooting yourself in the foot. Having a default behavior to prevent it and have a flag to "opt in to potentially shooting yourself" would actually be to disable http by default and allow it behind a flag right?

Importing executable code through https means you let the owner of the remote server run code on your machine.

Importing executable code through http means you let anyone on the internet run code on your machine.

@nayeemrmn
Copy link
Collaborator

I don't know how this is pointing out an inconsistency with the security flags which exist specifically to opt in to potentially shooting yourself somewhere.

well, importing something in http is shooting yourself in the foot. Having a default behavior to prevent it and have a flag to "opt in to potentially shooting yourself" would actually be to disable http by default and allow it behind a flag right?

No. Every script that uses http has a clear http in the URL (again, assuming no downgrades!). Not every script that depends on malicious runtime behaviour has "malicious-script.ts" in the name. That's the difference and why we need the permission flags.

@vdeturckheim
Copy link
Author

I think we reached a disagreement here. Right now, the project is aiming at browser compatibility without providing the same level of security expectations as browsers (some of them because they don't make sense). The issue is coming from this disalignment and there might be other similar problems because of that exact root cause.

Even if it was only about downgrading, the solution would actually be the same. Changing the default value and place a flag.

@nayeemrmn
Copy link
Collaborator

Right... so it's a duplicate other than an unnecessary (imo) extension with the same solution.

@vdeturckheim
Copy link
Author

vdeturckheim commented May 14, 2020

IMO, this is not a duplicate because #1064 is a subcase of this.

But this exact part about debating if two security issues that have the same solution are the same is probably taking us to a place where we don't discuss the real point: There are multiple attack vectors for which mitigation is not possible for the time being while being possible in browsers.

@RomainLanz
Copy link

RomainLanz commented May 19, 2020

Right... so it's a duplicate other than an unnecessary (imo) extension with the same solution.

In any case, duplicates or not, this is a real security issue that Deno should take care off.

Discord Conversation

As said in the Discord, I see no point adding flags (i.e: --allow-hrtime) for "security reason" while this is not sorted.

@nayeemrmn
Copy link
Collaborator

I see no point adding flags (i.e: --allow-hrtime) for "security reason" while this is not sorted.

Again, this is a ridiculous point. You're contributing nothing.

@vdeturckheim
Copy link
Author

vdeturckheim commented May 20, 2020

Reading the last messages of the screenshot I am under the impression that this issue might not be considered in its whole or might be dismissed quickly.
I don't feel comfortable keeping it open and will close the issue.

I have updated the original message to include a small summarizing conclusion to ease potential future references in case these topics are discussed down the road.

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

No branches or pull requests

5 participants