Make SecItemShim to not send return value for SecItemAdd
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jun 2018 22:22:14 +0000 (22:22 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jun 2018 22:22:14 +0000 (22:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186789
<rdar://problem/40892596>

Reviewed by Brent Fulgham.

Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to serialize SecKeychainItemRef.
Otherwise, it would go through the weird route of serializing SecKeychainItemRef by asking Keychain for its persistent
reference. This route contradicts the purpose of SecItemShim, which is to proxy all Keychain operations to UIProcess.

Also, this patch removes the release assertion on encode(Encoder&, SecAccessControlRef) and decode(Decoder&, RetainPtr<SecAccessControlRef>&)
as they don't query Keychain.

* Shared/cf/ArgumentCodersCF.cpp:
(IPC::encode):
(IPC::decode):
* Shared/mac/SecItemShim.cpp:
(WebKit::sendSecItemRequest):
(WebKit::webSecItemAdd):
* UIProcess/mac/SecItemShimProxy.cpp:
(WebKit::SecItemShimProxy::secItemRequest):
* UIProcess/mac/SecItemShimProxy.h:
* UIProcess/mac/SecItemShimProxy.messages.in:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232990 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
Source/WebKit/Shared/mac/SecItemShim.cpp
Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp

index 8befbee..3c7cf39 100644 (file)
@@ -1,3 +1,29 @@
+2018-06-18  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Make SecItemShim to not send return value for SecItemAdd
+        https://bugs.webkit.org/show_bug.cgi?id=186789
+        <rdar://problem/40892596>
+
+        Reviewed by Brent Fulgham.
+
+        Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to serialize SecKeychainItemRef.
+        Otherwise, it would go through the weird route of serializing SecKeychainItemRef by asking Keychain for its persistent
+        reference. This route contradicts the purpose of SecItemShim, which is to proxy all Keychain operations to UIProcess.
+
+        Also, this patch removes the release assertion on encode(Encoder&, SecAccessControlRef) and decode(Decoder&, RetainPtr<SecAccessControlRef>&)
+        as they don't query Keychain.
+
+        * Shared/cf/ArgumentCodersCF.cpp:
+        (IPC::encode):
+        (IPC::decode):
+        * Shared/mac/SecItemShim.cpp:
+        (WebKit::sendSecItemRequest):
+        (WebKit::webSecItemAdd):
+        * UIProcess/mac/SecItemShimProxy.cpp:
+        (WebKit::SecItemShimProxy::secItemRequest):
+        * UIProcess/mac/SecItemShimProxy.h:
+        * UIProcess/mac/SecItemShimProxy.messages.in:
+
 2018-06-19  Sihui Liu  <sihui_liu@apple.com>
 
         REGRESSION (r231850): Cookie file cannot be read or written by network process
index b430e6c..69d991d 100644 (file)
@@ -771,7 +771,6 @@ bool decode(Decoder& decoder, RetainPtr<SecKeychainItemRef>& result)
 #if HAVE(SEC_ACCESS_CONTROL)
 void encode(Encoder& encoder, SecAccessControlRef accessControl)
 {
-    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
     RetainPtr<CFDataRef> data = adoptCF(SecAccessControlCopyData(accessControl));
     if (data)
         encode(encoder, data.get());
@@ -779,7 +778,6 @@ void encode(Encoder& encoder, SecAccessControlRef accessControl)
 
 bool decode(Decoder& decoder, RetainPtr<SecAccessControlRef>& result)
 {
-    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
     RetainPtr<CFDataRef> data;
     if (!decode(decoder, data))
         return false;
index 81db3e7..44533a6 100644 (file)
@@ -99,14 +99,19 @@ static OSStatus webSecItemCopyMatching(CFDictionaryRef query, CFTypeRef* result)
     return response->resultCode();
 }
 
-static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* result)
+static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* unusedResult)
 {
+    // Return value of SecItemAdd should be ignored for WebKit use cases. WebKit can't serialize SecKeychainItemRef, so we do not use it.
+    // If someone passes a result value to be populated, the API contract is being violated so we should assert.
+    if (unusedResult) {
+        ASSERT_NOT_REACHED();
+        return errSecParam;
+    }
+
     auto response = sendSecItemRequest(SecItemRequestData::Add, query);
     if (!response)
         return errSecInteractionNotAllowed;
 
-    if (result)
-        *result = response->resultObject().leakRef();
     return response->resultCode();
 }
 
index 21ea7b0..729cfdd 100644 (file)
@@ -76,9 +76,10 @@ void SecItemShimProxy::secItemRequest(const SecItemRequestData& request, SecItem
     }
 
     case SecItemRequestData::Add: {
-        CFTypeRef resultObject = 0;
-        OSStatus resultCode = SecItemAdd(request.query(), &resultObject);
-        response = SecItemResponseData(resultCode, adoptCF(resultObject).get());
+        // Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to
+        // serialize SecKeychainItemRef.
+        OSStatus resultCode = SecItemAdd(request.query(), nullptr);
+        response = SecItemResponseData(resultCode, nullptr);
         break;
     }