Refine csp checks for `unsafeEval`. #11933

jdalton · 2015-05-22T23:37:47Z

This PR refines the csp checks for function compilation because the default policy in WWA (Windows Web Apps) allows unsafe-eval. This would give a ~30% boost when evaluating expressions.

googlebot · 2015-05-22T23:37:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

jdalton · 2015-06-01T23:58:13Z

src/Angular.js

+    active = false;
+  }
+  return (unsafeEval.isActive_ = active);
+};


Having unsafeEval split out of csp may make it chatty in the console. I can add guards to it if you all would like.

jdalton · 2015-06-05T21:10:52Z

Corporate CLA: Microsoft.

petebacondarwin · 2015-06-08T09:55:35Z

src/Angular.js

-    } catch (e) {
-      active = true;
-    }
+    active = !unsafeEval();


this whole bit could be changed to

active = active || !unsafeEval();

Nice! Updated.

jdalton · 2015-06-09T18:25:17Z

Any other concerns or issues?

IgorMinar · 2015-06-09T19:11:47Z

@jdalton can you please explain how this improves anything on Windows?

It looks like you are trying to force non-csp mode even when developer opts in to it, which changes the semantics of the opt-in.

What's the real world scenario you are trying to fix?

jdalton · 2015-06-09T19:37:49Z

@IgorMinar Currently the csp check in Angular assumes that csp means unsafe eval is not allowed, however in Windows 10 WWA the default csp policy allows unsafe eval.

The benefit is to give a perf boost out of the box for those using Angular in WWAs.

IgorMinar · 2015-06-09T20:16:15Z

The only side effect of opting into the CSP mode is that we don't use function constructor. So just by not opt-ing into the CSP mode you get the perf benefit you are looking for.

As far as I can tell the only impact of your change is that if someone explicitly opts into the CSP mode via ng-csp directive, we will ignore it and still use function constructor in $parse.

IgorMinar · 2015-06-09T20:30:49Z

There is one more place where csp mode plays any role and that's when including the default stylesheet. I assume that on WWA this would throw, in which case you'd want to prevent that via ng-csp, which then forces you down the slow path in $parse. Is that the case?

jdalton · 2015-06-09T20:33:41Z

if someone explicitly opts into the CSP mode via ng-csp directive, we will ignore it and still use function constructor in $parse.

It's not ignoring CSP, it's using unsafeEval if it's allowed by the users CSP policy which is a better check than blanket CSP "on" or "off".

There is one more place where csp mode plays any role and that's when including the default stylesheet. I assume that on WWA this would throw,

I don't think there's an issue with local stylesheets. How is it loading the default stylesheet? Inline scripts are still not allowed by the Win 10 WWA default csp policy.

IgorMinar · 2015-06-09T20:47:18Z

We prepend an inline stylesheet to the head while booting, unless ng-csp is found in the doc.

jdalton · 2015-06-09T20:51:34Z

unsafe-inline is allowed by the Win 10 default WWA CSP policy too.

IgorMinar · 2015-06-09T20:52:32Z

In that case then just don't use ng-csp in your main template and you are all set. no?

petebacondarwin · 2015-06-09T20:59:44Z

Sorry, slipped on the wrong button there

IgorMinar · 2015-06-09T21:03:33Z

if for whatever reason we need $sniffer.csp to distinguish between unsafe-inline and unsafe-eval, it would be preferable to remove the need to inline the stylesheet (and instead use https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule to add the default styles to the document).

afterwards ng-csp would only control if we attempt to autodetect unsafe-eval or not.

the primary reason for ng-csp was to avoid autodetection which relies on throwing exceptions which is not nice and interferes with debugging.

but from what you described, you actually don't need to change angular in order to get your performance benefit.

jdalton · 2015-06-09T21:09:00Z

but from what you described, you actually don't need to change angular in order to get your performance benefit.

Outside of default CSP policies devs can provide their own CSP policies in apps or sites that while they may restrict inline stylesheets can allow unsafeEval. This would allow devs who still indicate CSP to be able to get a boost if their policies allow unsafeEval while avoiding other CSP gotchas Angular may run into now or in the future.

petebacondarwin · 2015-06-09T21:21:50Z

I think we can fix this in a cleaner way.

Either we remove the inline stylesheet from Angular (using insertRule) so that ng-csp only refers to unsafeEval or make ng-csp more configurable (by providing a value to the directive).

<html ng-csp-inline-style ng-csp-unsafe-eval>

or

<html ng-csp="inline-style;unsafe-eval">

The first option is preferable to us as it means that we don't have to worry about people tweaking their CSP config in subtle ways that we don't support.

petebacondarwin · 2015-06-10T09:34:47Z

Closing for now. @jdalton if you would like to progress this along the lines of one of the above solutions, then please open a new PR. I would suggest trying to fix it by removing our dependency on inline styles first.

jdalton · 2015-06-10T17:05:44Z

@petebacondarwin I can dig it. ~~Could you point me in the right direction?~~ Nm, found it 🔎

Update:
It looks like insertRule won't work with style-src 'self'. Is there something else you had in mind?

jdalton · 2015-06-15T16:34:08Z

Ping @petebacondarwin. If the style issue can't be resolved then I think this PR still has merit.

jdalton · 2015-06-25T16:42:58Z

Ping again. I'd like to continue this discussion since there doesn't seem like there's a way to avoid the css csp restriction.

petebacondarwin · 2015-06-26T05:59:15Z

@jdalton sorry about that - got a bit distracted by AngularU. Can we continue this next week? I am flying back to the UK tomorrow.

jdalton · 2015-06-26T06:42:51Z

Can we continue this next week?

Sure thing

realityking · 2015-07-03T10:43:32Z

To be honest, I haven't really understood the aim of this PR.

From what I read here, WWAs have a default CSP policy which includes both unsafe-eval for javascript and unsafe-inline for Stylesheets. Since these are the only two directives which affect AngularJS, I'm missing what needs to change here. If people manually set ng-csp they're gonna sufffer some performance degradation, but do we really need to hold hands?

Slightly off topic: The best solution would be if browsers exposed the current security policy. This was once specified but for some reason it's absent from newer drafts.

jdalton · 2015-07-03T14:04:09Z

@realityking See #11933 (comment).

If people manually set ng-csp they're gonna sufffer some performance degradation, but do we really need to hold hands?

Yep. Having CSP restrictions is fine or getting rid of the directives (ng-csp), if doable, is great. The issue is the assumption that CSP is an auto-out for compilation. In this case the assumption is incorrect and needlessly costing 30%. The PR attempts to break down the checks to be a bit more granular. I think the change is a positive one. The discussion on styles would be a good follow-up PR.

petebacondarwin · 2015-07-03T21:08:00Z

So the deal is that Angular does two things that can be locked down by CSP:

unsafe eval
unsafe inline

Currently if you don't specify the ng-csp directive then Angular just tests for whether eval-ing a string throws an error. This causes an unwanted error message in the console.
To prevent this the developer can specify the ng-csp directive which tells Angular not to do either of the two things above.

@jdalton would like to refine this so that we can lock down only one or the other of these two things.

The SHA idea would mean that we don't need to lock down the inline styles issue, which means that Angular only need to lock down the unsafe eval, which could be done with ng-csp.

If this doesn't work then we should make ng-csp configurable to specify which of the two rules we need to lock down.

I would like to test the SHA approach on the browsers that we support before going with the second option.

It is worth noting that the hash based approach is part of the W3C Editor's Draft https://w3c.github.io/webappsec/specs/content-security-policy/#source-list-valid-hashes

petebacondarwin · 2015-07-04T07:25:10Z

A final option that I would consider is to simply have the option of a custom build that would exclude restricted aspects. I believe that @IgorMinar has generally been against this idea.

realityking · 2015-07-04T10:17:25Z

Yeah the custom build for the CSS was something I submitted way back: #8459

From the other options, I think I prefer this one:

<html ng-csp-inline-style ng-csp-unsafe-eval>

petebacondarwin · 2015-07-05T12:02:59Z

I think it would be more intuitive if it was:

<html ng-csp-no-inline-style ng-csp-no-unsafe-eval>

petebacondarwin · 2015-07-05T12:05:33Z

Here is another idea:

How about we use the fact that CSP rules can be defined as meta tags in the HTML. Angular will parse the meta tags for CSP rules and work out whether it is allowed to do inline-style and/or unsafe-eval.

This may require rules to be duplicated, both on the server and in the HTML but is not requiring specialised ng-csp style directives.

petebacondarwin · 2015-07-13T13:17:47Z

:-( I did some testing and indeed the SHA CSP header for styles doesn't work on

Safari 8.0.7
Firefox 34.0.5

So this is not a viable strategy. Although I think it was a nice idea.

jdalton · 2015-07-13T17:23:37Z

Ok, so for the unsafe eval side of things is there anything else to do/tweak?

petebacondarwin · 2015-07-13T19:29:33Z

We need to have a configurable csp sniffer value. I see that there are four options:

different builds of angular for different CSP configurations
configuration via the value of the ng-csp="..." attribute
configuration via new directives, e.g. ng-csp-no-unsafe-eval etc
configuration via standard CSP meta tags, e.g. <meta http-equiv="Content-Security-Policy" content="style-src \'self\'>

I would like to use the last one because, a) it doesn't require multiple build files, b) it is a standard way of specifying CSP directives anyway, and so would make sense to someone reading the HTML who doesn't know about angular.

What do people think?

petebacondarwin · 2015-07-13T20:30:52Z

After a discussion with @btford I am going to knock up a PR with the 4th idea

petebacondarwin · 2015-07-13T21:36:36Z

Actually, I began to implement that and found that there are so many potential combinations of directive rules that we would need to be able to parse it added far too much code.

Instead, I am going to go with the following:

You can choose from the following declarations in your application:

No declaration means that Angular will assume that you can do inline styles, but it will do a runtime check for unsafe-eval. [This is the same as we have currently]

<body>

A simple ng-csp (or data-ng-csp) attribute will tell Angular not to do either the inline styles or the unsafe eval. [This is the same as we have currently]

<body ng-csp>

Providing a value of allow-unsafe-eval will tell Angular that it should be able to do unsafe eval; there will be no runtime check. [This is new]

<body ng-csp="allow-unsafe-eval">

Providing a value of allow-unsafe-inline will tell Angular that it should be able to add the inline style; there will be no runtime check. [This is new]

<body ng-csp="allow-unsafe-inline">

lgalfaso · 2015-07-14T08:26:42Z

@petebacondarwin what would happen if tomorrow there is another CSP parameter we would like it to be configurable? Are we going to keep on adding things to the ng-csp attribute? What would happen with the proposal if someone does ng-csp="allow-unsafe-inline;allow-unsafe-eval"?

petebacondarwin · 2015-07-14T13:26:07Z

@lgalfaso:

We only care about CSP rules that we actively break inside core, which are doing eval and injecting inline styles. If we started doing something else in core, which would break a CSP rule then we would need to add an option to deactivate that code if the application developer wanted to have that rule turned on.

In the scenario described above ng-csp="allow-unsafe-inline;allow-unsafe-eval" would say that we expect there to be no CSP rules that block eval or inline styles, so Angular will go ahead and use them. This is pretty much the same as not using the ng-csp attribute at all, except, in this case, we wouldn't even do the runtime check as we have explicitly said that we support unsafe-eval.

That being said I am wondering whether the rules should be the other way around, i.e. the two options would be:

no-inline-style: this would stop Angular from injecting the styles inline (and would require the app developer to link to an external CSS file.
no-unsafe-eval: this would stop Angular from optimising $parse with unsafe eval of strings.

Then you would have the following options for ng-csp:

No declaration means that Angular will assume that you can do inline styles, but it will do a runtime check for unsafe-eval. [This is the same as we have currently]

<body>

A simple ng-csp (or data-ng-csp) attribute will tell Angular not to do either the inline styles or the unsafe eval. [This is the same as we have currently]

<body ng-csp>

This tells Angular that we must not use eval, but that we can inject inline styles.

<body ng-csp="no-unsafe-eval">

This tells Angular that we must not inject styles, but that we can run eval.

<body ng-csp="no-inline-style">

This tells Angular that we must not inject styles nor use eval, which is the same as an empty: ng-csp

<body ng-csp="no-inline-style;no-unsafe-eval">

If we add a new feature to Angular that potentially breaks a CSP rule then we would add a switch to turn it off.

jdalton · 2015-07-14T16:56:21Z

@petebacondarwin This sounds reasonable to me.
Can you post back a link to your PR once you have it up?

I'll close this one in favor of your PR.

petebacondarwin · 2015-07-14T18:25:12Z

Thanks. I will have it ready in a few hours - just going through the tests and docs

There are two different features in Angular that can break CSP rules: use of `eval` to execute a string as JavaScript and dynamic injection of CSS style rules into the DOM. This change allows us to configure which of these features should be turned off to allow a more fine grained set of CSP rules to be supported. Closes angular#11933 Closes angular#8459

petebacondarwin · 2015-07-14T19:44:40Z

Here you go @jdalton: #12346

jdalton · 2015-07-14T19:55:10Z

Thanks, will check it out

There are two different features in Angular that can break CSP rules: use of `eval` to execute a string as JavaScript and dynamic injection of CSS style rules into the DOM. This change allows us to configure which of these features should be turned off to allow a more fine grained set of CSP rules to be supported. Closes angular#11933 Closes angular#8459

There are two different features in Angular that can break CSP rules: use of `eval` to execute a string as JavaScript and dynamic injection of CSS style rules into the DOM. This change allows us to configure which of these features should be turned off to allow a more fine grained set of CSP rules to be supported. Closes #11933 Closes #8459 Closes #12346

There are two different features in Angular that can break CSP rules: use of `eval` to execute a string as JavaScript and dynamic injection of CSS style rules into the DOM. This change allows us to configure which of these features should be turned off to allow a more fine grained set of CSP rules to be supported. Closes angular#11933 Closes angular#8459 Closes angular#12346

googlebot added the cla: no label May 22, 2015

jdalton reviewed Jun 1, 2015
View reviewed changes

petebacondarwin reviewed Jun 8, 2015
View reviewed changes

Refine csp checks for unsafeEval.

1adb6bc

petebacondarwin closed this Jun 9, 2015

petebacondarwin reopened this Jun 9, 2015

petebacondarwin closed this Jun 10, 2015

petebacondarwin reopened this Jun 26, 2015

petebacondarwin modified the milestones: 1.4.2, 1.4.3 Jul 6, 2015

jdalton closed this Jul 14, 2015

petebacondarwin mentioned this pull request Jul 14, 2015

fix(ngCsp): allow CSP to be configurable #12346

Merged

Refine csp checks for unsafeEval. #11933

Refine csp checks for unsafeEval. #11933

Conversation

jdalton commented May 22, 2015

googlebot commented May 22, 2015

jdalton Jun 1, 2015

Choose a reason for hiding this comment

jdalton commented Jun 5, 2015

petebacondarwin Jun 8, 2015

Choose a reason for hiding this comment

jdalton Jun 8, 2015

Choose a reason for hiding this comment

jdalton commented Jun 9, 2015

IgorMinar commented Jun 9, 2015

jdalton commented Jun 9, 2015

IgorMinar commented Jun 9, 2015

IgorMinar commented Jun 9, 2015

jdalton commented Jun 9, 2015

IgorMinar commented Jun 9, 2015

jdalton commented Jun 9, 2015

IgorMinar commented Jun 9, 2015

petebacondarwin commented Jun 9, 2015

IgorMinar commented Jun 9, 2015

jdalton commented Jun 9, 2015

petebacondarwin commented Jun 9, 2015

petebacondarwin commented Jun 10, 2015

jdalton commented Jun 10, 2015

jdalton commented Jun 15, 2015

jdalton commented Jun 25, 2015

petebacondarwin commented Jun 26, 2015

jdalton commented Jun 26, 2015

realityking commented Jul 3, 2015

jdalton commented Jul 3, 2015

petebacondarwin commented Jul 3, 2015

petebacondarwin commented Jul 4, 2015

realityking commented Jul 4, 2015

petebacondarwin commented Jul 5, 2015

petebacondarwin commented Jul 5, 2015

petebacondarwin commented Jul 13, 2015

jdalton commented Jul 13, 2015

petebacondarwin commented Jul 13, 2015

petebacondarwin commented Jul 13, 2015

petebacondarwin commented Jul 13, 2015

lgalfaso commented Jul 14, 2015

petebacondarwin commented Jul 14, 2015

jdalton commented Jul 14, 2015

petebacondarwin commented Jul 14, 2015

petebacondarwin commented Jul 14, 2015

jdalton commented Jul 14, 2015

Refine csp checks for `unsafeEval`. #11933

Refine csp checks for `unsafeEval`. #11933