Summary: | Remove kCCNotVerified | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||
Component: | WebCore Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, commit-queue, jiewen_tan, tsavell, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jiewen Tan
2018-11-27 13:57:49 PST
Created attachment 355776 [details]
Patch
Comment on attachment 355776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355776&action=review > Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:80 > - return Exception { OperationError }; > + return false; This changes the behavior in the case of kCCParamError from raising an exception to returning false. Is that OK? Comment on attachment 355776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355776&action=review > Source/WebCore/ChangeLog:9 > + No change of behaviours. Covered by existing tests. Instead? >> Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:80 >> + return false; > > This changes the behavior in the case of kCCParamError from raising an exception to returning false. Is that OK? I think it is fine. 1) It doesn't regress any of the crypto test. 2) The spec doesn't define we should return OperationError here as the sanity check of the web params are performed in a higher level. 3) It is better to return yes or no for signature verification to prevent any potential attacks. > I think it is fine.
I don't have any strong objection.
However, I would be more comfortable not doing a behavior change in a build fix patch.
(In reply to Alexey Proskuryakov from comment #5) > > I think it is fine. > > I don't have any strong objection. > > However, I would be more comfortable not doing a behavior change in a build > fix patch. Okay, I will keep the kCCDecodeError then. Created attachment 355792 [details]
Patch
Comment on attachment 355792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355792&action=review > Source/WebCore/crypto/CommonCryptoUtilities.h:-76 > - kCCNotVerified = -4306 We don't need this for EC, do we? Comment on attachment 355792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355792&action=review Thanks Alexey for r+ this patch. >> Source/WebCore/crypto/CommonCryptoUtilities.h:-76 >> - kCCNotVerified = -4306 > > We don't need this for EC, do we? We don't. EC directly returns true or false. The commit-queue encountered the following flaky tests while processing attachment 355792 [details]: webgl/2.0.0/conformance/more/functions/bindBuffer.html bug 192051 (author: justin_fan@apple.com) The commit-queue is continuing to process your patch. Comment on attachment 355792 [details] Patch Clearing flags on attachment: 355792 Committed r238588: <https://trac.webkit.org/changeset/238588> All reviewed patches have been landed. Closing bug. It looks like the change in https://trac.webkit.org/changeset/238588/webkit has caused an API failure on iOS Failed TestWebKitAPI.SafeBrowsing.VisitUnsafeWebsite /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/SafeBrowsing.mm:215 Expected: (warning.subviews.firstObject.subviews[2].frame.size.height) > (0), actual: 0 vs 0 this was the first run that had the failure: https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1200 disregard my previous comment. I had the wrong revision |