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 (file)
--- 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 (file)
--- a/Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp
+++ b/Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp
@@ -101,8 +101,11 @@ SecKeychainAttributeList* SecKeychainItemRequestData::attributeList() const
 
     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 (file)
--- a/Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm
+++ b/Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm
@@ -82,8 +82,8 @@ void WebProcessProxy::secKeychainItemCopyContent(const SecKeychainItemRequestDat
     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 (file)
--- a/Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm
+++ b/Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm
@@ -208,6 +208,7 @@ static void webSecKeychainItemCopyContentOnMainThread(void* voidContext)
 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 @@ static OSStatus webSecKeychainItemCopyContent(SecKeychainItemRef item, SecItemCl
 
     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 void webSecKeychainItemCreateFromContentOnMainThread(void* voidContext)
 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;