#246 Change the way we handle encrypted buffers
Merged 4 years ago by rharwood. Opened 4 years ago by simo.
simo/gssproxy fixencrypt  into  master

file modified
+86 -24
@@ -193,9 +193,15 @@ 

      return ret_maj;

  }

  

- /* We need to include a length in our payloads because krb5_c_decrypt() will

-  * pad the contents for some enctypes, and gss_import_cred() doesn't like

-  * having extra bytes on tokens. */

+ #define ENC_MIN_PAD_LEN 8

+ 

+ /* We need to pad our payloads because krb5_c_decrypt() may pad the

+  * contents for some enctypes, and gss_import_cred() doesn't like

+  * having extra bytes on tokens.

+  * Explicit padding and depadding is used in order to maintain backwards

+  * compatibility over upgrades (and downgrades), it would have been

+  * better if we simply had a better formatting of the returned blob

+  * so we could simply change a "blob version" number */

  static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,

                               size_t len, void *buf, octet_string *out)

  {
@@ -203,8 +209,9 @@ 

      krb5_data data_in;

      krb5_enc_data enc_handle;

      size_t cipherlen;

-     char *packed = NULL;

-     uint32_t netlen;

+     size_t padcheck;

+     uint8_t pad = 0;

+     char *padded = NULL;

  

      if (len > (uint32_t)(-1)) {

          /* Needs to fit in 4 bytes of payload, so... */
@@ -212,28 +219,72 @@ 

          goto done;

      }

  

-     packed = malloc(len);

-     if (!packed) {

-         ret = errno;

+     ret = krb5_c_encrypt_length(context,

+                                 key->enctype,

+                                 len, &cipherlen);

+     if (ret) {

          goto done;

      }

  

-     netlen = htonl(len);

-     memcpy(packed, (uint8_t *)&netlen, 4);

-     memcpy(packed + 4, buf, len);

- 

-     data_in.length = len + 4;

-     data_in.data = packed;

- 

-     memset(&enc_handle, '\0', sizeof(krb5_enc_data));

- 

+     /* try again with len + 1 to see if padding is required */

      ret = krb5_c_encrypt_length(context,

                                  key->enctype,

-                                 data_in.length,

-                                 &cipherlen);

+                                 len + 1, &padcheck);

      if (ret) {

          goto done;

      }

+     if (padcheck == cipherlen) {

+         int i;

+         /* padding required */

+         pad = ENC_MIN_PAD_LEN;

+         /* always add enough padding that it makes it extremely unlikley

+          * legitimate plaintext will be incorrectly depadded in the

+          * decrypt function */

+         ret = krb5_c_encrypt_length(context,

+                                     key->enctype,

+                                     len + pad, &cipherlen);

+         if (ret) {

+             goto done;

+         }

+         /* we support only block sizes up to 16 bytes as this is the largest

+          * supported block size in krb ciphers for now */

+         for (i = 0; i < 15; i++) {

+             /* find the point at which padcheck increases, that's when we

+              * cross a blocksize boundary internally and we can calculate

+              * the padding that will be used */

+             ret = krb5_c_encrypt_length(context,

+                                         key->enctype,

+                                         len + pad + i + 1, &padcheck);

+             if (ret) {

+                 goto done;

+             }

+             if (padcheck > cipherlen) {

+                 pad += i;

+                 break;

+             }

+         }

+         if (i > 15) {

+             ret = EINVAL;

+             goto done;

+         }

+     }

+ 

+     if (pad != 0) {

+         padded = malloc(len + pad);

+         if (!padded) {

+             ret = errno;

+             goto done;

+         }

+ 

+         memcpy(padded, buf, len);

+         memset(padded + len, pad, pad);

+ 

+         data_in.length = len + pad;

+         data_in.data = padded;

+     } else {

+         data_in.length = len;

+         data_in.data = buf;

+     }

  

      enc_handle.ciphertext.length = cipherlen;

      enc_handle.ciphertext.data = malloc(enc_handle.ciphertext.length);
@@ -261,7 +312,7 @@ 

      }

  

  done:

-     free(packed);

+     free(padded);

      free(enc_handle.ciphertext.data);

      return ret;

  }
@@ -273,7 +324,8 @@ 

      int ret;

      krb5_data data_out;

      krb5_enc_data enc_handle;

-     uint32_t netlen;

+     uint8_t pad;

+     int i, j;

  

      memset(&enc_handle, '\0', sizeof(krb5_enc_data));

  
@@ -295,9 +347,19 @@ 

      }

  

      /* And handle the padding. */

-     memcpy(&netlen, buf, 4);

-     *len = ntohl(netlen);

-     memmove(buf, buf + 4, *len);

+     i = data_out.length - 1;

+     pad = data_out.data[i];

+     if (pad >= ENC_MIN_PAD_LEN && pad < i) {

+         j = pad;

+         while (j > 0) {

+             j--;

+             if (pad != data_out.data[i - j]) break;

+         }

+         if (j == 0) {

+             data_out.length -= pad;

+         }

+     }

+     *len = data_out.length;

  

      return 0;

  }

The previous change has backwards incompatible behavior that may also
lead to buffer overruns.

Because we have no easy way to indicate a format change and to maintain
backwards compatibility for the ciphers that were working (those that
added padding were hopelessly borken anyway) introduce code to simply
add padding that we can recognize and remove when we read back the token.

On ciphers that do not add padding this is basically a no op and the
tokens will be identical to the ones we previously emitted.

On ciphers that add padding we pad the plaintext so that we hit a block
boundary and cause no extra padding to be added by krb5_c_encrypt
itself. On decryption we check if padding bytes are appended to the
buffer and remove them.

Signed-off-by: Simo Sorce simo@redhat.com

FYI.

Hi, I have tests which are failed on the bare 0.8.1 on i586 machine ( x64 ones are OK).
Using this PR both are OK.

If you need more info, please let me know.
Thank you.

Commit 839be8a fixes this pull-request

Pull-Request has been merged by rharwood

4 years ago