Make C_Sign error handling spec compliant.
authorWerner Koch <wk@gnupg.org>
Wed, 20 Feb 2019 08:29:42 +0000 (09:29 +0100)
committerWerner Koch <wk@gnupg.org>
Wed, 20 Feb 2019 08:29:42 +0000 (09:29 +0100)
* src/slots.c (session_sign): Clear the flag sign-initialized flag on
error.
* src/p11-signinit.c (C_SignInit): Minor rework.
* src/p11-sign.c (C_Sign): Ditto.

Signed-off-by: Werner Koch <wk@gnupg.org>
src/p11-sign.c
src/p11-signinit.c
src/slots.c

index 870f08c..4f2ae7d 100644 (file)
  * If the function returns CKR_BUFFER_TOO_SMALL no further C_SignInit
  * is required, instead the function can be called again with a larger
  * buffer.  On a successful operation CKR_OK is returned and other
- * signatures may be created without an new C_SignInit.  On all other
+ * signatures may be created without a new C_SignInit.  On all other
  * return codes a new C_SignInit is required.
+ *
+ * In contrast to the specs the return code CKR_ARGUMENTS_BAD may not
+ * require a new C_SignInit because this can be considered a bug in
+ * the caller's code.
  */
 CK_RV CK_SPEC
 C_Sign (CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData, CK_ULONG ulDataLen,
@@ -46,7 +50,7 @@ C_Sign (CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData, CK_ULONG ulDataLen,
 {
   CK_RV err = CKR_OK;
   slot_iterator_t slot;
-  session_iterator_t session;
+  session_iterator_t sid;
 
   if (pData == NULL_PTR || pulSignatureLen == NULL_PTR)
     return CKR_ARGUMENTS_BAD;
@@ -55,18 +59,11 @@ C_Sign (CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData, CK_ULONG ulDataLen,
   if (err)
     return err;
 
-  err = slots_lookup_session (hSession, &slot, &session);
-  if (err)
-    goto out;
-
-  /* FIXME: Check that C_SignInit has been called.  */
-
-  err = session_sign (slot, session, pData, ulDataLen,
-                     pSignature, pulSignatureLen);
+  err = slots_lookup_session (hSession, &slot, &sid);
+  if (!err)
+    err = session_sign (slot, sid, pData, ulDataLen,
+                        pSignature, pulSignatureLen);
 
- out:
-  /* FIXME: Update the flag which indicates whether C_SignInit has
-   * been called.  */
   scute_global_unlock ();
   return err;
 }
index 2c54502..a529262 100644 (file)
@@ -38,7 +38,7 @@ C_SignInit (CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMechanism,
 {
   CK_RV err = CKR_OK;
   slot_iterator_t slot;
-  session_iterator_t session;
+  session_iterator_t sid;
 
   if (pMechanism == NULL_PTR || pMechanism->mechanism != CKM_RSA_PKCS)
     return CKR_ARGUMENTS_BAD;
@@ -50,13 +50,10 @@ C_SignInit (CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMechanism,
   if (err)
     return err;
 
-  err = slots_lookup_session (hSession, &slot, &session);
-  if (err)
-    goto out;
-
-  err = session_set_signing_key (slot, session, hKey);
+  err = slots_lookup_session (hSession, &slot, &sid);
+  if (!err)
+    err = session_set_signing_key (slot, sid, hKey);
 
- out:
   scute_global_unlock ();
   return err;
 }
index c97bbb8..ca6a015 100644 (file)
@@ -992,7 +992,8 @@ session_get_search_result (slot_iterator_t id, session_iterator_t sid,
 }
 
 
-/* Set the signing key for session SID in slot ID to KEY.  */
+/* Set the signing key for session SID in slot ID to KEY.  This is the
+ * core of C_SignInit.  */
 CK_RV
 session_set_signing_key (slot_iterator_t id, session_iterator_t sid,
                         object_iterator_t key)
@@ -1027,8 +1028,7 @@ session_set_signing_key (slot_iterator_t id, session_iterator_t sid,
 }
 
 
-/* FIXME: The description is wrong:
-   Set the signing key for session SID in slot ID to KEY.  */
+/* The core of C_Sign - see there for a description.  */
 CK_RV
 session_sign (slot_iterator_t id, session_iterator_t sid,
              CK_BYTE_PTR pData, CK_ULONG ulDataLen,
@@ -1036,6 +1036,7 @@ session_sign (slot_iterator_t id, session_iterator_t sid,
 {
   struct slot *slot = scute_table_data (slots, id);
   struct session *session = scute_table_data (slot->sessions, sid);
+  int rc;
   gpg_error_t err;
   CK_ATTRIBUTE_PTR attr;
   CK_ULONG attr_count;
@@ -1051,24 +1052,36 @@ session_sign (slot_iterator_t id, session_iterator_t sid,
   if (!session->signing_key)
     return CKR_OPERATION_NOT_INITIALIZED;
 
-  err = slot_get_object (id, session->signing_key, &attr, &attr_count);
-  if (err)
-    return err;
+  rc = slot_get_object (id, session->signing_key, &attr, &attr_count);
+  if (rc)
+    goto leave;
   if (attr_count == (CK_ULONG) -1)
-    return CKR_KEY_HANDLE_INVALID;
+    {
+      rc = CKR_KEY_HANDLE_INVALID;
+      goto leave;
+    }
   if (attr->ulValueLen != sizeof (key_class)
       || memcmp (attr->pValue, &key_class, sizeof (key_class)))
-    return CKR_KEY_HANDLE_INVALID;
+    {
+      rc = CKR_KEY_HANDLE_INVALID;
+      goto leave;
+    }
 
   /* Find the CKA_ID */
   for (i = 0; i < attr_count; i++)
     if (attr[i].type == CKA_ID)
       break;
   if (i == attr_count)
-    return CKR_GENERAL_ERROR;
+    {
+      rc = CKR_GENERAL_ERROR;
+      goto leave;
+    }
 
   if (attr[i].ulValueLen >= sizeof key_id - 1)
-    return CKR_GENERAL_ERROR;
+    {
+      rc = CKR_GENERAL_ERROR;
+      goto leave;
+    }
   strncpy (key_id, attr[i].pValue, attr[i].ulValueLen);
   key_id[attr[i].ulValueLen] = 0;
   DEBUG (DBG_INFO, "Found CKA_ID '%s'", key_id);
@@ -1076,16 +1089,19 @@ session_sign (slot_iterator_t id, session_iterator_t sid,
     ;
   if (*keyref)
     keyref++;  /* Point to the grip.  */
-  DEBUG (DBG_INFO, "Using keyref '%s'", keyref);
 
   sig_len = *pulSignatureLen;
   err = scute_agent_sign (keyref, pData, ulDataLen, pSignature, &sig_len);
-
   /* Take care of error codes which are not mapped by default.  */
   if (gpg_err_code (err) == GPG_ERR_INV_LENGTH)
-    return CKR_BUFFER_TOO_SMALL;
+    rc = CKR_BUFFER_TOO_SMALL;
   else if (gpg_err_code (err) == GPG_ERR_INV_ARG)
-    return CKR_ARGUMENTS_BAD;
+    rc = CKR_ARGUMENTS_BAD;
   else
-    return scute_gpg_err_to_ck (err);
+    rc = scute_gpg_err_to_ck (err);
+
+ leave:
+  if (rc != CKR_OK && rc != CKR_BUFFER_TOO_SMALL)
+    session->signing_key = 0;
+  return rc;
 }