57707 2011-04-02 11:42:53 -0700 WebKit2: Specify the certificate store in WKBundleSetClientCertificate() 2011-04-02 15:00:24 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 1 jeffm jeffm mitz oldest_to_newest 378405 0 jeffm 2011-04-02 11:42:53 -0700 The PCCERT_CONTEXT for the client certificate we create from the message from the UI process doesn't contain enough information to actually use it in a request, we need to get the real certificate from the certificate store (which is typically the "MY" store). 378407 1 87980 jeffm 2011-04-02 11:47:54 -0700 Created attachment 87980 Patch 378412 2 87980 mitz 2011-04-02 12:03:27 -0700 Comment on attachment 87980 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87980&action=review > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:114 > + ASSERT(!certificateSystemStoreName.isEmpty()); You can use ASSERT_ARG here. > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:122 > + ASSERT(certStore); I don’t know this Windows API, but I’m not sure it’s correct to assert this. At the very least it asserts that the bundle code, which is not part of WebKit, isn’t sending a bogus store name. But even if the store name was correct at some point, isn’t it possible that by the time we reach this statement, the store has been deleted, renamed, or otherwise made un-openable? Maybe LOG_ERROR() is more appropriate than ASSERT(). > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:127 > + ASSERT(realCert); Similar comment here. > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:134 > + // We can't close certStore here, since the certificate is still in use. So we never close it. Is this OK? 378423 3 jeffm 2011-04-02 13:54:17 -0700 (In reply to comment #2) > (From update of attachment 87980 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87980&action=review > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:114 > > + ASSERT(!certificateSystemStoreName.isEmpty()); > > You can use ASSERT_ARG here. Will do. > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:122 > > + ASSERT(certStore); > > I don’t know this Windows API, but I’m not sure it’s correct to assert this. At the very least it asserts that the bundle code, which is not part of WebKit, isn’t sending a bogus store name. But even if the store name was correct at some point, isn’t it possible that by the time we reach this statement, the store has been deleted, renamed, or otherwise made un-openable? Maybe LOG_ERROR() is more appropriate than ASSERT(). In theory, a system certificate store should always be available, but you're right, I should use LOG_ERROR(). > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:127 > > + ASSERT(realCert); > > Similar comment here. Yes, I'll change this to LOG_ERROR() as well. > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:134 > > + // We can't close certStore here, since the certificate is still in use. > > So we never close it. Is this OK? Yes, this is what we do in Safari as well. 378432 4 jeffm 2011-04-02 15:00:24 -0700 Committed r82776: <http://trac.webkit.org/changeset/82776> 87980 2011-04-02 11:47:54 -0700 2011-04-02 12:03:27 -0700 Patch bug-57707-20110402114745.patch text/plain 6055 jeffm SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDgyNzcxKQorKysgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjEgQEAKKzIwMTEtMDQtMDIgIEplZmYgTWls bGVyICA8amVmZm1AYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFdlYktpdDI6IFNwZWNpZnkgdGhlIGNlcnRpZmljYXRlIHN0b3JlIGlu IFdLQnVuZGxlU2V0Q2xpZW50Q2VydGlmaWNhdGUoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTc3MDcKKworICAgICAgICBJbmNsdWRlIHRoZSBuYW1l IG9mIHRoZSBzeXN0ZW0gY2VydGlmaWNhdGUgc3RvcmUgdGhhdCB0aGUgY2xpZW50IGNlcnRpZmlj YXRlIGNhbWUgZnJvbSBpbiBXS0J1bmRsZVNldENsaWVudENlcnRpZmljYXRlKCkuCisKKyAgICAg ICAgVGhlIFBDQ0VSVF9DT05URVhUIGZvciB0aGUgY2xpZW50IGNlcnRpZmljYXRlIHdlIGNyZWF0 ZSBmcm9tIHRoZSBtZXNzYWdlIGZyb20gdGhlIFVJIHByb2Nlc3MgZG9lc24ndCBjb250YWluIGVu b3VnaCBpbmZvcm1hdGlvbiB0byBhY3R1YWxseSB1c2UgaXQgaW4gYSByZXF1ZXN0LCB3ZSBuZWVk IHRvIGdldCB0aGUgcmVhbCBjZXJ0aWZpY2F0ZSBmcm9tIHRoZSBjZXJ0aWZpY2F0ZSBzdG9yZSAo d2hpY2ggaXMgdHlwaWNhbGx5IHRoZSAiTVkiIHN0b3JlKS4KKworICAgICAgICAqIFdlYlByb2Nl c3MvSW5qZWN0ZWRCdW5kbGUvQVBJL2Mvd2luL1dLQnVuZGxlUHJpdmF0ZVdpbi5jcHA6CisgICAg ICAgIChXS0J1bmRsZVNldENsaWVudENlcnRpZmljYXRlKTogQWRkIGNlcnRpZmljYXRlU3lzdGVt U3RvcmVOYW1lIHRvIHBhcmFtZXRlcnMuCisgICAgICAgICogV2ViUHJvY2Vzcy9JbmplY3RlZEJ1 bmRsZS9BUEkvYy93aW4vV0tCdW5kbGVQcml2YXRlV2luLmg6IEFkZCBjZXJ0aWZpY2F0ZVN5c3Rl bVN0b3JlTmFtZSB0byBXS0J1bmRsZVNldENsaWVudENlcnRpZmljYXRlKCkgcGFyYW1ldGVycy4K KyAgICAgICAgKiBXZWJQcm9jZXNzL0luamVjdGVkQnVuZGxlL0luamVjdGVkQnVuZGxlLmg6IEFk ZCBjZXJ0aWZpY2F0ZVN5c3RlbVN0b3JlTmFtZSB0byBzZXRDbGllbnRDZXJ0aWZpY2F0ZSgpIHBh cmFtZXRlcnMuCisgICAgICAgICogV2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS93aW4vSW5qZWN0 ZWRCdW5kbGVXaW4uY3BwOgorICAgICAgICAoV2ViS2l0OjpJbmplY3RlZEJ1bmRsZTo6c2V0Q2xp ZW50Q2VydGlmaWNhdGUpOiBSZWFkIHRoZSByZWFsIGNlcnRpZmljYXRlIGZyb20gdGhlIGNlcnRp ZmljYXRlIHN0b3JlLgorCiAyMDExLTA0LTAxICBKb24gTGVlICA8am9ubGVlQGFwcGxlLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KSW5kZXg6IFNvdXJjZS9XZWJLaXQy L1dlYlByb2Nlc3MvSW5qZWN0ZWRCdW5kbGUvSW5qZWN0ZWRCdW5kbGUuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0luamVjdGVkQnVuZGxlL0luamVjdGVkQnVuZGxl LmgJKHJldmlzaW9uIDgyNzYzKQorKysgU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3Rl ZEJ1bmRsZS9JbmplY3RlZEJ1bmRsZS5oCSh3b3JraW5nIGNvcHkpCkBAIC04Miw3ICs4Miw3IEBA IHB1YmxpYzoKICAgICB2b2lkIHBvc3RTeW5jaHJvbm91c01lc3NhZ2UoY29uc3QgU3RyaW5nJiwg QVBJT2JqZWN0KiwgUmVmUHRyPEFQSU9iamVjdD4mIHJldHVybkRhdGEpOwogI2lmIFBMQVRGT1JN KFdJTikKICAgICB2b2lkIHNldEhvc3RBbGxvd3NBbnlIVFRQU0NlcnRpZmljYXRlKGNvbnN0IFN0 cmluZyYpOwotICAgIHZvaWQgc2V0Q2xpZW50Q2VydGlmaWNhdGUoY29uc3QgU3RyaW5nJiwgY29u c3QgV2ViQ2VydGlmaWNhdGVJbmZvKik7CisgICAgdm9pZCBzZXRDbGllbnRDZXJ0aWZpY2F0ZShj b25zdCBTdHJpbmcmIGhvc3QsIGNvbnN0IFN0cmluZyYgY2VydGlmaWNhdGVTeXN0ZW1TdG9yZU5h bWUsIGNvbnN0IFdlYkNlcnRpZmljYXRlSW5mbyopOwogI2VuZGlmCiAKICAgICAvLyBUZXN0UnVu bmVyIG9ubHkgU1BJCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0luamVjdGVkQnVu ZGxlL0FQSS9jL3dpbi9XS0J1bmRsZVByaXZhdGVXaW4uY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9XZWJLaXQyL1dlYlByb2Nlc3MvSW5qZWN0ZWRCdW5kbGUvQVBJL2Mvd2luL1dLQnVuZGxlUHJp dmF0ZVdpbi5jcHAJKHJldmlzaW9uIDgyNzYzKQorKysgU291cmNlL1dlYktpdDIvV2ViUHJvY2Vz cy9JbmplY3RlZEJ1bmRsZS9BUEkvYy93aW4vV0tCdW5kbGVQcml2YXRlV2luLmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtMzcsNyArMzcsNyBAQCB2b2lkIFdLQnVuZGxlU2V0SG9zdEFsbG93c0FueUhU VFBTQ2VydGlmCiAgICAgdG9JbXBsKGJ1bmRsZVJlZiktPnNldEhvc3RBbGxvd3NBbnlIVFRQU0Nl cnRpZmljYXRlKHRvV1RGU3RyaW5nKGhvc3QpKTsKIH0KIAotdm9pZCBXS0J1bmRsZVNldENsaWVu dENlcnRpZmljYXRlKFdLQnVuZGxlUmVmIGJ1bmRsZVJlZiwgV0tTdHJpbmdSZWYgaG9zdCwgV0tD ZXJ0aWZpY2F0ZUluZm9SZWYgY2VydGlmaWNhdGVJbmZvUmVmKQordm9pZCBXS0J1bmRsZVNldENs aWVudENlcnRpZmljYXRlKFdLQnVuZGxlUmVmIGJ1bmRsZVJlZiwgV0tTdHJpbmdSZWYgaG9zdCwg V0tTdHJpbmdSZWYgY2VydGlmaWNhdGVTeXN0ZW1TdG9yZU5hbWUsIFdLQ2VydGlmaWNhdGVJbmZv UmVmIGNlcnRpZmljYXRlSW5mb1JlZikKIHsKLSAgICB0b0ltcGwoYnVuZGxlUmVmKS0+c2V0Q2xp ZW50Q2VydGlmaWNhdGUodG9XVEZTdHJpbmcoaG9zdCksIHRvSW1wbChjZXJ0aWZpY2F0ZUluZm9S ZWYpKTsKKyAgICB0b0ltcGwoYnVuZGxlUmVmKS0+c2V0Q2xpZW50Q2VydGlmaWNhdGUodG9XVEZT dHJpbmcoaG9zdCksIHRvV1RGU3RyaW5nKGNlcnRpZmljYXRlU3lzdGVtU3RvcmVOYW1lKSwgdG9J bXBsKGNlcnRpZmljYXRlSW5mb1JlZikpOwogfQpJbmRleDogU291cmNlL1dlYktpdDIvV2ViUHJv Y2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy93aW4vV0tCdW5kbGVQcml2YXRlV2luLmgKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy93 aW4vV0tCdW5kbGVQcml2YXRlV2luLmgJKHJldmlzaW9uIDgyNzYzKQorKysgU291cmNlL1dlYktp dDIvV2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy93aW4vV0tCdW5kbGVQcml2YXRlV2lu LmgJKHdvcmtpbmcgY29weSkKQEAgLTMzLDcgKzMzLDcgQEAgZXh0ZXJuICJDIiB7CiAjZW5kaWYK IAogV0tfRVhQT1JUIHZvaWQgV0tCdW5kbGVTZXRIb3N0QWxsb3dzQW55SFRUUFNDZXJ0aWZpY2F0 ZShXS0J1bmRsZVJlZiBidW5kbGUsIFdLU3RyaW5nUmVmIGhvc3QpOwotV0tfRVhQT1JUIHZvaWQg V0tCdW5kbGVTZXRDbGllbnRDZXJ0aWZpY2F0ZShXS0J1bmRsZVJlZiBidW5kbGUsIFdLU3RyaW5n UmVmIGhvc3QsIFdLQ2VydGlmaWNhdGVJbmZvUmVmIGNlcnRpZmljYXRlSW5mbyk7CitXS19FWFBP UlQgdm9pZCBXS0J1bmRsZVNldENsaWVudENlcnRpZmljYXRlKFdLQnVuZGxlUmVmIGJ1bmRsZSwg V0tTdHJpbmdSZWYgaG9zdCwgV0tTdHJpbmdSZWYgY2VydGlmaWNhdGVTeXN0ZW1TdG9yZU5hbWUs IFdLQ2VydGlmaWNhdGVJbmZvUmVmIGNlcnRpZmljYXRlSW5mbyk7CiAKICNpZmRlZiBfX2NwbHVz cGx1cwogfQpJbmRleDogU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS93 aW4vSW5qZWN0ZWRCdW5kbGVXaW4uY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1dl YlByb2Nlc3MvSW5qZWN0ZWRCdW5kbGUvd2luL0luamVjdGVkQnVuZGxlV2luLmNwcAkocmV2aXNp b24gODI3NjMpCisrKyBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0luamVjdGVkQnVuZGxlL3dp bi9JbmplY3RlZEJ1bmRsZVdpbi5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTk5LDcgKzk5LDcgQEAg dm9pZCBJbmplY3RlZEJ1bmRsZTo6c2V0SG9zdEFsbG93c0FueUhUVAogI2VuZGlmCiB9CiAKLXZv aWQgSW5qZWN0ZWRCdW5kbGU6OnNldENsaWVudENlcnRpZmljYXRlKGNvbnN0IFN0cmluZyYgaG9z dCwgY29uc3QgV2ViQ2VydGlmaWNhdGVJbmZvKiBjZXJ0aWZpY2F0ZUluZm8pCit2b2lkIEluamVj dGVkQnVuZGxlOjpzZXRDbGllbnRDZXJ0aWZpY2F0ZShjb25zdCBTdHJpbmcmIGhvc3QsIGNvbnN0 IFN0cmluZyYgY2VydGlmaWNhdGVTeXN0ZW1TdG9yZU5hbWUsIGNvbnN0IFdlYkNlcnRpZmljYXRl SW5mbyogY2VydGlmaWNhdGVJbmZvKQogewogI2lmIFVTRShDRk5FVFdPUkspCiAgICAgQVNTRVJU KGNlcnRpZmljYXRlSW5mbyk7CkBAIC0xMTEsNyArMTExLDI3IEBAIHZvaWQgSW5qZWN0ZWRCdW5k bGU6OnNldENsaWVudENlcnRpZmljYXQKICAgICBpZiAoY2VydGlmaWNhdGVDaGFpbi5zaXplKCkg IT0gMSkKICAgICAgICAgcmV0dXJuOwogICAgIAotICAgIFJlc291cmNlSGFuZGxlOjpzZXRDbGll bnRDZXJ0aWZpY2F0ZShob3N0LCBXZWJDb3JlOjpjb3B5Q2VydGlmaWNhdGVUb0RhdGEoY2VydGlm aWNhdGVDaGFpbi5maXJzdCgpKS5nZXQoKSk7CisgICAgQVNTRVJUKCFjZXJ0aWZpY2F0ZVN5c3Rl bVN0b3JlTmFtZS5pc0VtcHR5KCkpOworICAgIGlmIChjZXJ0aWZpY2F0ZVN5c3RlbVN0b3JlTmFt ZS5pc0VtcHR5KCkpCisgICAgICAgIHJldHVybjsKKyAgICAKKyAgICAvLyBUaGUgUENDRVJUX0NP TlRFWFQgaW4gdGhlIFdlYkNlcnRpZmljYXRlSW5mbyB3ZSBjcmVhdGVkIHVzaW5nIHRoZSBtZXNz YWdlIGZyb20gdGhlIFVJIHByb2Nlc3MgZG9lc24ndCBjb250YWluIGVub3VnaCBpbmZvcm1hdGlv bgorICAgIC8vIHRvIGFjdHVhbGx5IHVzZSBpdCBpbiBhIHJlcXVlc3QsIHdlIG5lZWQgdG8gZ2V0 IHRoZSByZWFsIGNlcnRpZmljYXRlIGZyb20gdGhlIGNlcnRpZmljYXRlIHN0b3JlICh3aGljaCBp cyB0eXBpY2FsbHkgdGhlICJNWSIgc3RvcmUpLgorICAgIFN0cmluZyBtdXRhYmxlQ2VydGlmaWNh dGVTeXN0ZW1TdG9yZU5hbWUgPSBjZXJ0aWZpY2F0ZVN5c3RlbVN0b3JlTmFtZTsKKyAgICBIQ0VS VFNUT1JFIGNlcnRTdG9yZSA9IDo6Q2VydE9wZW5TeXN0ZW1TdG9yZSgwLCBtdXRhYmxlQ2VydGlm aWNhdGVTeXN0ZW1TdG9yZU5hbWUuY2hhcmFjdGVyc1dpdGhOdWxsVGVybWluYXRpb24oKSk7Cisg ICAgQVNTRVJUKGNlcnRTdG9yZSk7CisgICAgaWYgKCFjZXJ0U3RvcmUpCisgICAgICAgIHJldHVy bjsKKyAgICAKKyAgICBQQ0NFUlRfQ09OVEVYVCByZWFsQ2VydCA9IDo6Q2VydEZpbmRDZXJ0aWZp Y2F0ZUluU3RvcmUoY2VydFN0b3JlLCBYNTA5X0FTTl9FTkNPRElORyB8IFBLQ1NfN19BU05fRU5D T0RJTkcsIDAsIENFUlRfRklORF9FWElTVElORywgY2VydGlmaWNhdGVDaGFpbi5maXJzdCgpLCAw KTsKKyAgICBBU1NFUlQocmVhbENlcnQpOworICAgIGlmICghcmVhbENlcnQpCisgICAgICAgIHJl dHVybjsKKworICAgIFJlc291cmNlSGFuZGxlOjpzZXRDbGllbnRDZXJ0aWZpY2F0ZShob3N0LCBX ZWJDb3JlOjpjb3B5Q2VydGlmaWNhdGVUb0RhdGEocmVhbENlcnQpLmdldCgpKTsKKyAgICBDZXJ0 RnJlZUNlcnRpZmljYXRlQ29udGV4dChyZWFsQ2VydCk7CisKKyAgICAvLyBXZSBjYW4ndCBjbG9z ZSBjZXJ0U3RvcmUgaGVyZSwgc2luY2UgdGhlIGNlcnRpZmljYXRlIGlzIHN0aWxsIGluIHVzZS4K ICNlbmRpZgogfQogCg==