Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move CertificateInfo::containsNonRootSHA1SignedCertificate call from UI to web process #8161

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Move CertificateInfo::containsNonRootSHA1SignedCertificate call from UI to web process #8161

merged 1 commit into from
Jan 5, 2023

Conversation

achristensen07
Copy link
Contributor

@achristensen07 achristensen07 commented Jan 3, 2023 •
edited by webkit-early-warning-system

6cd18ec

Move CertificateInfo::containsNonRootSHA1SignedCertificate call from UI to web process
https://bugs.webkit.org/show_bug.cgi?id=250043
rdar://99324948

Reviewed by Chris Dumez.

It calls security framework functions which call SecTrustEvaluateIfNecessary which now warns developers
when it's used on the main thread.  This warning is a good thing, but many developers are getting
confused and thinking that the problem is in their application code, when it's really in WebKit code.
To stop the warning when debugging third party apps, move the call to the web process.  Also stop
sending an unnecessary std::optional.

* Source/WebKit/UIProcess/ProvisionalFrameProxy.cpp:
(WebKit::ProvisionalFrameProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/ProvisionalFrameProxy.h:
* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
* Source/WebKit/UIProcess/WebFrameProxy.cpp:
(WebKit::WebFrameProxy::commitProvisionalFrame):
* Source/WebKit/UIProcess/WebFrameProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::commitProvisionalPage):
(WebKit::WebPageProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDidCommitLoad):

Canonical link: https://commits.webkit.org/258511@main

ba0a1f6

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim
✅ 🛠 🧪 unsafe-merge

@achristensen07 achristensen07 self-assigned this Jan 3, 2023
@achristensen07 achristensen07 added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jan 3, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash ba0a1f6)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim
✅ 🛠 🧪 unsafe-merge

cdumez
cdumez approved these changes Jan 5, 2023
View reviewed changes
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
auto certificateInfo = valueOrCompute(documentLoader.response().certificateInfo(), [] {
return CertificateInfo();
});
hasInsecureContent = hasInsecureContent ? *hasInsecureContent : (certificateInfo.containsNonRootSHA1SignedCertificate() ? HasInsecureContent::Yes : HasInsecureContent::No);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably check with @pvollan that we're OK doing the SecTrustEvaluate calls from the WebProcess in the long term (because of sandboxing reasons).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did. I verified there's already a call to containsNonRootSHA1SignedCertificate in the web process and it hasn't caused any problems. I also verified that this doesn't cause any issues. I think it's because we call response.includeCertificateInfo in NetworkLoad::notifyDidReceiveResponse before sending the SecTrustRef to the web process, so the web process doesn't actually need to do anything that our sandbox would oppose.

@achristensen07 achristensen07 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 5, 2023
@achristensen07
Move CertificateInfo::containsNonRootSHA1SignedCertificate call from …
6cd18ec 
…UI to web process

https://bugs.webkit.org/show_bug.cgi?id=250043
rdar://99324948

Reviewed by Chris Dumez.

It calls security framework functions which call SecTrustEvaluateIfNecessary which now warns developers
when it's used on the main thread.  This warning is a good thing, but many developers are getting
confused and thinking that the problem is in their application code, when it's really in WebKit code.
To stop the warning when debugging third party apps, move the call to the web process.  Also stop
sending an unnecessary std::optional.

* Source/WebKit/UIProcess/ProvisionalFrameProxy.cpp:
(WebKit::ProvisionalFrameProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/ProvisionalFrameProxy.h:
* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
* Source/WebKit/UIProcess/WebFrameProxy.cpp:
(WebKit::WebFrameProxy::commitProvisionalFrame):
* Source/WebKit/UIProcess/WebFrameProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::commitProvisionalPage):
(WebKit::WebPageProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDidCommitLoad):

Canonical link: https://commits.webkit.org/258511@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Move-CertificateInfocontainsNonRootSHA1SignedCertificate-call-from-UI-to-web-process branch from ba0a1f6 to 6cd18ec Compare January 5, 2023 22:24
@webkit-commit-queue
Copy link
Collaborator

Committed 258511@main (6cd18ec): https://commits.webkit.org/258511@main

Reviewed commits have been landed. Closing PR #8161 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 6cd18ec into WebKit:main Jan 5, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez approved these changes

Assignees

@achristensen07 achristensen07

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
4 participants
@achristensen07 @webkit-early-warning-system @webkit-commit-queue @cdumez