209987 2020-04-03 14:23:09 -0700 Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy() 2020-04-03 17:16:11 -0700 1 1 1 Unclassified WebKit Page Loading WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ddkilzer achristensen achristensen beidson bfulgham cdumez darin ews-watchlist japhet product-security rniwa webkit-bug-importer wenson_hsieh oldest_to_newest 1637526 0 ddkilzer 2020-04-03 14:23:09 -0700 Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy(). The last if/else statement uses formState.get(), but that's been moved into the `decisionHandler` lambda. void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, const ResourceResponse& redirectResponse, DocumentLoader* loader, RefPtr<FormState>&& formState, NavigationPolicyDecisionFunction&& function, PolicyDecisionMode policyDecisionMode) { [...] FramePolicyFunction decisionHandler = [this, function = WTFMove(function), request = ResourceRequest(request), formState = WTFMove(formState), suggestedFilename = WTFMove(suggestedFilename), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension), requestIdentifier, isInitialEmptyDocumentLoad] (PolicyAction policyAction, PolicyCheckIdentifier responseIdentifier) mutable { [...] }; if (isInitialEmptyDocumentLoad) { // We ignore the response from the client for initial empty document loads and proceed with the load synchronously. m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, redirectResponse, formState.get(), policyDecisionMode, requestIdentifier, [](PolicyAction, PolicyCheckIdentifier) { }); decisionHandler(PolicyAction::Use, requestIdentifier); } else m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, redirectResponse, formState.get(), policyDecisionMode, requestIdentifier, WTFMove(decisionHandler)); } 1637530 1 ddkilzer 2020-04-03 14:32:00 -0700 Not sure the best way to fix this. Should WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() be changed to take a RefPtr<FormState>&& instead of a FormState* and the `decisionHandler` just make a copy of `formState` instead of moving it? 1637547 2 webkit-bug-importer 2020-04-03 15:36:46 -0700 <rdar://problem/61278558> 1637550 3 cdumez 2020-04-03 15:46:13 -0700 I think Alex found it a few days ago too. 1637551 4 cdumez 2020-04-03 15:48:22 -0700 (In reply to Chris Dumez from comment #3) > I think Alex found it a few days ago too. I believe he has a pending fix for it. Also, I don't see why this is a security bug. The behavior of the RefPtr<> move constructor is well defined. We simply pass null the second time if it was already moved (which was intentional behavior). Anyway, AFAICT, the current code is safe and correct. I do agree that it is bad practice though and should be written some other way (which I believe Alex is doing). 1637552 5 rniwa 2020-04-03 15:55:29 -0700 Okay, back to normal component. 1637554 6 cdumez 2020-04-03 15:58:51 -0700 Should be coordinated with Alex because I think he is in the process of refactoring this code (and fixing this at the same time) I believe. That said, a trivial fix would be to replace WTFMove(formState), std::exchange(formState, nullptr). It is equivalent given our current implementation of the RefPtr<> move constructor but looks safer. 1637567 7 achristensen 2020-04-03 16:46:57 -0700 I fixed a similar use-after-move in WebPageProxy::receivedNavigationPolicyDecision in r259307. We should use std::exchange here and be well defined without changing behavior. 1637569 8 395418 achristensen 2020-04-03 16:48:49 -0700 Created attachment 395418 Patch 1637574 9 ews-feeder 2020-04-03 17:16:09 -0700 Committed r259522: <https://trac.webkit.org/changeset/259522> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395418. 395418 2020-04-03 16:48:49 -0700 2020-04-03 17:16:10 -0700 Patch bug-209987-20200403164847.patch text/plain 2134 achristensen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI1OTUyMSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDIwLTA0LTAzICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgVXNlLWFmdGVy LW1vdmUgb2YgYGZvcm1TdGF0ZWAgaW4gV2ViQ29yZTo6UG9saWN5Q2hlY2tlcjo6Y2hlY2tOYXZp Z2F0aW9uUG9saWN5KCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTIwOTk4NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIFVzZSBzdGQ6OmV4Y2hhbmdlIGJlY2F1c2UgZm9ybVN0YXRlIGlzIHVzZWQgbGF0ZXIu CisgICAgICAgIE5vIGNoYW5nZSBpbiBiZWhhdmlvciwgYnV0IHRoaXMgd2lsbCBhbGxvdyB1c2Ut YWZ0ZXItbW92ZSBodW50cyB0byBjb250aW51ZS4KKworICAgICAgICAqIGxvYWRlci9Qb2xpY3lD aGVja2VyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBvbGljeUNoZWNrZXI6OmNoZWNrTmF2aWdh dGlvblBvbGljeSk6CisKIDIwMjAtMDQtMDMgIFNpaHVpIExpdSAgPHNpaHVpX2xpdUBhcHBsZS5j b20+CiAKICAgICAgICAgQVNTRVJUSU9OIEZBSUxFRDogb2JqZWN0U3RvcmVJbmZvIGluIFNRTGl0 ZUlEQkJhY2tpbmdTdG9yZTo6Z2V0UmVjb3JkCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIv UG9saWN5Q2hlY2tlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvbG9hZGVyL1Bv bGljeUNoZWNrZXIuY3BwCShyZXZpc2lvbiAyNTk0ODApCisrKyBTb3VyY2UvV2ViQ29yZS9sb2Fk ZXIvUG9saWN5Q2hlY2tlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIwMyw3ICsyMDMsNyBAQCB2 b2lkIFBvbGljeUNoZWNrZXI6OmNoZWNrTmF2aWdhdGlvblBvbGljCiAgICAgYXV0byByZXF1ZXN0 SWRlbnRpZmllciA9IFBvbGljeUNoZWNrSWRlbnRpZmllcjo6Y3JlYXRlKCk7CiAgICAgbV9kZWxl Z2F0ZUlzRGVjaWRpbmdOYXZpZ2F0aW9uUG9saWN5ID0gdHJ1ZTsKICAgICBTdHJpbmcgc3VnZ2Vz dGVkRmlsZW5hbWUgPSBhY3Rpb24uZG93bmxvYWRBdHRyaWJ1dGUoKS5pc0VtcHR5KCkgPyBudWxs QXRvbSgpIDogYWN0aW9uLmRvd25sb2FkQXR0cmlidXRlKCk7Ci0gICAgRnJhbWVQb2xpY3lGdW5j dGlvbiBkZWNpc2lvbkhhbmRsZXIgPSBbdGhpcywgZnVuY3Rpb24gPSBXVEZNb3ZlKGZ1bmN0aW9u KSwgcmVxdWVzdCA9IFJlc291cmNlUmVxdWVzdChyZXF1ZXN0KSwgZm9ybVN0YXRlID0gV1RGTW92 ZShmb3JtU3RhdGUpLCBzdWdnZXN0ZWRGaWxlbmFtZSA9IFdURk1vdmUoc3VnZ2VzdGVkRmlsZW5h bWUpLAorICAgIEZyYW1lUG9saWN5RnVuY3Rpb24gZGVjaXNpb25IYW5kbGVyID0gW3RoaXMsIGZ1 bmN0aW9uID0gV1RGTW92ZShmdW5jdGlvbiksIHJlcXVlc3QgPSBSZXNvdXJjZVJlcXVlc3QocmVx dWVzdCksIGZvcm1TdGF0ZSA9IHN0ZDo6ZXhjaGFuZ2UoZm9ybVN0YXRlLCBudWxscHRyKSwgc3Vn Z2VzdGVkRmlsZW5hbWUgPSBXVEZNb3ZlKHN1Z2dlc3RlZEZpbGVuYW1lKSwKICAgICAgICAgIGJs b2JVUkxMaWZldGltZUV4dGVuc2lvbiA9IFdURk1vdmUoYmxvYlVSTExpZmV0aW1lRXh0ZW5zaW9u KSwgcmVxdWVzdElkZW50aWZpZXIsIGlzSW5pdGlhbEVtcHR5RG9jdW1lbnRMb2FkXSAoUG9saWN5 QWN0aW9uIHBvbGljeUFjdGlvbiwgUG9saWN5Q2hlY2tJZGVudGlmaWVyIHJlc3BvbnNlSWRlbnRp ZmllcikgbXV0YWJsZSB7CiAgICAgICAgIGlmICghcmVzcG9uc2VJZGVudGlmaWVyLmlzVmFsaWRG b3IocmVxdWVzdElkZW50aWZpZXIpKSB7CiAgICAgICAgICAgICBSRUxFQVNFX0xPR19JRl9BTExP V0VEKCJjaGVja05hdmlnYXRpb25Qb2xpY3k6IGlnbm9yaW5nIGJlY2F1c2UgcmVzcG9uc2UgaXMg bm90IHZhbGlkIGZvciByZXF1ZXN0Iik7Cg==