Bug 146159 – Check for SHA1 certificates ignores subresources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Michael Catanzaro]

WebPageProxy::didCommitLoadForFrame decides whether to throw a security warning if the certificate chain includes a SHA1 certificate, but does so only for the main resource. Subresources are ignored. That's not good: the overall security of the page should be determined by the security of the least-secure subresource.

We should probably replace the second parameter to PageLoadState::didCommitLoad (which should only be called for the main resource) with a new PageLoadState member function for this.

[Michael Catanzaro]

No need for a new function; there is already PageLoadState::didDisplayOrRunInsecureContent.

[Michael Catanzaro]

Created attachment 255224 [details]
Patch

[Michael Catanzaro]

The patch is really simple so I think it is safe, but note that this is Mac-specific functionality and I don't have a Mac.

Comment on attachment 255224 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-2947
> -        m_pageLoadState.didCommitLoad(transaction, hasInsecureCertificateChain);

When hasInsecureCertificateChain was false, this was the code that would reset the hasInsecureContent flag in the page load state when navigating from an “insecure content” page to a “secure” page.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2949
> +        m_pageLoadState.didDisplayOrRunInsecureContent(transaction);

This is no substitute for that, because it only changes the state from “secure” to “insecure”.

[Michael Catanzaro]

Good catch, thanks! I was thinking that during the course of one load, we would never want to change the state from insecure to secure, but of course we need to do so for subsequent loads.

The easy fix to my patch is to reset the state to false in didCommitLoad, but that would be more fragile than the current code since the correctness would depend on the order the PageLoadState functions are called inside WebPageProxy::didCommitLoadForFrame. So we should keep the parameter and just rename it to something more accurate. It's hard to think of a good name though, so let's punt that to bug #146157.

[Michael Catanzaro]

Created attachment 255265 [details]
Patch

Comment on attachment 255265 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2944
> +    // The page is definitely insecure if true, but could still be insecure even if false.

I think I understand this comment, but I am not sure what motivates adding it here now. Doing so doesn’t seem to be directly related to fixing the bug.

[Michael Catanzaro]

Created attachment 255289 [details]
Patch

[Michael Catanzaro]

Comment on attachment 255289 [details]
Patch

Thanks for the review. I'll just remove the comment.

[WebKit Commit Bot]

Comment on attachment 255289 [details]
Patch

Clearing flags on attachment: 255289

Committed r185795: <http://trac.webkit.org/changeset/185795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.