Bug 192034 - Remove kCCNotVerified
Summary: Remove kCCNotVerified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-27 13:57 PST by Jiewen Tan
Modified: 2018-11-28 09:39 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.29 KB, patch)
2018-11-27 14:00 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2018-11-27 15:18 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-11-27 13:57:49 PST
Remove kCCNotVerified as the return code is never used by CCRSACryptorVerify.
Comment 1 Jiewen Tan 2018-11-27 13:58:04 PST
<rdar://problem/46235863>
Comment 2 Jiewen Tan 2018-11-27 14:00:38 PST
Created attachment 355776 [details]
Patch
Comment 3 Alexey Proskuryakov 2018-11-27 14:12:31 PST
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 4 Jiewen Tan 2018-11-27 14:25:56 PST
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.
Comment 5 Alexey Proskuryakov 2018-11-27 14:48:21 PST
> 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.
Comment 6 Jiewen Tan 2018-11-27 14:54:32 PST
(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.
Comment 7 Jiewen Tan 2018-11-27 15:18:20 PST
Created attachment 355792 [details]
Patch
Comment 8 Alexey Proskuryakov 2018-11-27 15:29:50 PST
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 9 Jiewen Tan 2018-11-27 15:35:43 PST
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.
Comment 10 WebKit Commit Bot 2018-11-27 16:25:29 PST
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 11 WebKit Commit Bot 2018-11-27 16:26:18 PST
Comment on attachment 355792 [details]
Patch

Clearing flags on attachment: 355792

Committed r238588: <https://trac.webkit.org/changeset/238588>
Comment 12 WebKit Commit Bot 2018-11-27 16:26:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Truitt Savell 2018-11-28 09:31:52 PST
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
Comment 14 Truitt Savell 2018-11-28 09:39:18 PST
disregard my previous comment. I had the wrong revision