Bug 228879 – Add console logging to encourage the use of authenticated encryption in WebCrypto

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

[Kate Cheney]

Add console logging to encourage the use of authenticated encryption in WebCrypto

[Kate Cheney]

Created attachment 435096 [details]
Patch

[Kate Cheney]

rdar://80655397

[Brent Fulgham]

Comment on attachment 435096 [details]
Patch

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

r=me

> Source/WebCore/crypto/SubtleCrypto.cpp:538
> +        scriptExecutionContext()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "AES-CBC and AES-CTR do not provide authentication by default, and implementing it manually can result in minor, but serious mistakes. We recommended using authenticated encryption like GCM to protect against chosen-ciphertext attacks.");

This might be better as a separate function:

void SubtleCrypto::addAuthenticatedEncryptionWarningIfNecessary()
{
    if (key.algorithmIdentifier() == CryptoAlgorithmIdentifier::AES_CBC || key.algorithmIdentifier() == CryptoAlgorithmIdentifier::AES_CTR)
        scriptExecutionContext()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "AES-CBC and AES-CTR do not provide authentication by default, and implementing it manually can result in minor, but serious mistakes. We recommended using authenticated encryption like GCM to protect against chosen-ciphertext attacks.");
}

Then the two implementations would always be consistent in language and format, and if we want to warn about more things we could just add them in one place.

> Source/WebCore/crypto/SubtleCrypto.cpp:579
> +        scriptExecutionContext()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "AES-CBC and AES-CTR do not provide authentication by default, and implementing it manually can result in minor, but serious mistakes. We recommended using authenticated encryption like GCM to protect against chosen-ciphertext attacks.");

... and this could just be replaced with:

addAuthenticatedEncryptionWarningIfNecessary();

[Kate Cheney]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 435096 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435096&action=review
> 
> r=me
> 
> > Source/WebCore/crypto/SubtleCrypto.cpp:538
> > +        scriptExecutionContext()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "AES-CBC and AES-CTR do not provide authentication by default, and implementing it manually can result in minor, but serious mistakes. We recommended using authenticated encryption like GCM to protect against chosen-ciphertext attacks.");
> 
> This might be better as a separate function:
> 
> void SubtleCrypto::addAuthenticatedEncryptionWarningIfNecessary()
> {
>     if (key.algorithmIdentifier() == CryptoAlgorithmIdentifier::AES_CBC ||
> key.algorithmIdentifier() == CryptoAlgorithmIdentifier::AES_CTR)
>         scriptExecutionContext()->addConsoleMessage(MessageSource::Security,
> MessageLevel::Warning, "AES-CBC and AES-CTR do not provide authentication by
> default, and implementing it manually can result in minor, but serious
> mistakes. We recommended using authenticated encryption like GCM to protect
> against chosen-ciphertext attacks.");
> }
> 
> Then the two implementations would always be consistent in language and
> format, and if we want to warn about more things we could just add them in
> one place.
> 
> > Source/WebCore/crypto/SubtleCrypto.cpp:579
> > +        scriptExecutionContext()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "AES-CBC and AES-CTR do not provide authentication by default, and implementing it manually can result in minor, but serious mistakes. We recommended using authenticated encryption like GCM to protect against chosen-ciphertext attacks.");
> 
> ... and this could just be replaced with:
> 
> addAuthenticatedEncryptionWarningIfNecessary();

Good idea, will change.

[Kate Cheney]

Created attachment 435097 [details]
Patch

[Kate Cheney]

Will land after EWS is green.

[Kate Cheney]

Created attachment 435188 [details]
Patch

[Kate Cheney]

Updated test expectations for imported tests to avoid logging console output to stdout. Also some tests called encrypt/decrypt up to 1000 times to test for crashing. To avoid 1000 lines of console logging in test expectations I also marked these with the "DumpJSConsoleLogInStdErr" expectation. Awaiting green bots.

[Kate Cheney]

Created attachment 435195 [details]
Patch

[EWS]

Committed r280790 (240369@main): <https://commits.webkit.org/240369@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435195 [details].