Bug 124569 – [curl] Improve ssl certificate storage and check

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Created attachment 217277 [details]
proposed patch

Storage and check the whole certificate chain, not just the root certificate.

[Brent Fulgham]

Comment on attachment 217277 [details]
proposed patch

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

Looks good overall.  I have some concern about the BIO_get_mem_data call (though I realize this wasn't code you added).  Can you review and let me know what you think?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:146
> +        unsigned char *certificateData;

unsigned char* certificateData;

> Source/WebCore/platform/network/curl/SSLHandle.cpp:147
> +        long len = BIO_get_mem_data(bio, &certificateData);

The BIO_get_mem_data documentation is pretty weak.  Does it ever return a negative value?  If not, why is the return value signed?  If it is negative, then the following code will do bad things.

(In reply to comment #1)
> (From update of attachment 217277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217277&action=review
> 
Thanks for the detailed review. 

> Looks good overall.  I have some concern about the BIO_get_mem_data call (though I realize this wasn't code you added).  Can you review and let me know what you think?
> 
BIO_get_mem_data() sets pp to a pointer to the start of the memory BIOs data and returns the total amount of data available. 
length = BIO_get_mem_data(bio, &certificateData);
// here - certificateData is a pointer to encoded data, length - length of data.

> > Source/WebCore/platform/network/curl/SSLHandle.cpp:146
> > +        unsigned char *certificateData;
> 
> unsigned char* certificateData;
>
I changed this. 
 
> > Source/WebCore/platform/network/curl/SSLHandle.cpp:147
> > +        long len = BIO_get_mem_data(bio, &certificateData);
> 
> The BIO_get_mem_data documentation is pretty weak.  Does it ever return a negative value?  If not, why is the return value signed?  If it is negative, then the following code will do bad things.
Yes, I made the change what is necessary to avoid unexpected behaviors in Bug119436 which this bug depends on.

Created attachment 217534 [details]
proposed patch

Created attachment 217537 [details]
proposed patch

Add missing ChangeLog

[Brent Fulgham]

Comment on attachment 217537 [details]
proposed patch

r=me.

[WebKit Commit Bot]

Comment on attachment 217537 [details]
proposed patch

Clearing flags on attachment: 217537

Committed r159632: <http://trac.webkit.org/changeset/159632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.