Bug 124569

Summary: [curl] Improve ssl certificate storage and check
Product: WebKit Reporter: sipka
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, galpeter, sipka
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 119436    
Bug Blocks: 117300    
Attachments:
Description Flags
proposed patch
none
proposed patch
none
proposed patch none

Description sipka 2013-11-19 01:08:18 PST
Created attachment 217277 [details]
proposed patch

Storage and check the whole certificate chain, not just the root certificate.
Comment 1 Brent Fulgham 2013-11-19 16:53:02 PST
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.
Comment 2 sipka 2013-11-21 01:47:39 PST
(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.
Comment 3 sipka 2013-11-21 01:48:36 PST
Created attachment 217534 [details]
proposed patch
Comment 4 sipka 2013-11-21 02:04:28 PST
Created attachment 217537 [details]
proposed patch

Add missing ChangeLog
Comment 5 Brent Fulgham 2013-11-21 09:14:07 PST
Comment on attachment 217537 [details]
proposed patch

r=me.
Comment 6 WebKit Commit Bot 2013-11-21 09:40:42 PST
Comment on attachment 217537 [details]
proposed patch

Clearing flags on attachment: 217537

Committed r159632: <http://trac.webkit.org/changeset/159632>
Comment 7 WebKit Commit Bot 2013-11-21 09:40:45 PST
All reviewed patches have been landed.  Closing bug.