2011-05-28 Simon Fraser <simon.fraser@apple.com>
Reviewed by Dan Bernstein, Maciej Stachowiak.
Denying access to your keychain on login crashes WebKit2
https://bugs.webkit.org/show_bug.cgi?id=61695
<rdar://problem/9520570>
Fix two sources of crashes if you hit the Deny button when WebKit2 is
doing HTTP authentication.
First, SecKeychainItemRequestData::attributeList() failed to initialize the
length and data members of SecKeychainAttributes in the list if there was no data.
This caused invalid memory reads later.
Second, returning a non-zero error from the SecKeychainItemCopyContent shim method
would cause a later crash in a system framework, which is not set up to handle
errors. Instead, we always return noErr, and allow the authentication to fail.
Finally, paranoically initialize the SecKeychainItemContext in two places
to avoid uninitialized data members, and initialize length and outData
to 0 in secKeychainItemCopyContent() in case SecKeychainItemCopyContent()
fails to set them on error.
* Shared/mac/SecKeychainItemRequestData.cpp:
(WebKit::SecKeychainItemRequestData::attributeList):
* UIProcess/mac/WebProcessProxyMac.mm:
(WebKit::WebProcessProxy::secKeychainItemCopyContent):
* WebProcess/mac/KeychainItemShimMethods.mm:
(WebKit::webSecKeychainItemCopyContent):
(WebKit::webSecKeychainItemCreateFromContent):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@87627 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index 1c187d8..4c1e3d7 100644
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,35 @@
+2011-05-28 Simon Fraser <simon.fraser@apple.com>
+
+ Reviewed by Dan Bernstein, Maciej Stachowiak.
+
+ Denying access to your keychain on login crashes WebKit2
+ https://bugs.webkit.org/show_bug.cgi?id=61695
+ <rdar://problem/9520570>
+
+ Fix two sources of crashes if you hit the Deny button when WebKit2 is
+ doing HTTP authentication.
+
+ First, SecKeychainItemRequestData::attributeList() failed to initialize the
+ length and data members of SecKeychainAttributes in the list if there was no data.
+ This caused invalid memory reads later.
+
+ Second, returning a non-zero error from the SecKeychainItemCopyContent shim method
+ would cause a later crash in a system framework, which is not set up to handle
+ errors. Instead, we always return noErr, and allow the authentication to fail.
+
+ Finally, paranoically initialize the SecKeychainItemContext in two places
+ to avoid uninitialized data members, and initialize length and outData
+ to 0 in secKeychainItemCopyContent() in case SecKeychainItemCopyContent()
+ fails to set them on error.
+
+ * Shared/mac/SecKeychainItemRequestData.cpp:
+ (WebKit::SecKeychainItemRequestData::attributeList):
+ * UIProcess/mac/WebProcessProxyMac.mm:
+ (WebKit::WebProcessProxy::secKeychainItemCopyContent):
+ * WebProcess/mac/KeychainItemShimMethods.mm:
+ (WebKit::webSecKeychainItemCopyContent):
+ (WebKit::webSecKeychainItemCreateFromContent):
+
2011-05-28 Anders Carlsson <andersca@apple.com>
Reviewed by Dan Bernstein.
diff --git a/Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp b/Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp
index b7f7c45..7f988ff 100644
--- a/Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp
+++ b/Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp
@@ -101,8 +101,11 @@
for (size_t i = 0; i < m_attributeList->count; ++i) {
m_attributeList->attr[i].tag = m_keychainAttributes[i].tag;
- if (!m_keychainAttributes[i].data)
+ if (!m_keychainAttributes[i].data) {
+ m_attributeList->attr[i].length = 0;
+ m_attributeList->attr[i].data = 0;
continue;
+ }
m_attributeList->attr[i].length = CFDataGetLength(m_keychainAttributes[i].data.get());
m_attributeList->attr[i].data = const_cast<void*>(static_cast<const void*>(CFDataGetBytePtr(m_keychainAttributes[i].data.get())));
diff --git a/Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm b/Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm
index a327ad3..1d35a03 100644
--- a/Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm
+++ b/Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm
@@ -82,8 +82,8 @@
SecKeychainItemRef item = request.keychainItem();
SecItemClass itemClass;
SecKeychainAttributeList* attrList = request.attributeList();
- UInt32 length;
- void* outData;
+ UInt32 length = 0;
+ void* outData = 0;
OSStatus resultCode = SecKeychainItemCopyContent(item, &itemClass, attrList, &length, &outData);
diff --git a/Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm b/Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm
index 3f2521f..e7e9922 100644
--- a/Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm
+++ b/Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm
@@ -208,6 +208,7 @@
static OSStatus webSecKeychainItemCopyContent(SecKeychainItemRef item, SecItemClass* itemClass, SecKeychainAttributeList* attrList, UInt32* length, void** outData)
{
SecKeychainItemContext context;
+ memset(&context, 0, sizeof(SecKeychainItemContext));
context.item = item;
context.resultItemClass = itemClass;
context.attributeList = attrList;
@@ -216,7 +217,9 @@
callOnMainThreadAndWait(webSecKeychainItemCopyContentOnMainThread, &context);
- return context.resultCode;
+ // FIXME: should return context.resultCode. Returning noErr is a workaround for <rdar://problem/9520886>;
+ // the authentication should fail anyway, since on error no data will be returned.
+ return noErr;
}
static void webSecKeychainItemCreateFromContentOnMainThread(void* voidContext)
@@ -239,6 +242,7 @@
static OSStatus webSecKeychainItemCreateFromContent(SecItemClass itemClass, SecKeychainAttributeList* attrList, UInt32 length, const void* data, SecKeychainItemRef *item)
{
SecKeychainItemContext context;
+ memset(&context, 0, sizeof(SecKeychainItemContext));
context.initialItemClass = itemClass;
context.attributeList = attrList;
context.length = length;
