Bug 1528174 - Don't modify lengths if decryption/encryption fails, r=jcj

Summary:
This modifies the encrypt/decrypt paths to only modify their outparams
when the operation succeeds.  It adds tests to verify this.

Reviewers: jcj

Reviewed By: jcj

Bug #: 1528174

Differential Revision: https://phabricator.services.mozilla.com/D20962

4 files changed, 51 insertions, 37 deletions

diff --git a/lib/freebl/chacha20poly1305.c b/lib/freebl/chacha20poly1305.c
index b97e845f4..9cf92f81d 100644
--- a/lib/freebl/chacha20poly1305.c
+++ b/lib/freebl/chacha20poly1305.c
@@ -210,8 +210,7 @@ ChaCha20Poly1305_Seal(const ChaCha20Poly1305Context *ctx, unsigned char *output,
         PORT_SetError(SEC_ERROR_INPUT_LEN);
         return SECFailure;
     }
-    *outputLen = inputLen + ctx->tagLen;
-    if (maxOutputLen < *outputLen) {
+    if (maxOutputLen < inputLen + ctx->tagLen) {
         PORT_SetError(SEC_ERROR_OUTPUT_LEN);
         return SECFailure;
     }
@@ -227,6 +226,7 @@ ChaCha20Poly1305_Seal(const ChaCha20Poly1305Context *ctx, unsigned char *output,
     Poly1305Do(tag, ad, adLen, output, inputLen, block);
     PORT_Memcpy(output + inputLen, tag, ctx->tagLen);
 
+    *outputLen = inputLen + ctx->tagLen;
     return SECSuccess;
 #endif
 }
@@ -254,8 +254,7 @@ ChaCha20Poly1305_Open(const ChaCha20Poly1305Context *ctx, unsigned char *output,
         return SECFailure;
     }
     ciphertextLen = inputLen - ctx->tagLen;
-    *outputLen = ciphertextLen;
-    if (maxOutputLen < *outputLen) {
+    if (maxOutputLen < ciphertextLen) {
         PORT_SetError(SEC_ERROR_OUTPUT_LEN);
         return SECFailure;
     }
@@ -274,6 +273,7 @@ ChaCha20Poly1305_Open(const ChaCha20Poly1305Context *ctx, unsigned char *output,
     ChaCha20Xor(output, (uint8_t *)input, ciphertextLen, (uint8_t *)ctx->key,
                 (uint8_t *)nonce, 1);
 
+    *outputLen = ciphertextLen;
     return SECSuccess;
 #endif
 }
diff --git a/lib/pk11wrap/pk11cxt.c b/lib/pk11wrap/pk11cxt.c
index e9726d03c..61faeaafc 100644
--- a/lib/pk11wrap/pk11cxt.c
+++ b/lib/pk11wrap/pk11cxt.c
@@ -1008,12 +1008,12 @@ PK11_DigestFinal(PK11Context *context, unsigned char *data,
     }
     PK11_ExitContextMonitor(context);
 
-    *outLen = (unsigned int)len;
     context->init = PR_FALSE; /* allow Begin to start up again */
 
     if (crv != CKR_OK) {
         PORT_SetError(PK11_MapError(crv));
         return SECFailure;
     }
+    *outLen = (unsigned int)len;
     return SECSuccess;
 }
diff --git a/lib/pk11wrap/pk11obj.c b/lib/pk11wrap/pk11obj.c
index 937ac654a..468f94ebc 100644
--- a/lib/pk11wrap/pk11obj.c
+++ b/lib/pk11wrap/pk11obj.c
@@ -933,11 +933,11 @@ PK11_Decrypt(PK11SymKey *symKey,
     if (haslock)
         PK11_ExitSlotMonitor(slot);
     pk11_CloseSession(slot, session, owner);
-    *outLen = len;
     if (crv != CKR_OK) {
         PORT_SetError(PK11_MapError(crv));
         return SECFailure;
     }
+    *outLen = len;
     return SECSuccess;
 }
 
@@ -979,11 +979,11 @@ PK11_Encrypt(PK11SymKey *symKey,
     if (haslock)
         PK11_ExitSlotMonitor(slot);
     pk11_CloseSession(slot, session, owner);
-    *outLen = len;
     if (crv != CKR_OK) {
         PORT_SetError(PK11_MapError(crv));
         return SECFailure;
     }
+    *outLen = len;
     return SECSuccess;
 }
 
@@ -1665,10 +1665,10 @@ pk11_CreateGenericObjectHelper(PK11SlotInfo *slot,
 /* This is the classic interface. Applications would call this function to
  * create new object that would not be destroyed later. This lead to resource
  * leaks (and thus memory leaks in the PKCS #11 module).  To solve this we have
- * a new interface that automatically marks objects created on the fly to be 
- * destroyed later. 
+ * a new interface that automatically marks objects created on the fly to be
+ * destroyed later.
  * The old interface is preserved because applications like Mozilla purposefully
- * leak the reference to be found later with PK11_FindGenericObjects. New 
+ * leak the reference to be found later with PK11_FindGenericObjects. New
  * applications should use the new interface PK11_CreateManagedGenericObject */
 PK11GenericObject *
 PK11_CreateGenericObject(PK11SlotInfo *slot, const CK_ATTRIBUTE *pTemplate,
@@ -1678,8 +1678,8 @@ PK11_CreateGenericObject(PK11SlotInfo *slot, const CK_ATTRIBUTE *pTemplate,
                                           PR_FALSE);
 }
 
-/* Use this interface. It will automatically destroy any temporary objects 
- * (token = PR_FALSE) when the PK11GenericObject is freed. Permanent objects still 
+/* Use this interface. It will automatically destroy any temporary objects
+ * (token = PR_FALSE) when the PK11GenericObject is freed. Permanent objects still
  * need to be destroyed by hand with PK11_DestroyTokenObject.
  */
 PK11GenericObject *
diff --git a/lib/softoken/pkcs11c.c b/lib/softoken/pkcs11c.c
index 003e2bec5..5830a8857 100644
--- a/lib/softoken/pkcs11c.c
+++ b/lib/softoken/pkcs11c.c
@@ -1379,8 +1379,11 @@ NSC_EncryptUpdate(CK_SESSION_HANDLE hSession,
     /* do it: NOTE: this assumes buf size in is >= buf size out! */
     rv = (*context->update)(context->cipherInfo, pEncryptedPart,
                             &outlen, maxout, pPart, ulPartLen);
+    if (rv != SECSuccess) {
+        return sftk_MapCryptError(PORT_GetError());
+    }
     *pulEncryptedPartLen = (CK_ULONG)(outlen + padoutlen);
-    return (rv == SECSuccess) ? CKR_OK : sftk_MapCryptError(PORT_GetError());
+    return CKR_OK;
 }
 
 /* NSC_EncryptFinal finishes a multiple-part encryption operation. */
@@ -1460,26 +1463,29 @@ NSC_Encrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData,
         return crv;
 
     if (!pEncryptedData) {
-        *pulEncryptedDataLen = context->rsa ? context->maxLen : ulDataLen + 2 * context->blockSize;
-        goto finish;
+        outlen = context->rsa ? context->maxLen : ulDataLen + 2 * context->blockSize;
+        goto done;
     }
 
     if (context->doPad) {
         if (context->multi) {
+            CK_ULONG updateLen = maxoutlen;
             CK_ULONG finalLen;
             /* padding is fairly complicated, have the update and final
              * code deal with it */
             sftk_FreeSession(session);
             crv = NSC_EncryptUpdate(hSession, pData, ulDataLen, pEncryptedData,
-                                    pulEncryptedDataLen);
-            if (crv != CKR_OK)
-                *pulEncryptedDataLen = 0;
-            maxoutlen -= *pulEncryptedDataLen;
-            pEncryptedData += *pulEncryptedDataLen;
+                                    &updateLen);
+            if (crv != CKR_OK) {
+                updateLen = 0;
+            }
+            maxoutlen -= updateLen;
+            pEncryptedData += updateLen;
             finalLen = maxoutlen;
             crv2 = NSC_EncryptFinal(hSession, pEncryptedData, &finalLen);
-            if (crv2 == CKR_OK)
-                *pulEncryptedDataLen += finalLen;
+            if (crv == CKR_OK && crv2 == CKR_OK) {
+                *pulEncryptedDataLen = updateLen + finalLen;
+            }
             return crv == CKR_OK ? crv2 : crv;
         }
         /* doPad without multi means that padding must be done on the first
@@ -1505,14 +1511,15 @@ NSC_Encrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData,
     rv = (*context->update)(context->cipherInfo, pEncryptedData,
                             &outlen, maxoutlen, pText.data, pText.len);
     crv = (rv == SECSuccess) ? CKR_OK : sftk_MapCryptError(PORT_GetError());
-    *pulEncryptedDataLen = (CK_ULONG)outlen;
     if (pText.data != pData)
         PORT_ZFree(pText.data, pText.len);
 fail:
     sftk_TerminateOp(session, SFTK_ENCRYPT, context);
-finish:
+done:
     sftk_FreeSession(session);
-
+    if (crv == CKR_OK) {
+        *pulEncryptedDataLen = (CK_ULONG)outlen;
+    }
     return crv;
 }
 
@@ -1601,8 +1608,11 @@ NSC_DecryptUpdate(CK_SESSION_HANDLE hSession,
     /* do it: NOTE: this assumes buf size in is >= buf size out! */
     rv = (*context->update)(context->cipherInfo, pPart, &outlen,
                             maxout, pEncryptedPart, ulEncryptedPartLen);
+    if (rv != SECSuccess) {
+        return sftk_MapDecryptError(PORT_GetError());
+    }
     *pulPartLen = (CK_ULONG)(outlen + padoutlen);
-    return (rv == SECSuccess) ? CKR_OK : sftk_MapDecryptError(PORT_GetError());
+    return CKR_OK;
 }
 
 /* NSC_DecryptFinal finishes a multiple-part decryption operation. */
@@ -1693,25 +1703,27 @@ NSC_Decrypt(CK_SESSION_HANDLE hSession,
         return crv;
 
     if (!pData) {
-        *pulDataLen = ulEncryptedDataLen + context->blockSize;
-        goto finish;
+        outlen = ulEncryptedDataLen + context->blockSize;
+        goto done;
     }
 
     if (context->doPad && context->multi) {
+        CK_ULONG updateLen = maxoutlen;
         CK_ULONG finalLen;
         /* padding is fairly complicated, have the update and final
          * code deal with it */
         sftk_FreeSession(session);
         crv = NSC_DecryptUpdate(hSession, pEncryptedData, ulEncryptedDataLen,
-                                pData, pulDataLen);
-        if (crv != CKR_OK)
-            *pulDataLen = 0;
-        maxoutlen -= *pulDataLen;
-        pData += *pulDataLen;
+                                pData, &updateLen);
+        if (crv == CKR_OK) {
+            maxoutlen -= updateLen;
+            pData += updateLen;
+        }
         finalLen = maxoutlen;
         crv2 = NSC_DecryptFinal(hSession, pData, &finalLen);
-        if (crv2 == CKR_OK)
-            *pulDataLen += finalLen;
+        if (crv == CKR_OK && crv2 == CKR_OK) {
+            *pulDataLen = updateLen + finalLen;
+        }
         return crv == CKR_OK ? crv2 : crv;
     }
 
@@ -1736,10 +1748,12 @@ NSC_Decrypt(CK_SESSION_HANDLE hSession,
             }
         }
     }
-    *pulDataLen = (CK_ULONG)outlen;
     sftk_TerminateOp(session, SFTK_DECRYPT, context);
-finish:
+done:
     sftk_FreeSession(session);
+    if (crv == CKR_OK) {
+        *pulDataLen = (CK_ULONG)outlen;
+    }
     return crv;
 }