Bug 46450 – Add Windows implementation of PlatformCertificateInfo

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

Windows needs an implementation of PlatformCertificateInfo.

[Sam Weinig]

Created attachment 68653 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/win/PlatformCertificateInfo.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/win/PlatformCertificateInfo.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/win/WKCertificateInfoWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 7 files


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

[Adam Roben (:aroben)]

Comment on attachment 68653 [details]
Patch

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

> WebKit2/Shared/win/PlatformCertificateInfo.cpp:54
> +    RetainPtr<CFDictionaryRef> certificateInfo = wkGetSSLCertificateInfo(cfResponse);
> +    if (!certificateInfo)
> +        return;

Why the RetainPtr? There's no need to retain/release the dictionary in this function.

> WebKit2/Shared/win/PlatformCertificateInfo.cpp:60
> +    m_certificateContext = CertDuplicateCertificateContext((PCCERT_CONTEXT)data);

static_cast would be nicer.

Seems like this file needs a sprinkling of :: on the CryptoAPI calls.

> WebKit2/Shared/win/PlatformCertificateInfo.cpp:118
> +    nameSize = CertGetNameString(certificate, dwType, 0, pvTypePara, 0, 0);
> +    if (!nameSize)
> +        return 0;
> +    OwnArrayPtr<WCHAR> name(new WCHAR[nameSize - 1]);
> +    CertGetNameString(certificate, dwType, 0, pvTypePara, name.get(), nameSize);

I think you're causing a buffer overrun here. You're telling CertGetNameString that the buffer is nameSize characters long, but it's only nameSize - 1 characters long!

You should also use ::CertGetNameStringW.

[Sam Weinig]

(In reply to comment #3)
> (From update of attachment 68653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68653&action=review
> 
> > WebKit2/Shared/win/PlatformCertificateInfo.cpp:54
> > +    RetainPtr<CFDictionaryRef> certificateInfo = wkGetSSLCertificateInfo(cfResponse);
> > +    if (!certificateInfo)
> > +        return;
> 
> Why the RetainPtr? There's no need to retain/release the dictionary in this function.

Quite true. Removed.

> 
> > WebKit2/Shared/win/PlatformCertificateInfo.cpp:60
> > +    m_certificateContext = CertDuplicateCertificateContext((PCCERT_CONTEXT)data);
> 
> static_cast would be nicer.
> 
> Seems like this file needs a sprinkling of :: on the CryptoAPI calls.
> 

Fixed both.

> > WebKit2/Shared/win/PlatformCertificateInfo.cpp:118
> > +    nameSize = CertGetNameString(certificate, dwType, 0, pvTypePara, 0, 0);
> > +    if (!nameSize)
> > +        return 0;
> > +    OwnArrayPtr<WCHAR> name(new WCHAR[nameSize - 1]);
> > +    CertGetNameString(certificate, dwType, 0, pvTypePara, name.get(), nameSize);
> 
> I think you're causing a buffer overrun here. You're telling CertGetNameString that the buffer is nameSize characters long, but it's only nameSize - 1 characters long!
> 
> You should also use ::CertGetNameStringW.

Ok. This was debugging code I stole from somewhere else, but I will fix it.

[Sam Weinig]

Landed in r68260 without the function.