Bug 165835 – Handle key generation with empty challenge string

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

[John Wilander]

https://bugs.webkit.org/show_bug.cgi?id=160945 introduced an ASSERT(challenge.data()) that didn't catch empty challenge strings. Also, empty challenge strings are allowed:

"If the element has a challenge attribute, then let challenge be that attribute's value. Otherwise, let challenge be the empty string."
https://www.w3.org/TR/html5/forms.html#the-keygen-element

Email certificate generation at https://www.comodo.com/home/email-security/free-email-certificate.php broke because of https://bugs.webkit.org/show_bug.cgi?id=160945.

[John Wilander]

rdar://problem/29128710

[John Wilander]

Created attachment 297057 [details]
Patch

[Conrad Shultz]

Comment on attachment 297057 [details]
Patch

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

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:180
> +        signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Data = (uint8 *)strdup("\0");

Does this need to be freed at some point?

[John Wilander]

Created attachment 297104 [details]
Patch

[John Wilander]

(In reply to comment #3)
> Comment on attachment 297057 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297057&action=review
> 
> > Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:180
> > +        signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Data = (uint8 *)strdup("\0");
> 
> Does this need to be freed at some point?

Thanks! You're right. The old deallocation strategy was very different and covered this part too.

But after a conversation with Anders Carlsson I found a simpler fix that doesn't require string duplication. See new patch.

[Anders Carlsson]

Comment on attachment 297104 [details]
Patch

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

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:184
> +    if (!challenge.length()) {
> +        // Needed to account for the null terminator
> +        signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Length = 1;
> +    } else
> +        signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Length = challenge.length();

I'm wondering whether this can just be 

signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Length = challenge.length() + 1; 

always?

[John Wilander]

Created attachment 297109 [details]
Patch

[John Wilander]

(In reply to comment #6)
> Comment on attachment 297104 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297104&action=review
> 
> > Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:184
> > +    if (!challenge.length()) {
> > +        // Needed to account for the null terminator
> > +        signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Length = 1;
> > +    } else
> > +        signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Length = challenge.length();
> 
> I'm wondering whether this can just be 
> 
> signedPublicKeyAndChallenge.publicKeyAndChallenge.challenge.Length =
> challenge.length() + 1; 
> 
> always?

Seems to work. And it is aligned with the other place where we set the Length in a CSSM_DATA struct:

uint8 encodeNull[2] { SEC_ASN1_NULL, 0 };
...
signedPublicKeyAndChallenge.algorithmIdentifier.parameters.Data = (uint8 *)encodeNull;
signedPublicKeyAndChallenge.algorithmIdentifier.parameters.Length = 2;

See new patch.

[Anders Carlsson]

Comment on attachment 297109 [details]
Patch

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

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:180
> +    // Length needs to account for the null terminator

Add a period to make this a proper sentence.

[John Wilander]

Committed r209822: <http://trac.webkit.org/changeset/209822>