Bug 190956 – [WPE][GTK] Certificates loaded from the cache don't include the certificate chain

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Claudio Saavedra]

[WPE][GTK] Certificates loaded from the cache don't include the certificate chain

[Claudio Saavedra]

Created attachment 353177 [details]
Patch

I didn't increase the cache version because I am not sure if this change needs it. AFAICT, the loader would load a boolean as integer, which would be interpreted as 1 and then the iteration would try to load only one certificate from the cache.

[Claudio Saavedra]

I also think that at some point it would be interesting to check if we can factor the code to a common place so that both IPC and Cache coders use the same encoding/decoding code/format. Not sure it's needed to have this redundant code in different places. For now, let's fix this though.

[Michael Catanzaro]

Comment on attachment 353177 [details]
Patch

r=me on the code changes, since this is just syncing the cache coders with the IPC coders that we've now reviewed thoroughly.

I'm not sure about the rules for changing the cache coders either, though. I think it does probably need a new cache version, otherwise downgrades could break: old WebKitGTK+ could interpret 3 certificates as a bool true, then read only one certificate, and cache corruption would ensue. And the cache version is shared for all ports, so I fear this needs owner approval to bump the cache version. Let's see what Youenn thinks.

[Michael Catanzaro]

(In reply to Claudio Saavedra from comment #2)
> I also think that at some point it would be interesting to check if we can
> factor the code to a common place so that both IPC and Cache coders use the
> same encoding/decoding code/format. Not sure it's needed to have this
> redundant code in different places. For now, let's fix this though.

We discussed this is probably to make it easier to make changes to the IPC coders.

[Michael Catanzaro]

Another question: what actually happens to these certificates stored in the cache?

Obviously they are not used for anything important, because without the full chain, they have been broken up till now.

[youenn fablet]

Comment on attachment 353177 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        the entire certificate chain in the cache coder.

Yes, we need to increment NetworkCache::Storage::version.
Incrementing the counter is invalidating the whole cache for all users.
I wonder whether this is okayish or whether we should introduce two counters, one for cross-platform changes and one for platform-specific changes.
John for some other stuff might bump the version for all platforms as well so maybe we do not care.
Antti, any suggestion?

> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:36
> +        encoder << 0;

Is there a guarantee that this line is always equivalent to below encoding of the vector?
I would tend to write it this way:
Vector<GRefPtr<GByteArray>> certificatesDataListFromCertificateInfo(const WebCore::CertificateInfo& certificateInfo)
{
    if (!certificate)
        return { };
    ...
}

void Coder<WebCore::CertificateInfo>::encode(Encoder& encoder, const WebCore::CertificateInfo& certificateInfo)
{
    encoder << certificatesDataListFromCertificateInfo(certificateInfo);
}

This requires introducing a coder for GRefPtr<GByteArray>.

> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:69
>  {

Ditto here, we should be able to do: 
decoder >> certificateDataList;

That way, we are sure that encoding/decoding of the vector is consistent.

[Claudio Saavedra]

Comment on attachment 353177 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:36
>> +        encoder << 0;
> 
> Is there a guarantee that this line is always equivalent to below encoding of the vector?
> I would tend to write it this way:
> Vector<GRefPtr<GByteArray>> certificatesDataListFromCertificateInfo(const WebCore::CertificateInfo& certificateInfo)
> {
>     if (!certificate)
>         return { };
>     ...
> }
> 
> void Coder<WebCore::CertificateInfo>::encode(Encoder& encoder, const WebCore::CertificateInfo& certificateInfo)
> {
>     encoder << certificatesDataListFromCertificateInfo(certificateInfo);
> }
> 
> This requires introducing a coder for GRefPtr<GByteArray>.

GRefPtr<GByteArray> or just GByteArray?

[Michael Catanzaro]

(In reply to youenn fablet from comment #6)
> Yes, we need to increment NetworkCache::Storage::version.
> Incrementing the counter is invalidating the whole cache for all users.
> I wonder whether this is okayish or whether we should introduce two
> counters, one for cross-platform changes and one for platform-specific
> changes.
> John for some other stuff might bump the version for all platforms as well
> so maybe we do not care.
> Antti, any suggestion?

It's the first time I remember having to do this, and I don't expect it to happen again anytime soon. So perhaps we should just remember to land this whenever you next need to bump the cache version.

[Claudio Saavedra]

(In reply to Michael Catanzaro from comment #8)
>  So perhaps we should just remember to land this
> whenever you next need to bump the cache version.

Eh, no. Actually as it is the certificate chain for a resource loaded from the cache or from the network are different. From WK's pov it might not matter, but the WKGTK/WPE API will have inconsistent certificate info depending on whether a webview was loaded from the cache or the network. So I think we need to fix this before 2.24.

[Michael Catanzaro]

OK, good point. Then let's try to coordinate this bump with Antti and John.

[youenn fablet]

> GRefPtr<GByteArray> or just GByteArray?

Not sure, GRefPtr<GByteArray> will work.
You might be able to do like optional, see PersistentCoders.h

[Claudio Saavedra]

Created attachment 353363 [details]
Patch

Let's try again.

[youenn fablet]

Comment on attachment 353363 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:62
> +    for (uint32_t i = 0; i < certificatesDataList.size(); i++) {

for (auto& certificateData : certificatesDataList) or remove the reverse above and decrement the counter here.

> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:85
> +    decoder.decode(certificatesDataList);

We should probably check the returned value here.

> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:116
> +    byteArray = adoptGRef(g_byte_array_append(bytes, vector.data(), vector.size()));

We do two memory allocations. Is GByteArray allowing to make only one?

[Claudio Saavedra]

Comment on attachment 353363 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:116
>> +    byteArray = adoptGRef(g_byte_array_append(bytes, vector.data(), vector.size()));
> 
> We do two memory allocations. Is GByteArray allowing to make only one?

Not sure I understand. There's only one allocation, afaik (the new() call). g_byte_array_append() doesn't add a reference.

[Claudio Saavedra]

Created attachment 353368 [details]
Patch

[youenn fablet]

(In reply to Claudio Saavedra from comment #14)
> Comment on attachment 353363 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353363&action=review
> 
> >> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:116
> >> +    byteArray = adoptGRef(g_byte_array_append(bytes, vector.data(), vector.size()));
> > 
> > We do two memory allocations. Is GByteArray allowing to make only one?
> 
> Not sure I understand. There's only one allocation, afaik (the new() call).
> g_byte_array_append() doesn't add a reference.

There is the Vector<allocation> and then the GByteArray allocation.
I am not sure whether it is feasible to just have one allocation here.

[Claudio Saavedra]

Comment on attachment 353363 [details]
Patch

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

>>>> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:116
>>>> +    byteArray = adoptGRef(g_byte_array_append(bytes, vector.data(), vector.size()));
>>> 
>>> We do two memory allocations. Is GByteArray allowing to make only one?
>> 
>> Not sure I understand. There's only one allocation, afaik (the new() call). g_byte_array_append() doesn't add a reference.
> 
> There is the Vector<allocation> and then the GByteArray allocation.
> I am not sure whether it is feasible to just have one allocation here.

I see. One can pass the data without reallocating it but then it will eventually be freed with g_free(). I'm not sure if that will match whatever Vector is using to allocate memory.

[Claudio Saavedra]

Created attachment 353373 [details]
Patch

Okay, this should do it. Let's get rid of the need for a Vector and decode directly into the GByteArray.

[Michael Catanzaro]

Comment on attachment 353373 [details]
Patch

Should these be synced with the IPC coders now?

[Claudio Saavedra]

I don't think that's necessary. These are entirely independent implementations. We could improve the other one though but that's a story for another day..

[youenn fablet]

Comment on attachment 353373 [details]
Patch

LGTM, it would be good to have another LGTM from soup side.

As of the refactoring for sharing with IPC, maybe a templated encode/decode routine could be added to CertificateInfo directly.
Some unit test might be feasible, especially if that was a source of issues in the past.

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheCodersSoup.cpp:90
> +    certificateInfo.setCertificate(certificateFromCertificatesDataList(certificatesDataList).get());

Should setCertificate take a GRefPtr<>&&?

[Michael Catanzaro]

(In reply to youenn fablet from comment #21) 
> LGTM, it would be good to have another LGTM from soup side.

LGTM

[Claudio Saavedra]

Committed r237628: <https://trac.webkit.org/changeset/237628>