Bug 192034 – Remove kCCNotVerified

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Remove kCCNotVerified as the return code is never used by CCRSACryptorVerify.

[Jiewen Tan]

<rdar://problem/46235863>

[Jiewen Tan]

Created attachment 355776 [details]
Patch

[Alexey Proskuryakov]

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?

[Jiewen Tan]

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.

[Alexey Proskuryakov]

> 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.

[Jiewen Tan]

(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.

[Jiewen Tan]

Created attachment 355792 [details]
Patch

[Alexey Proskuryakov]

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?

[Jiewen Tan]

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.

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 355792 [details]
Patch

Clearing flags on attachment: 355792

Committed r238588: <https://trac.webkit.org/changeset/238588>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[Truitt Savell]

disregard my previous comment. I had the wrong revision