Bug 172856 – [GCrypt] RSA-PSS support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GCrypt] RSA-PSS support

[Zan Dobersek]

Created attachment 311822 [details]
WIP

[Build Bot]

Attachment 311822 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:203:  CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:231:  CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 311822 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review

Now that Jiewen is a reviewer, I'll let him give the final r+. Just a couple nits:

> Source/WTF/wtf/Platform.h:1274
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || PLATFORM(GTK) || PLATFORM(WPE)

Shouldn't this be || USE(GCRYPT)?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:31
> +#if ENABLE(SUBTLE_CRYPTO) && HAVE(RSA_PSS)

This is confusing, since it implies that HAVE(RSA_PSS) can be false when compiling this file. I would remove it.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:198
> +    // a failure in any other case. OperationError should not be returned at this point.

Same as in the last patch, you should explain why.

[Jiewen Tan]

Comment on attachment 311822 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review

Looks good to me, and please address comments in gcryptVerify.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:43
> +static std::optional<PAL::CryptoDigest::Algorithm> hashCryptoDigestAlgorithm(CryptoAlgorithmIdentifier identifier)

Remember to move this into a utility file.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:61
> +static std::optional<const char*> hashAlgorithmName(CryptoAlgorithmIdentifier identifier)

Ditto.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:79
> +static std::optional<Vector<uint8_t>> mpiData(gcry_sexp_t paramSexp)

Ditto.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:198
>> +    // a failure in any other case. OperationError should not be returned at this point.
> 
> Same as in the last patch, you should explain why.

I start wondering maybe we shouldn't return any std::nullopt before as that will cause OperationError.

[Jiewen Tan]

Comment on attachment 311822 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review

>>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:198
>>> +    // a failure in any other case. OperationError should not be returned at this point.
>> 
>> Same as in the last patch, you should explain why.
> 
> I start wondering maybe we shouldn't return any std::nullopt before as that will cause OperationError.

Maybe we shouldn't even pass exceptionCallback to platformVerify as there won't be any exceptions till this level. I will address that in Bug 171983.

[Zan Dobersek]

Comment on attachment 311822 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review

>> Source/WTF/wtf/Platform.h:1274
>> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || PLATFORM(GTK) || PLATFORM(WPE)
> 
> Shouldn't this be || USE(GCRYPT)?

Could be, doesn't exist yet though. Should be simple to expose through CMake.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:31
>> +#if ENABLE(SUBTLE_CRYPTO) && HAVE(RSA_PSS)
> 
> This is confusing, since it implies that HAVE(RSA_PSS) can be false when compiling this file. I would remove it.

The cross-platform code still uses HAVE(RSA_PSS). I'll add a preprocessor error that's triggered if HAVE(RSA_PSS) is false here, which it shouldn't be because USE(GCRYPT) is at the moment definitely true if this file is being compiled.

[Zan Dobersek]

Created attachment 311954 [details]
Patch

[Build Bot]

Attachment 311954 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:145:  CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:173:  CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 311954 [details]
Patch

wpe port seems not happy.

[Zan Dobersek]

Created attachment 312058 [details]
Patch

[Build Bot]

Attachment 312058 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:145:  CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:173:  CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 312058 [details]
Patch

r=me. Thanks.

[Zan Dobersek]

Created attachment 312172 [details]
Patch for landing

[Build Bot]

Attachment 312172 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:145:  CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:173:  CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 312172 [details]
Patch for landing

Clearing flags on attachment: 312172

Committed r217877: <http://trac.webkit.org/changeset/217877>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.