Leaving out max-age on amp-script leads to invalid signature

[dritter]

Hi there!

We use an amp-script with script attribute, but without specifying a max-age. When trying to verify the signature, it turns out to be invalid (message: "sig_expires is in the past").. AFAIK the default expiration time for that case is 1 day.

Looking over the code, it looks like the expiration is calculated based on 24 hours prior to now. That is rather odd to me. Wouldn't that code lead to an expiration of 0 (aka the current time)? And secondly, isn't the minimal max-age > 86400 now?

We worked around that by setting a max-age to a high value (1 year) explicitly.

Thanks in advance.

[twifkak]

Hi, thanks for the report. I agree, serving an expired SXG is odd. It's a confluence of two deliberate choices. I think the thing to do is just not sign at all, if the computed max-age is <24h. I don't think it's a high priority change, as it won't have an impact on behavior in the Google AMP Cache, which already treats this case as unsigned. Let me know if you disagree. Details:

Clock skew

We want to serve SXGs that are somewhat forgiving of client-side clock skew. Section 7.1 of this paper on cert errors suggests that client clocks are much more often behind than ahead, and says that 93.3% of HTTPS requests have a skew of less than 24h. It doesn't have more fine-grained details within that 24h window. I'd love to see more public data/analysis on this subject.

Downgrades

We want to minimize the security considerations of enabling AMP Packager, and of writing an inline <amp-script>. There is a potential spooky-action-at-a-distance in that the interaction of SXG and inline JS requires some thought. But in reality, the publisher choice to enable SXG and choice to write some inline JS are probably made independently and without consideration for one another. Forcing the script-writer to specify a max-age eliminates that spooky-action-at-a-distance.

One example scenario is: 1) publisher has a vulnerability in their JS, 2) attacker downloads an SXG of that vulnerability, 3) publisher fixes it, 4) attacker serves the SXG to people. Thus, the lifetime of the vulnerability is extended to the lifetime of its SXG. 24h was chosen as a default max-age because that's the default max-age of service workers, which have a similar lifetime-extension property.

Cache requirements

The Google AMP Cache does require a minimum lifetime of 4d. A minimum lifetime is necessary to ensure that the SXG can be reused for long enough so that the resultant cache hit rate is reasonably high. However, given a well-formed but invalid SXG, the Google AMP Cache will extract the HTML payload and use it as if it were unsigned (also documented at that above "require" link).

[dritter]

Hi @twifkak thanks for your detailed answer. To be honest, I am a bit more confused than before.

I think the thing to do is just not sign at all, if the computed max-age is <24h.

Wouldn't that make people think that the amppackager does not work at all, if the didn't specify a max-age?
IMHO setting an implicit minimum duration of 1 day would be the better choice.
The tests for the transformer made me think that this is the default anyway.

The Google AMP Cache does require a minimum lifetime of 4d. A minimum lifetime is necessary to ensure that the SXG can be reused for long enough so that the resultant cache hit rate is reasonably high.

So, the optimal max-age should be between 4 and 7 days, right? It would be a good recommendation in the max-age docs of amp-script.

[twifkak]

I'm not sure if this latest treatise helps with confusion any more! I need to learn how to write...

FWIW, there is precedent for the packager not signing in certain circumstances; see the callers of the proxy function. This may make debugging a bit more difficult for publishers most of whose pages are amp-scripted, but I think that's an OK trade-off against security. Note that at all callsite of proxy(), it logs a warning to explain why -- we should do that here, too.

In this case, the duration is 1 day -- that day just happens to be yesterday on most computers. I would prefer not to make a 1-day minimum because there may be legitimate reasons to specify max-age=3600 and I want to respect that. I see two alternatives to not signing:

• Don't backdate by a day.
• Make the default duration 2 days instead of 1.

I would recommend against the latter because I would prefer to err on the side of offering the security property to the publisher that their old version will survive for no longer than 24 hours (unless otherwise specified), and this property would be weaker if it didn't account for at least some of the tail of the clock skew distribution.

The former appeals to my desire for simplicity, but it does lower the utility of these SXGs. Could only backdate when duration > 2 days, but that would be extra complexity for no benefit to the browser-user, given the 4 day requirement.

Good suggestion re: docs; I filed a PR.

What do you think?

[dritter]

I'll try to explain where we came from, maybe that will clarify my point of view.

Initially, we had no amp-script at all, but already used amppackager to sign the requests. Our first use of amp-script (without max-age) made the amppackager fail, which was quite unexpected.

The same scenario now (after #396 being merged), it would be unexpected as well, because it will look like the amppackager stopped working (which is not the case).

So, both cases contain some unexpectedness (= bad DX), which could be solved in three ways IMHO:

1. Improve documentation
   This sounds easier than it actually is. The problem here is that docs speak of max-age as an optional attribute, which is not true, in the described case.

2. Set default duration to 2 days
   This would be a workaround for the next suggestion..

3. Remove backdating
   I think most confusion comes from backdating. Wouldn't it be better to tell people to fix their server times instead of trying to fix that automatically?
   Another example of confusion is: The max duration is 7 days, but isn't it effectively 6 days with 1 day backdated?

Btw. thanks for improving the docs on the default duration. 👍

[twifkak]

Thanks for explaining where you came from. I agree this is bad/frustrating DX. This is a tricky problem. We specifically want to encourage some human intervention at the time when inline script and SXG are combined. Ideally, it should be some sort of alert like "Hey! Look at this thing. Go make a decision." Some questions (for nobody in particular):

• Where should it go? stdout is clearly insufficient. Docker suggests maybe we should use stderr (updated Improving logging format #132). Is there somewhere better?
• What guidance should we provide? There is some middle ground between "you figure it out" and us trying to make a precise security decision tree.

Re: your suggestions:

1. Yes. Always happy to improve docs. I filed 📖 Clarify when max-age is required amphtml#26956 but if you have other ideas please suggest.
2. My inclination is not to go this route due to what I perceive as a high risk:benefit ratio. If we did this, I'd want to loop some security people into the discussion.
3. I like this idea, too. Filed Stop backdating SXGs by 1 day #397, though it's not actionable quite yet.

[dritter]

Thanks for the insights. I agree this is a tricky problem. One point that bothered me in this regard is that the toolchain did not help me very much tracking down this problem:

At first I noticed that our amppackager did not sign the exchanges. Looking at our tooling around that, I saw that amp-linter from amp-toolbox responded with isValid: false. Internally, amp-toolbox uses dump-signedexchange, which responded with valid: false. That was all I had. Only at some point in this process there was a warning printed "sig_expires is in the past", which pointed me into the right direction. It would've been nice if the reason would bubble up in that chain, so that amp-toolbox responds with the reason behind the problem. But that would require all of this tools to be improved.

About 1:
Indeed, I have another suggestion 😁
The Limitations Section in the Documentation could be improved. It would be great to add the duration rules there and have a writeup about other rules that apply (like what happens, if I add multiple amp-scripts with different max-ages?).

After that, IMHO the documentation part is 👍 .

About 2:
Yep, I mentioned that just for the sake of completeness.

About 3:
Even after sleeping a night over that, this looks like the best option to me. But having the docs improved, that is not very urgent.

[twifkak]

Thanks, @dritter. I merged in your suggestion; hopefully the wording is OK. I'll keep (3) in mind, would be nice to address that.

[virendra-rastogi]

@twifkak @dritter I am facing the similar issue which you guys have discussed and fixed here please have a look [(https://github.com//issues/623)] and suggest me how to fix because I am facing this on my production environment. urgent help required