Make WKWebView._negotiatedLegacyTLS accurate when loading main resouorce from network or cache

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Fix race condition for WKWebView._negotiatedLegacyTLS when loading main resouorce

[Alex Christensen]

Created attachment 389676 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/59153742>

[Alex Christensen]

Created attachment 389688 [details]
Patch

[Alex Christensen]

Created attachment 389692 [details]
Patch

[Alex Christensen]

Created attachment 389699 [details]
Patch

[Alex Christensen]

Created attachment 389704 [details]
Patch

[Alex Christensen]

Created attachment 389713 [details]
Patch

[Chris Dumez]

Comment on attachment 389713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389713&action=review

> Source/WebCore/platform/network/ResourceResponseBase.h:285
> +    uint8_t isRedirectedAndUsedLegacyTLS = (m_isRedirected ? 0x1 : 0x0) | (m_usedLegacyTLS == UsedLegacyTLS::Yes ? 0x2 : 0x0);

Can we please avoid obscure code like this simply to encode a boolean? I'm really not convinced the extra complexity is worth the very slight saving of data sent over IPC.

> Source/WebKit/NetworkProcess/NetworkLoad.cpp:236
> +        response.includeCertificateInfo(negotiatedLegacyTLS == NegotiatedLegacyTLS::Yes ? UsedLegacyTLS::Yes : UsedLegacyTLS::No);

Do we really need both NegotiatedLegacyTLS and UsedLegacyTLS?

[Alex Christensen]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 389713 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389713&action=review
> 
> > Source/WebCore/platform/network/ResourceResponseBase.h:285
> > +    uint8_t isRedirectedAndUsedLegacyTLS = (m_isRedirected ? 0x1 : 0x0) | (m_usedLegacyTLS == UsedLegacyTLS::Yes ? 0x2 : 0x0);
> 
> Can we please avoid obscure code like this simply to encode a boolean? I'm
> really not convinced the extra complexity is worth the very slight saving of
> data sent over IPC.
This isn't about IPC.  This is about reading from existing disk caches.  I did not think this was enough to increment the disk cache version.
 
> > Source/WebKit/NetworkProcess/NetworkLoad.cpp:236
> > +        response.includeCertificateInfo(negotiatedLegacyTLS == NegotiatedLegacyTLS::Yes ? UsedLegacyTLS::Yes : UsedLegacyTLS::No);
> 
> Do we really need both NegotiatedLegacyTLS and UsedLegacyTLS?
We need something in WebCore, and I'm trying to make this patch as easy to merge as possible.  I'll clean this up with one type with its own header after landing.

[Chris Dumez]

Comment on attachment 389713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389713&action=review

>>> Source/WebCore/platform/network/ResourceResponseBase.h:285
>>> +    uint8_t isRedirectedAndUsedLegacyTLS = (m_isRedirected ? 0x1 : 0x0) | (m_usedLegacyTLS == UsedLegacyTLS::Yes ? 0x2 : 0x0);
>> 
>> Can we please avoid obscure code like this simply to encode a boolean? I'm really not convinced the extra complexity is worth the very slight saving of data sent over IPC.
> 
> This isn't about IPC.  This is about reading from existing disk caches.  I did not think this was enough to increment the disk cache version.

Still, I do not like the added complexity. I'd still prefer bumping the cache version. One alternative: Do we really need to save this bit to the disk cache, or is IPC encoding/decoding sufficient?

[Alex Christensen]

Created attachment 389734 [details]
Patch

[Geoffrey Garen]

Comment on attachment 389734 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389734&action=review

> Source/WebKit/ChangeLog:18
> +        In order to maintain binary compatibility with existing disk caches, I bundle two booleans together
> +        when encoding.  This results in existing disk caches saying they loaded nothing over legacy TLS,
> +        which is somewhat correct because TLS 1.0 and 1.1 were not deprecated when the resources were fetched.
> +        Going forward, the bit will be set and the disk cache entries will store whether they were loaded
> +        over legacy TLS.

This isn't true in the newest patch.

Also, this is a separate change that is unrelated to the race condition you're trying to fix. Can you make two separate patches? The choice for what's right for the disk cache probably deserves some discussion, and the fix for the race condition need not wait on that discussion.

[Alex Christensen]

Created attachment 389740 [details]
Patch

[Alex Christensen]

(In reply to Geoffrey Garen from comment #12)
> Comment on attachment 389734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389734&action=review
> 
> > Source/WebKit/ChangeLog:18
> > +        In order to maintain binary compatibility with existing disk caches, I bundle two booleans together
> > +        when encoding.  This results in existing disk caches saying they loaded nothing over legacy TLS,
> > +        which is somewhat correct because TLS 1.0 and 1.1 were not deprecated when the resources were fetched.
> > +        Going forward, the bit will be set and the disk cache entries will store whether they were loaded
> > +        over legacy TLS.
> 
> This isn't true in the newest patch.
You're right.  I updated the changelog to reflect the changes that were made in the updated patch.
> 
> Also, this is a separate change that is unrelated to the race condition
> you're trying to fix. Can you make two separate patches? The choice for
> what's right for the disk cache probably deserves some discussion, and the
> fix for the race condition need not wait on that discussion.

The title was indeed misleading, so I changed it.  This patch fixes the actual problem which was that users observe _negotiatedLegacyTLS to be incorrectly false because of a race condition and because of the disk cache.  Luckily the same code fixes both: the ResourceResponseBase serialization code.

[Alex Christensen]

Created attachment 389744 [details]
Patch

[Alex Christensen]

Created attachment 389748 [details]
Patch

[Alex Christensen]

Created attachment 389750 [details]
Patch

[Alex Christensen]

Created attachment 389756 [details]
Patch

[Geoffrey Garen]

You’ve made an important discover: We didn’t previously consider the http cache.

Given that discovery, I’d like you to facilitate a discussion about how the http cache should interact with the legacy TLS warning.

Some questions I have for that discussion:

1. If an existing cache entry was loaded over unknown or legacy TLS, and then the user updates WebKit, should WebKit evict that cache entry, load it and show it as insecure, load it and show it as secure, or select a behavior based on whether a new connection to the server yields modern TLS?

2. If an existing cache entry was loaded over unknown or legacy TLS, and then the server updates to modern TLS…. { Same questions as 1 }?

3. Is it accurate to say that a connection to a server is insecure when, in fact, no connection to the server has been made (and the connection, if made, might be secure)?

4. What do other browsers do?

5. Is it acceptable to pay the cost of evicting all caches worldwide in order to get the behavior we want here?

6. Should we put a cap on the lifetime of legacy TLS resources in the cache, to avoid the case where a long-lived resource never gets upgraded to modern TLS?

[Alex Christensen]

(In reply to Geoffrey Garen from comment #19)
> You’ve made an important discover: We didn’t previously consider the http
> cache.
> 
> Given that discovery, I’d like you to facilitate a discussion about how the
> http cache should interact with the legacy TLS warning.
> 
> Some questions I have for that discussion:
> 
> 1. If an existing cache entry was loaded over unknown or legacy TLS, and
> then the user updates WebKit, should WebKit evict that cache entry, load it
> and show it as insecure, load it and show it as secure, or select a behavior
> based on whether a new connection to the server yields modern TLS?
This will remove all existing cache entries, so this question is invalid.

> 2. If an existing cache entry was loaded over unknown or legacy TLS, and
> then the server updates to modern TLS…. { Same questions as 1 }?
The only mechanism for this is the existing mechanism for if they change the content on their server, at which point the cache will be updated.  Other than that, we can't know what the server does if we don't connect to the server.

> 3. Is it accurate to say that a connection to a server is insecure when, in
> fact, no connection to the server has been made (and the connection, if
> made, might be secure)?
We want our users to consistently see that a web page has negotiated legacy TLS, whether it is loaded from the server or from the cache.  We already have people wondering why it is sometimes true and sometimes false for the same site, and that is the problem to be fixed by this patch.

> 4. What do other browsers do?
There is no precedent, and this is intentionally different than their stated plans.

> 5. Is it acceptable to pay the cost of evicting all caches worldwide in
> order to get the behavior we want here?
I thought it was not, but responding to Chris's review feedback I did it.

> 6. Should we put a cap on the lifetime of legacy TLS resources in the cache,
> to avoid the case where a long-lived resource never gets upgraded to modern
> TLS?
No.

[Alex Christensen]

Created attachment 389833 [details]
patch without affecting cache

[Alex Christensen]

I feel an urgency to land this. If you feel it would be better to not affect the cache and respond to users' visible inconsistency on the same page later, feel free to review that patch.

[Chris Dumez]

Comment on attachment 389756 [details]
Patch

r=me

[Geoffrey Garen]

I'm OK with landing this patch.

It seems like we do need to blow away existing cache entries. Otherwise, the behavior of our insecure warning will be inconsistent and confusing, with some users getting the warning and some not, depending on pre-existing cache state. Also wouldn't be great to load an insecure resource from the cache and call it secure.

I do think we should consider limiting the lifetime of legacy TLS resources in the disk cache, or not storing them at all. A long-lived legacy TLS resource in the disk cache makes it much harder for a website to upgrade to modern TLS, since it will need not only to start supporting modern TLS but also to rename a lot resources to prevent pulling old resources from the cache. And it feels like an own goal to keep loading an insecure resource when a secure resource is available.

[Alex Christensen]

https://trac.webkit.org/changeset/255846/webkit

[Chris Dumez]

Follow-up build fix: <https://trac.webkit.org/changeset/255863>

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> Follow-up build fix: <https://trac.webkit.org/changeset/255863>

And another build fix: <https://trac.webkit.org/changeset/255880>