Amphtml: Validator: transformed AMP does not support script nonce
I've noticed a few cases were AMP Optimizer would turn a valid AMP doc into invalid AMP when the original file defined script nonces. The validation error is triggered by the presence of transformed="self;v=1". Is this intentional?
For example, this file is not considered valid:
<!DOCTYPE html>
<html amp lang="en" i-amphtml-layout transformed="self;v=1">
<head>
<script async nonce="484fee01e48047fa23773ddcbc3feb0a" custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
==> The attribute 'nonce' may not appear in tag 'amp-experiment extension .js script'.
but it's non-transformed version is:
<!DOCTYPE html>
<html amp>
<head>
<script async nonce="484fee01e48047fa23773ddcbc3feb0a" custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
...
//cc @honeybadgerdontcare @Gregable
sebastianbenz
All 14 comments
tl;dr: The optimizer should probably strip nonces, the same way that the packager does: https://github.com/ampproject/amppackager/blob/e4bf0430ba152cfe82ccf063df92021dfc0f26a5/transformer/transformers/nodecleanup.go#L64
nonces cannot be present in the AMP Cache output, since the AMP Cache will not emit the matching CSP header to enable the nonce. The AMP Cache implements security of the <script> tags through URL-based CSP, rather than nonce. The effect is the same security wise.
We allow them in non-transformed AMP because the AMP Cache can and does strip them before serving. Theoretically the AMP Cache could strip nonces from transformed AMP as well. The validator's implementation of transformed validation assumes the document was already transformed for Cache Serving, as required for Signed Exchanges. In order for the validator to accept (transformed AND nonce), there would need to essentially be a new third validation mode (optimized?). I'd prefer to avoid that if we can get away with it. In this case, I think we can just have the optimizer strip nonces.
Gregable
on 21 Jan 2020
This means we block publishers from using nonces for optimized AMP pages served from their own origin. I'm not an CSP expert, but this sounds like a serious limitation to me.
sebastianbenz
on 21 Jan 2020
I suppose we could also just allow transformed AMP to have nonces. We'd still strip them in the packager/cache, but it wouldn't be verified for SXG in the cache.
Worst case, if a publisher modified the packager code / implemented their own and didn't strip nonces, a document would fail to load AMP javascript.
cc @ampproject/wg-security-privacy
Gregable
on 21 Jan 2020
That'd be even better!
Worst case, if a publisher modified the packager code / implemented their own and didn't strip nonces, a document would fail to load AMP javascript.
Sounds like the better compromise which could be easily solved by documenting the constraints for implementing a packager.
sebastianbenz
on 22 Jan 2020
What's the next step here? Should I submit a PR for the validator or do we need further review?
sebastianbenz
on 23 Jan 2020
@twifkak for thoughts on Signed Exchanges.
Gregable
on 23 Jan 2020
I sync'ed offline with @twifkak. I think we're good to go to modify the validator. You can create the PR or I can if you'd rather.
Gregable
on 23 Jan 2020
For posterity, the SXG decision is based on:
- These nonces are only being applied to AMP runtime scripts.
- The nonces are stripped by amppkg.
- The AMP runtime scripts are otherwise still valid given the CSP provided by amppkg, so stripping nonces won't break pages.
- AMP Cache SXG rules prevent alternative AMP SXG generators from doing anything too different from this.
twifkak
on 23 Jan 2020
Nonced script tags wouldn't break, the nonce is ignored when the page has a whitelist based CSP. Quick test that doesn't throw CSP error: data:text/html,<meta http-equiv="Content-Security-Policy" content="default-src https://cdn.example.net;"><script nonce="asdf" src="https://cdn.example.net/x.js"></script>
At the same time, it doesn't make sense to use CSP nonces in a signed exchange, since the point of the nonce is that it's unpredictable. If it's a signed exchange, it can be replayed and it becomes predictable. The validation error could include a message that explains this. Or maybe we can make it a warning to enable reusing that mode for optimized AMP. Previous internal discussion: b/117119313
molnarg
on 6 Mar 2020
We're running into this at Pinterest. We're trying to implement the AMP optimizer but it's returning invalid AMP due to the nonces.
We'd prefer to have the nonces so that we don't have to modify our CSP policy just for our /amp/ routes.
jonluca
on 16 Jul 2020
I have no memory of this place.
Re: @molnarg's concern around replay attacks, the Google AMP Cache requires a CSP header that does not include script-src nonce, so I believe any nonces in signed exchanges would be ineffectual and thus safe from replay. (Here's some code.)
I think it would be safe to remove this line:
https://github.com/ampproject/amphtml/blob/824fda3d62804cadb3bf1e23d897f12929267b3a/validator/validator-main.protoascii#L4669
This would also allow nonces in a few other places: fonts, application/ld+json, amp-rtc. Are any of those replay attack surfaces? (I note, for instance, that Google allows any font-src in an SXG CSP. I guess that's an indicator that it's not too much of an issue.)
twifkak
on 16 Jul 2020
Yeah, this change seems totally safe to me. Here's my attempt at an assessment:
- In general, Google AMP Cache allows any font-src (including
data:,unsafe-eval...) so there's no change in worst-case security. - For median-case security, amppkg doesn't mutate
font-src, so most pages will fall back todefault-src * blob: data:.data:seems just as susceptible to replay attacks, so there's no change there either. nonceis also ineffectual for thescript type=application/jsoncases, because CSP doesn't apply to them.
twifkak
on 16 Jul 2020
@Gregable fixed this in http://cl/321630472; expect this to land in Google AMP Cache and in this repo w/i a week.
twifkak
on 16 Jul 2020
Just verified, it works now. I'll update Optimizer to not remove nonces.
sebastianbenz
on 3 Aug 2020
Related issues
jpettitt
路
3Comments
mrjoro
路
3Comments
samanthamorco
路
3Comments
Download
路
3Comments
gmajoulet
路
3Comments
Most helpful comment
I have no memory of this place.
Re: @molnarg's concern around replay attacks, the Google AMP Cache requires a CSP header that does not include
script-src nonce, so I believe any nonces in signed exchanges would be ineffectual and thus safe from replay. (Here's some code.)I think it would be safe to remove this line:
https://github.com/ampproject/amphtml/blob/824fda3d62804cadb3bf1e23d897f12929267b3a/validator/validator-main.protoascii#L4669
This would also allow nonces in a few other places: fonts,
application/ld+json,amp-rtc. Are any of those replay attack surfaces? (I note, for instance, that Google allows anyfont-srcin an SXG CSP. I guess that's an indicator that it's not too much of an issue.)