From d5e68e9d1776eb0d1d3deaae461a376a42bda3a5 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Apr 16 2019 19:12:03 +0000
Subject: Handle gss_import_cred() failure when importing gssx creds
Otherwise, we might attempt to set options on a non-existent handle,
leading to a segfault.
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
diff --git a/src/gp_export.c b/src/gp_export.c
index 403e339..7ad8037 100644
--- a/src/gp_export.c
+++ b/src/gp_export.c
@@ -476,6 +476,10 @@ uint32_t gp_import_gssx_cred(uint32_t *min, struct gp_call_ctx *gpcall,
}
ret_maj = gss_import_cred(&ret_min, &token, out);
+ if (ret_maj) {
+ GPDEBUG("gss_import_cred failed when importing gssx cred\n");
+ goto done;
+ }
/* check if there is any client option we need to set on credentials */
gp_set_cred_options(cred, *out);
From 2555d76fe3e5ce7b3a04f137054da0ba1f1a9374 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Apr 16 2019 19:11:59 +0000
Subject: Always initialize out cred in gp_import_gssx_cred()
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
diff --git a/src/gp_export.c b/src/gp_export.c
index 5e8e160..403e339 100644
--- a/src/gp_export.c
+++ b/src/gp_export.c
@@ -449,6 +449,8 @@ uint32_t gp_import_gssx_cred(uint32_t *min, struct gp_call_ctx *gpcall,
uint32_t ret_min = 0;
int ret;
+ *out = GSS_C_NO_CREDENTIAL;
+
handle = gp_service_get_creds_handle(gpcall->service);
if (!handle) {
ret_maj = GSS_S_FAILURE;
@@ -470,7 +472,6 @@ uint32_t gp_import_gssx_cred(uint32_t *min, struct gp_call_ctx *gpcall,
if (ret) {
/* Allow for re-issuance of the keytab. */
GPDEBUG("Stored ccache failed to decrypt; treating as empty\n");
- *out = GSS_C_NO_CREDENTIAL;
goto done;
}
From 25ff6504f18b12ee1a65215808c661518802b832 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Apr 16 2019 20:52:13 +0000
Subject: Include length when using krb5_c_decrypt()
For some enctypes, krb5_c_decrypt() will add padding bytes which are
included in the returned length. However, functions which use the
objects we're storing aren't always prepared for that: in particular,
gss_import_cred() will declare a token invalid if there's trailing
garbage.
Work around this by including 4 bytes of length on encrypted objects.
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
diff --git a/src/gp_export.c b/src/gp_export.c
index 7ad8037..aa0a8ec 100644
--- a/src/gp_export.c
+++ b/src/gp_export.c
@@ -193,6 +193,9 @@ done:
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. */
static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,
size_t len, void *buf, octet_string *out)
{
@@ -200,9 +203,27 @@ static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,
krb5_data data_in;
krb5_enc_data enc_handle;
size_t cipherlen;
+ char *packed = NULL;
+ uint32_t netlen;
- data_in.length = len;
- data_in.data = buf;
+ if (len > (uint32_t)(-1)) {
+ /* Needs to fit in 4 bytes of payload, so... */
+ ret = ENOMEM;
+ goto done;
+ }
+
+ packed = malloc(len);
+ if (!packed) {
+ ret = errno;
+ 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));
@@ -240,16 +261,19 @@ static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,
}
done:
+ free(packed);
free(enc_handle.ciphertext.data);
return ret;
}
+/* See comment above on gp_encrypt_buffer(). */
static int gp_decrypt_buffer(krb5_context context, krb5_keyblock *key,
- octet_string *in, size_t *len, void *buf)
+ octet_string *in, size_t *len, char *buf)
{
int ret;
krb5_data data_out;
krb5_enc_data enc_handle;
+ uint32_t netlen;
memset(&enc_handle, '\0', sizeof(krb5_enc_data));
@@ -270,7 +294,10 @@ static int gp_decrypt_buffer(krb5_context context, krb5_keyblock *key,
return ret;
}
- *len = data_out.length;
+ /* And handle the padding. */
+ memcpy(&netlen, buf, 4);
+ *len = ntohl(netlen);
+ memmove(buf, buf + 4, *len);
return 0;
}
From 839be8aa7e54e93819e8291b570e4c7cfe7e98f1 Mon Sep 17 00:00:00 2001
From: Simo Sorce <simo@redhat.com>
Date: Apr 18 2019 15:23:49 +0000
Subject: Change the way we handle encrypted buffers
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>
Reviewed-by: Robbie Harwood <rharwood@redhat.com>
Merges: #246
---
diff --git a/src/gp_export.c b/src/gp_export.c
index aa0a8ec..dbfddeb 100644
--- a/src/gp_export.c
+++ b/src/gp_export.c
@@ -193,9 +193,15 @@ done:
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 @@ static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,
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 @@ static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,
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 @@ static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key,
}
done:
- free(packed);
+ free(padded);
free(enc_handle.ciphertext.data);
return ret;
}
@@ -273,7 +324,8 @@ static int gp_decrypt_buffer(krb5_context context, krb5_keyblock *key,
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 @@ static int gp_decrypt_buffer(krb5_context context, krb5_keyblock *key,
}
/* 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;
}