[Webkit-unassigned] [Bug 162910] [SOUP] Move global TLS errors handling from ResourceHandle to SoupNetworkSession

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162910

--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #2)
> Comment on attachment 290607 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290607&action=review
> 
> I know this is a preexisting problem, but please fix it first: either use a
> secure hash algorithm in HostTLSCertificateSet (e.g. SHA-256 from
> CryptoDigest.h), or else just stop using hashes there and copy the full
> certificates into the hash table. This is a sufficiently-minor issue that I
> don't think we need to get a CVE for it, but it needs fixed.

What's the problem of using SHA1 here? What's the security problem exactly? We are using it just as a hash, for non private information, as an optimization to avoid having to do comparisons with large certificate contents. It's not the only place in WebKit that SHA1 is used, I don't mind if the algorithm is not secure enough for this particular use case.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:67
> > +    bool contains(GTlsCertificate* certificate)
> 
> Should be const (preexisting issue)

Yes.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.h:68
> > +    static void setShouldIgnoreTLSErrors(bool);
> > +    static void checkTLSErrors(SoupRequest*, SoupMessage*, std::function<void (const ResourceError&)>&&);
> > +    static void allowSpecificHTTPSCertificateForHost(const CertificateInfo&, const String& host);
> 
> Why make these static? Doesn't it defeat the point of moving this code to
> SoupNetworkSession? I would expect different network sessions to handle
> these independently. If you were planning to change this in a future patch,
> it'd be better to do it now by using SoupNetworkSession::defaultSession() in
> NetworkProcessSoup.cpp, and fix up NetworkProcessSoup.cpp in a future patch.

No, the point, as the ChangeLog says, is using this from NetworkSession that replaces ResourceHandle, SoupNetworkSession is a good place common in both code paths. I'm definitely going to rework the soup session handling, but I'm still not sure how exactly. The TLS policy is a network process global setting anyway, so it should be applied to any network session used. Note that there are only 3 possible sessions: default, private and testing. We don't support private browsing and testing is only used internally by layout tests, so from the users point of view there's only one global session which is the default one.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161005/a497fb78/attachment.html>