]> The Tcpdump Group git mirrors - tcpdump/commitdiff
Clean up ESP and ISAKMP decryption.
authorGuy Harris <[email protected]>
Mon, 6 Jan 2020 02:37:52 +0000 (18:37 -0800)
committerGuy Harris <[email protected]>
Mon, 6 Jan 2020 02:37:52 +0000 (18:37 -0800)
At least as I read RFC 5996 section 3.14 and RFC 4303 section 2.4, if
the cipher has a block size of which the ciphertext's size must be a
multiple, the payload must be padded to make that happen, so the
ciphertext length must be a multiple of the block size.  Instead of
allocating a buffer, copying the ciphertext to it, and padding it to the
block size, fail if its size isn't a multiple of the block size.

(Note also that the old padding code added a block's worth of padding to
the end of a ciphertext block that *was* a multiple of the cipher block
size; this might have caused problems.)

Don't use the undocumented EVP_Cipher(); the lack of documentation means
a lack of information about whatever requirements it might impose.  Use
EVP_DecryptUpdate() instead.

Before calling it, use EVP_CIPHER_CTX_set_padding() to say "don't do
your own padding, this block is a multiple of the cipher block size".

Instead of using EVP_CipherInit() or EVP_CipherInit_ex(), use
EVP_DecryptInit() or EVP_DecryptInit_ex().  as we're always doing
decryption and never doing encryption - the extra parameter to
EVP_CipherInit() and EVP_CipherInit_ex() is always 0.

This may address GitHub issue #814.

It may also make it a bit easier to have the code use Common Crypto on
macOS (rather than requiring that OpenSSL be installed - macOS ships
with an OpenSSL shared library for binary compatibility with older
releases, but doesn't ship with the headers, because Apple wants you
using their crypto code) and use Cryptography API: Next Generation on
Windows (Vista/Server 2008 and later) (rather than requiring a Windows
build of OpenSSL).

(Hopefully this will all work with LibreSSL.)

CMakeLists.txt
cmakeconfig.h.in
config.h.in
configure
configure.ac
print-esp.c

index f69d803b1304fb52ba13fd218af7834a822984a2..c019f4787b1022c073db00d0fa6d5d3db1c435ed 100644 (file)
@@ -718,16 +718,16 @@ if(WITH_CRYPTO)
         check_function_exists(EVP_CIPHER_CTX_new HAVE_EVP_CIPHER_CTX_NEW)
 
         #
-        # 2) do we have EVP_CipherInit_ex()?
+        # 2) do we have EVP_DecryptInit_ex()?
         # If so, we use it, because we need to be able to make two
         # "initialize the cipher" calls, one with the cipher and key,
         # and one with the IV, and, as of OpenSSL 1.1, You Can't Do That
-        # with EVP_CipherInit(), because a call to EVP_CipherInit() will
+        # with EVP_DecryptInit(), because a call to EVP_DecryptInit() will
         # unconditionally clear the context, and if you don't supply a
         # cipher, it'll clear the cipher, rendering the context unusable
         # and causing a crash.
         #
-        check_function_exists(EVP_CipherInit_ex HAVE_EVP_CIPHERINIT_EX)
+        check_function_exists(EVP_DecryptInit_ex HAVE_EVP_DECRYPTINIT_EX)
 
         cmake_pop_check_state()
 
index 38925d7ba316adeb39069660ca29662cd7162cca..852d15176b8bed2f5846bc5a6e472c6b2fadd7c2 100644 (file)
 /* Define to 1 if you have the `ether_ntohost' function. */
 #cmakedefine HAVE_ETHER_NTOHOST 1
 
-/* Define to 1 if you have the `EVP_CipherInit_ex' function. */
-#cmakedefine HAVE_EVP_CIPHERINIT_EX 1
-
 /* Define to 1 if you have the `EVP_CIPHER_CTX_new' function. */
 #cmakedefine HAVE_EVP_CIPHER_CTX_NEW 1
 
+/* Define to 1 if you have the `EVP_DecryptInit_ex' function. */
+#cmakedefine HAVE_EVP_DECRYPTINIT_EX 1
+
 /* Define to 1 if you have the <fcntl.h> header file. */
 #cmakedefine HAVE_FCNTL_H 1
 
index 4b22e15ca810b12fb115df60e1f7daa7310a83a1..365651faf79f7602978e08b73699967233e76e1b 100644 (file)
 /* Define to 1 if you have the `ether_ntohost' function. */
 #undef HAVE_ETHER_NTOHOST
 
-/* Define to 1 if you have the `EVP_CipherInit_ex' function. */
-#undef HAVE_EVP_CIPHERINIT_EX
-
 /* Define to 1 if you have the `EVP_CIPHER_CTX_new' function. */
 #undef HAVE_EVP_CIPHER_CTX_NEW
 
+/* Define to 1 if you have the `EVP_DecryptInit_ex' function. */
+#undef HAVE_EVP_DECRYPTINIT_EX
+
 /* Define to 1 if you have the <fcntl.h> header file. */
 #undef HAVE_FCNTL_H
 
index 3b2041c9a2016cb870f476e9e3b432bd146c08f6..1943642ab22f1b785ab7b05f3d571b6ffd0c5056 100755 (executable)
--- a/configure
+++ b/configure
@@ -7676,19 +7676,19 @@ done
                        # EVP_CIPHER_CTX, as EVP_CIPHER_CTX may be
                        # opaque; otherwise, we allocate it ourselves.
                        #
-                       # 2) do we have EVP_CipherInit_ex()?
+                       # 2) do we have EVP_DecryptInit_ex()?
                        # If so, we use it, because we need to be
                        # able to make two "initialize the cipher"
                        # calls, one with the cipher and key, and
                        # one with the IV, and, as of OpenSSL 1.1,
-                       # You Can't Do That with EVP_CipherInit(),
-                       # because a call to EVP_CipherInit() will
+                       # You Can't Do That with EVP_DecryptInit(),
+                       # because a call to EVP_DecryptInit() will
                        # unconditionally clear the context, and
                        # if you don't supply a cipher, it'll
                        # clear the cipher, rendering the context
                        # unusable and causing a crash.
                        #
-                       for ac_func in EVP_CIPHER_CTX_new EVP_CipherInit_ex
+                       for ac_func in EVP_CIPHER_CTX_new EVP_DecryptInit_ex
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
index 26c1b3a8c0ccd5f644265fe2933c93de24a86753..d81ff2b064e11eeb8ae48d7483bc444bcda94b72 100644 (file)
@@ -924,19 +924,19 @@ if test "$want_libcrypto" != "no"; then
                        # EVP_CIPHER_CTX, as EVP_CIPHER_CTX may be
                        # opaque; otherwise, we allocate it ourselves.
                        #
-                       # 2) do we have EVP_CipherInit_ex()?
+                       # 2) do we have EVP_DecryptInit_ex()?
                        # If so, we use it, because we need to be
                        # able to make two "initialize the cipher"
                        # calls, one with the cipher and key, and
                        # one with the IV, and, as of OpenSSL 1.1,
-                       # You Can't Do That with EVP_CipherInit(),
-                       # because a call to EVP_CipherInit() will
+                       # You Can't Do That with EVP_DecryptInit(),
+                       # because a call to EVP_DecryptInit() will
                        # unconditionally clear the context, and
                        # if you don't supply a cipher, it'll
                        # clear the cipher, rendering the context
                        # unusable and causing a crash.
                        #
-                       AC_CHECK_FUNCS(EVP_CIPHER_CTX_new EVP_CipherInit_ex)
+                       AC_CHECK_FUNCS(EVP_CIPHER_CTX_new EVP_DecryptInit_ex)
                fi
        ])
 fi
index 135426b8c9544804055d4ed5b3de549eb890942d..ae8cb038937b145ed8325394398b9a5ec916afaa 100644 (file)
@@ -147,12 +147,12 @@ EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx)
 
 #ifdef HAVE_EVP_CIPHERINIT_EX
 /*
- * Initialize the cipher by calling EVP_CipherInit_ex(), because
- * calling EVP_CipherInit() will reset the cipher context, clearing
+ * Initialize the cipher by calling EVP_DecryptInit_ex(), because
+ * calling EVP_DecryptInit() will reset the cipher context, clearing
  * the cipher, so calling it twice, with the second call having a
- * null cipher, will clear the already-set cipher.  EVP_CipherInit_ex(),
+ * null cipher, will clear the already-set cipher.  EVP_DecryptInit_ex(),
  * however, won't reset the cipher context, so you can use it to specify
- * the IV in a second call after a first call to EVP_CipherInit_ex()
+ * the IV in a second call after a first call to EVP_DecryptInit_ex()
  * to set the cipher and the key.
  *
  * XXX - is there some reason why we need to make two calls?
@@ -160,21 +160,21 @@ EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx)
 static int
 set_cipher_parameters(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher,
                      const unsigned char *key,
-                     const unsigned char *iv, int enc)
+                     const unsigned char *iv)
 {
-       return EVP_CipherInit_ex(ctx, cipher, NULL, key, iv, enc);
+       return EVP_DecryptInit_ex(ctx, cipher, NULL, key, iv);
 }
 #else
 /*
- * Initialize the cipher by calling EVP_CipherInit(), because we don't
- * have EVP_CipherInit_ex(); we rely on it not trashing the context.
+ * Initialize the cipher by calling EVP_DecryptInit(), because we don't
+ * have EVP_DecryptInit_ex(); we rely on it not trashing the context.
  */
 static int
 set_cipher_parameters(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher,
                      const unsigned char *key,
-                     const unsigned char *iv, int enc)
+                     const unsigned char *iv)
 {
-       return EVP_CipherInit(ctx, cipher, key, iv, enc);
+       return EVP_DecryptInit(ctx, cipher, key, iv);
 }
 #endif
 
@@ -200,11 +200,12 @@ int esp_print_decrypt_buffer_by_ikev2(netdissect_options *ndo,
        struct sa_list *sa;
        const u_char *iv;
        const u_char *ct;
-       unsigned int len;
+       unsigned int ctlen;
+       int len;
        EVP_CIPHER_CTX *ctx;
-       unsigned int block_size, buffer_size;
-       u_char *input_buffer, *output_buffer;
-       const u_char *pt;
+       unsigned int block_size;
+       u_char *pt;
+       u_int ptlen;
 
        /* initiator arg is any non-zero value */
        if(initiator) initiator=1;
@@ -228,79 +229,76 @@ int esp_print_decrypt_buffer_by_ikev2(netdissect_options *ndo,
        end = end - sa->authlen;
        iv  = buf;
        ct = iv + sa->ivlen;
-       len = end-ct;
+       ctlen = end-ct;
 
        if(end <= ct) return 0;
 
        ctx = EVP_CIPHER_CTX_new();
        if (ctx == NULL)
                return 0;
-       if (set_cipher_parameters(ctx, sa->evp, sa->secret, NULL, 0) < 0) {
+       if (set_cipher_parameters(ctx, sa->evp, sa->secret, NULL) < 0) {
+               EVP_CIPHER_CTX_free(ctx);
                (*ndo->ndo_warning)(ndo, "espkey init failed");
                return 0;
        }
-       if (set_cipher_parameters(ctx, NULL, NULL, iv, 0) < 0) {
+       if (set_cipher_parameters(ctx, NULL, NULL, iv) < 0) {
+               EVP_CIPHER_CTX_free(ctx);
                (*ndo->ndo_warning)(ndo, "IV init failed");
                return 0;
        }
+
        /*
-        * Allocate buffers for the encrypted and decrypted data.
-        * Both buffers' sizes must be a multiple of the cipher block
-        * size, and the output buffer must be separate from the input
-        * buffer.
+        * At least as I read RFC 5996 section 3.14, if the cipher
+        * has a block size of which the ciphertext's size must
+        * be a multiple, the payload must be padded to make that
+        * happen, so the ciphertext length must be a multiple of
+        * the block size.  Fail if that's not the case.
         */
        block_size = (unsigned int)EVP_CIPHER_CTX_block_size(ctx);
-       buffer_size = len + (block_size - len % block_size);
+       if ((ctlen % block_size) != 0) {
+               EVP_CIPHER_CTX_free(ctx);
+               (*ndo->ndo_warning)(ndo, "ciphertext size %u is not a multiple of the cipher block size %u",
+                   ctlen, block_size);
+               return 0;
+       }
 
        /*
-        * Attempt to allocate the input buffer.
+        * Attempt to allocate a buffer for the decrypted data, because
+        * we can't decrypt on top of the input buffer.
         */
-       input_buffer = (u_char *)malloc(buffer_size);
-       if (input_buffer == NULL) {
+       ptlen = ctlen;
+       pt = (u_char *)malloc(ptlen);
+       if (pt == NULL) {
                EVP_CIPHER_CTX_free(ctx);
                (*ndo->ndo_error)(ndo, S_ERR_ND_MEM_ALLOC,
-                       "can't allocate memory for encrypted data buffer");
+                       "can't allocate memory for decryption buffer");
        }
-       /*
-        * Copy the input data to the encrypted data buffer, and pad it
-        * with zeroes.
-        */
-       memcpy(input_buffer, ct, len);
-       memset(input_buffer + len, 0, buffer_size - len);
 
        /*
-        * Attempt to allocate the output buffer.
+        * The size of the ciphertext handed to us is a multiple of the
+        * cipher block size, so we don't need to worry about padding.
         */
-       output_buffer = (u_char *)malloc(buffer_size);
-       if (output_buffer == NULL) {
-               free(input_buffer);
+       if (!EVP_CIPHER_CTX_set_padding(ctx, 0)) {
+               free(pt);
                EVP_CIPHER_CTX_free(ctx);
-               (*ndo->ndo_error)(ndo, S_ERR_ND_MEM_ALLOC,
-                       "can't allocate memory for decryption buffer");
+               (*ndo->ndo_warning)(ndo, "EVP_CIPHER_CTX_set_padding failed");
+               return 0;
        }
-       if (!EVP_Cipher(ctx, output_buffer, input_buffer, len)) {
-               (*ndo->ndo_warning)(ndo, "EVP_Cipher failed");
+       if (!EVP_DecryptUpdate(ctx, pt, &len, ct, ctlen)) {
+               free(pt);
+               EVP_CIPHER_CTX_free(ctx);
+               (*ndo->ndo_warning)(ndo, "EVP_DecryptUpdate failed");
                return 0;
        }
        EVP_CIPHER_CTX_free(ctx);
 
-       /*
-        * Free the input buffer; we no longer need it.
-        */
-       free(input_buffer);
-
-       /*
-        * Get a pointer to the plaintext.
-        */
-       pt = output_buffer;
-
        /*
         * Switch to the output buffer for dissection, and save it
         * on the buffer stack so it can be freed; our caller must
         * pop it when done.
         */
-       if (!nd_push_buffer(ndo, output_buffer, pt, pt + len)) {
-               free(output_buffer);
+       if (!nd_push_buffer(ndo, pt, pt, pt + ctlen)) {
+               free(pt);
                return 0;
        }
 
@@ -712,10 +710,10 @@ esp_print(netdissect_options *ndo,
        u_int ivlen;
        const u_char *ct;
        u_int ctlen;
+       int len;
        EVP_CIPHER_CTX *ctx;
-       unsigned int block_size, buffer_size;
-       u_char *input_buffer, *output_buffer;
-       const u_char *pt;
+       unsigned int block_size;
+       u_char *pt;
        u_int ptlen;
        u_int padlen;
        u_int nh;
@@ -857,81 +855,73 @@ esp_print(netdissect_options *ndo,
                        "esp_print: can't allocate memory for cipher context");
        }
 
-       if (set_cipher_parameters(ctx, sa->evp, sa->secret, NULL, 0) < 0) {
+       if (set_cipher_parameters(ctx, sa->evp, sa->secret, NULL) < 0) {
+               EVP_CIPHER_CTX_free(ctx);
                (*ndo->ndo_warning)(ndo, "espkey init failed");
                return;
        }
 
-       if (set_cipher_parameters(ctx, NULL, NULL, iv, 0) < 0) {
+       if (set_cipher_parameters(ctx, NULL, NULL, iv) < 0) {
+               EVP_CIPHER_CTX_free(ctx);
                (*ndo->ndo_warning)(ndo, "IV init failed");
                return;
        }
 
        /*
-        * Allocate buffers for the encrypted and decrypted
-        * data.  Both buffers' sizes must be a multiple of
-        * the cipher block size, and the output buffer must
-        * be separate from the input buffer.
+        * At least as I read RFC 4303 section 2.4, if the cipher
+        * has a block size of which the ciphertext's size must
+        * be a multiple, the payload must be padded to make that
+        * happen, so the ciphertext length must be a multiple of
+        * the block size.  Fail if that's not the case.
         */
        block_size = (unsigned int)EVP_CIPHER_CTX_block_size(ctx);
-       buffer_size = ctlen + (block_size - ctlen % block_size);
+       if ((ctlen % block_size) != 0) {
+               EVP_CIPHER_CTX_free(ctx);
+               (*ndo->ndo_warning)(ndo, "ciphertext size %u is not a multiple of the cipher block size %u",
+                   ctlen, block_size);
+               return;
+       }
 
        /*
-        * Attempt to allocate the input buffer.
+        * Attempt to allocate a buffer for the decrypted data, because
+        * we can't decrypt on top of the input buffer.
         */
-       input_buffer = (u_char *)malloc(buffer_size);
-       if (input_buffer == NULL) {
+       ptlen = ctlen;
+       pt = (u_char *)malloc(ptlen);
+       if (pt == NULL) {
                EVP_CIPHER_CTX_free(ctx);
                (*ndo->ndo_error)(ndo, S_ERR_ND_MEM_ALLOC,
-                       "esp_print: can't allocate memory for encrypted data buffer");
+                       "esp_print: can't allocate memory for decryption buffer");
        }
-       /*
-        * Copy the input data to the encrypted data buffer,
-        * and pad it with zeroes.
-        */
-       memcpy(input_buffer, ct, ctlen);
-       memset(input_buffer + ctlen, 0, buffer_size - ctlen);
 
        /*
-        * Attempt to allocate the output buffer.
+        * The size of the ciphertext handed to us is a multiple of the
+        * cipher block size, so we don't need to worry about padding.
         */
-       output_buffer = (u_char *)malloc(buffer_size);
-       if (output_buffer == NULL) {
-               free(input_buffer);
+       if (!EVP_CIPHER_CTX_set_padding(ctx, 0)) {
+               free(pt);
                EVP_CIPHER_CTX_free(ctx);
-               (*ndo->ndo_error)(ndo, S_ERR_ND_MEM_ALLOC,
-                       "esp_print: can't allocate memory for decryption buffer");
+               (*ndo->ndo_warning)(ndo, "EVP_CIPHER_CTX_set_padding failed");
+               return;
        }
-
-       if (!EVP_Cipher(ctx, output_buffer, input_buffer, ctlen)) {
-               free(input_buffer);
-               (*ndo->ndo_warning)(ndo, "EVP_Cipher failed");
+       if (!EVP_DecryptUpdate(ctx, pt, &len, ct, ctlen)) {
+               free(pt);
+               EVP_CIPHER_CTX_free(ctx);
+               (*ndo->ndo_warning)(ndo, "EVP_DecryptUpdate failed");
                return;
        }
-       free(input_buffer);
        EVP_CIPHER_CTX_free(ctx);
 
-       /*
-        * Pointer to the plaintext.
-        */
-       pt = output_buffer;
-
-       /*
-        * Length of the plaintext, which is the same as the length
-        * of the ciphertext.
-        */
-       ptlen = ctlen;
-
        /*
         * Switch to the output buffer for dissection, and
         * save it on the buffer stack so it can be freed.
         */
-       if (!nd_push_buffer(ndo, output_buffer, pt, pt + ctlen)) {
-               free(output_buffer);
+       ep = pt + ptlen;
+       if (!nd_push_buffer(ndo, pt, pt, ep)) {
+               free(pt);
                (*ndo->ndo_error)(ndo, S_ERR_ND_MEM_ALLOC,
                        "esp_print: can't push buffer on buffer stack");
        }
-       ep = pt + ptlen;
 
        /*
         * Sanity check for pad length; if it, plus 2 for the pad