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

CSP: defaultSrc should not be required #237

Closed
jnardone opened this issue Aug 4, 2020 · 24 comments · Fixed by #278
Closed

CSP: defaultSrc should not be required #237

jnardone opened this issue Aug 4, 2020 · 24 comments · Fixed by #278

Comments

@jnardone
Copy link

jnardone commented Aug 4, 2020

The new CSP module says it is a lot less restrictive on policy definition, but it added some new restrictions.

defaultSrc now apparently required, though if you follow guidelines like Google's strict CSP, it is not necessary. https://csp.withgoogle.com/docs/strict-csp.html

If you read the specifications for V2 https://www.w3.org/TR/CSP2/ and V3 https://www.w3.org/TR/CSP3/ there is no mention that default-src is required.

But with the latest Helmet, now I must define a default even if I don't want to use it. Without it I get "Content-Security-Policy needs a default-src but none was provided"

@EvanHahn
Copy link
Member

EvanHahn commented Aug 4, 2020

My understanding, which may be wrong, is that a missing default-src directive is the same as default-src *. I wanted to make sure that Helmet's users explicitly opted into this behavior because I feel that it's a little dangerous.

Would you be okay setting default-src to *? (Do I misunderstand the way this directive works?)

As another option, you can avoid Helmet's CSP module entirely to get more control. Here's a quick sketch of what that could look like:

const contentSecurityPolicy = [
  "script-src 'self' example.com",
  "style-src 'self'",
  // ...
].join(";");

app.use((req, res, next) => {
  res.setHeader("Content-Security-Policy", contentSecurityPolicy);
  next();
});

I'm open to reverting this behavior but want to learn more before I do so.

@jnardone
Copy link
Author

jnardone commented Aug 5, 2020

I mean, I'm using other parts of helmet now, would rather keep using it (though the other change where you can no longer use a function inside a directive, e.g. for nonce generation, is a step backwards as well I feel.) I realize I can set the headers myself.

A * for default-src does not cover everything; you would still have to explicitly also allow things like data: and blob: and some of the unsafe variants. So, it's a bit more complicated to explicitly declare what the implicit "no" policy would be. (Looking at https://stackoverflow.com/questions/35978863/allow-all-content-security-policy as help for the reference.)

@EvanHahn
Copy link
Member

EvanHahn commented Aug 5, 2020

A * for default-src does not cover everything; you would still have to explicitly also allow things like data: and blob: and some of the unsafe variants. So, it's a bit more complicated to explicitly declare what the implicit "no" policy would be. (Looking at https://stackoverflow.com/questions/35978863/allow-all-content-security-policy as help for the reference.)

I didn't realize that—thanks for sending.

I made a judgment call here: I want people to explicitly say "I'm doing something dangerous". This was inspired, in part, by React's dangerouslySetInnerHTML behavior.

Of course, that's subjective. Is it truly dangerous to omit a default-src? That depends on a variety of factors, and it sounds like you're a clear example of a case where my judgment call was wrong. However, I hope the ability to disable the Helmet's CSP middleware lets you get around my potentially-poor judgement.

You probably already know this, but for anyone else running into this problem, here's what you could do:

app.use(helmet({
  contentSecurityPolicy: false,
}));

app.use(myCspMiddleware);

I'm willing to budge on this, but given that there's a hopefully-not-too-horrible workaround, I'd want to see more support for this before I made the change.

...you can no longer use a function inside a directive, e.g. for nonce generation, is a step backwards as well I feel.

Another judgment call on my part, which we should discuss further in a separate issue. A summary:

I got a bunch of issues about conditional middleware usage in various forms, but it was tricky to do holistically. For example, should all of CSP's directives be a function, or should each directive be a function? Should reportOnly accept a function? What if you wanted to conditionally disable CSP entirely? What about all of Helmet's other middleware?

My solution was to get out of the business of conditional middleware entirely, and rely on documentation. You can see this wiki page which shows how to do this, which I hope is helpful.

@EvanHahn
Copy link
Member

...you can no longer use a function inside a directive, e.g. for nonce generation, is a step backwards as well I feel.

I've changed my mind on this part and plan to add this back. You can try out the release candidate today (npm install helmet@4.1.0-rc.1), and follow along with the release at #245. (This nonce generation stuff is unrelated to this issue, so let's discuss it on #243.)

@nax3t
Copy link

nax3t commented Aug 15, 2020

As another option, you can avoid Helmet's CSP module entirely to get more control. Here's a quick sketch of what that could look like:

const contentSecurityPolicy = [
  "script-src 'self' example.com",
  "style-src 'self'",
  // ...
].join(";");

app.use((req, res, next) => {
  res.setHeader("Content-Security-Policy", contentSecurityPolicy);
  next();
});

I'm open to reverting this behavior but want to learn more before I do so.

Perfect, thanks!
Adding a lot of urls can get messy in a single line string so I included some additional code for including urls in an array:

// Additional configuring of CSP to mitigate errors from Helmet default settings
// read more here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy
const scriptSrcUrls = [
  'https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js',
  'https://api.tiles.mapbox.com/mapbox-gl-js/v0.51.0/mapbox-gl.js',
  'https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-geocoder/v2.3.0/mapbox-gl-geocoder.min.js',
  'https://kit.fontawesome.com/7870957ffd.js',
  'https://code.jquery.com/jquery-3.3.1.slim.min.js',
  'https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.7/umd/popper.min.js'
];
const styleSrcUrls = [
'https://kit-free.fontawesome.com/releases/latest/css/free.min.css',
'https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css',
'https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-geocoder/v2.3.0/mapbox-gl-geocoder.css',
'https://api.tiles.mapbox.com/mapbox-gl-js/v0.51.0/mapbox-gl.css'
];
const contentSecurityPolicy = [
  "script-src 'unsafe-inline' 'self' " + scriptSrcUrls.join(' '),
  "style-src 'self' " + styleSrcUrls.join(' '),
  "worker-src 'self' blob:" // specific to scripts used in my app, not necessarily something you need
].join(';');

app.use((req, res, next) => {
  res.setHeader('Content-Security-Policy', contentSecurityPolicy);
  next();
});

I could probably write an additional npm package to allow for easy inclusion of script/style source urls, unless you felt like it could be added directly to Helmet.

e.g.,

const cspConfig: {
  scriptSrcUrls: [
    'https://somecdn.com/somescript.js',
    // ...
  ],
  styleUrls:  [
    'https://somecdn.com/somestyle.css',
    // ...
  ],
}
app.use(helmet(cspConfig));

@molaeiali
Copy link

I'm loading a simple apiDoc static page on express with helmet 4.1.0 and getting this error with csp enabled on helmet

Content Security Policy: Couldn’t process unknown directive ‘script-src-attr’
Content Security Policy: The page’s settings blocked the loading of a resource at eval (“script-src”).

Don't konw if it's related or not. In version 3.22.1 of helmet it wasn't a problem.

Screenshot from 2020-08-16 22-45-39

@EvanHahn
Copy link
Member

@jnardone I think we've solved your initial problem, but let me know if that's wrong and we can reopen.

@molaeiali Your issue seems unrelated to this one. Would you mind creating a new issue?

@molaeiali
Copy link

Of course, thanks

@nax3t
Copy link

nax3t commented Aug 17, 2020

Just now seeing helmet.contentSecurityPolicy(options)

Thanks!

@jnardone
Copy link
Author

jnardone commented Dec 2, 2020

@EvanHahn bringing this one back up. Being forced to define a defaultSrc breaks recommendations on v3 policy definition.

If you follow something like https://csp.withgoogle.com/docs/strict-csp.html then you can't do this through helmet CSP -- you don't WANT a default-src because you don't care about enforcement for most categories.

I want to use helmet CSP; I know I could just write my own header, but we use helmet everywhere else and I like the declarativity of it. But I can't define the google recommended strict CSP policy because of defaultSrc. I should be able to say "i only care about these rules".

Using a defaultSrc of '*' does not work as it still applies to style-src, which we don't want ANY rules to apply to.

@jnardone
Copy link
Author

jnardone commented Dec 2, 2020

But I did want to clarify an earlier comment: a missing default-src directive is the same as default-src * <-- this is definitely NOT true, at least in terms of Chrome and likely other browsers.

@EvanHahn
Copy link
Member

EvanHahn commented Dec 2, 2020

@jnardone When does default-src * behave differently from omitting the directive?

@jnardone
Copy link
Author

jnardone commented Dec 2, 2020

I saw it immediately on inline styles:

Refused to apply inline style because it violates the following Content Security Policy directive: "default-src *". Either the 'unsafe-inline' keyword, a hash ('sha256-Y9v1MZrln1N8aPBY5lmpxYKwFkcp/nyBMMEnn7WFjuw='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

@EvanHahn
Copy link
Member

EvanHahn commented Dec 3, 2020

That makes sense. How would you feel about an API like this?

app.use(
  helmet.contentSecurityPolicy({
    directives: {
      defaultSrc: null,
      // ...
    },
  })
);

This would require you to explicitly disable default-src.

@jnardone
Copy link
Author

jnardone commented Dec 3, 2020

@EvanHahn yes, that would work. i don't miind if the default is set, but i need a way to be able to unset it.

@EvanHahn
Copy link
Member

EvanHahn commented Dec 4, 2020

I'll plan to add this soon, ideally in the next few days.

@EvanHahn
Copy link
Member

EvanHahn commented Dec 8, 2020

Haven't gotten to this yet, apologies.

@EvanHahn
Copy link
Member

Just put up #278, a change that should address this. If it looks good, I'll merge and deploy soon, probably this weekend or next.

@EvanHahn
Copy link
Member

@jnardone Actually, what happens if you set default-src to ["*", "unsafe-inline"]? Does that solve your problem?

@jnardone
Copy link
Author

jnardone commented Dec 10, 2020 via email

@EvanHahn
Copy link
Member

But isn't the default to allow everything, if you don't supply it? I'm probably misunderstanding...

@jnardone
Copy link
Author

The spec is confusing and vague around defaultSrc so I'm loathe to rely on it or around trying to emulate the lack of a defaultSrc. https://security.stackexchange.com/questions/240445/what-is-the-behaviour-of-csp-if-default-src-not-specified seems to back that up.

Basically: if the goal of this module is to provide a way to express different types of policies, then one way that should be possible is to explicitly NOT set a default-src policy directive.

@EvanHahn
Copy link
Member

EvanHahn commented Dec 13, 2020 via email

@EvanHahn
Copy link
Member

This has been released in helmet@4.3.0. Here's how you disable default-src:

app.use(
  helmet.contentSecurityPolicy({
    directives: {
      defaultSrc: helmet.contentSecurityPolicy.dangerouslyDisableDefaultSrc,
      // ...
    },
  })
);

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

Successfully merging a pull request may close this issue.

4 participants