CMAC + SERPENT/IDEA/RC2 buffer overflow/crash with oversized key

[GnuPG - Libgcrypt - Dev mailing list]

In the program below, each of three calls to cmac() causes a different crash (use AddressSanitizer to be sure). I think the correct approach is to make gcry_mac_setkey() return an error code if the key has an inappropriate size.

#include <gcrypt.h>

#define CF_CHECK_EQ(expr, res) if ( (expr) != (res) ) { goto end; }

static void cmac(const int mac, const int keysize) {
    unsigned char key[keysize];
    memset(key, 0, keysize);

    gcry_mac_hd_t h;
    CF_CHECK_EQ(gcry_mac_open(&h, mac, 0, NULL), GPG_ERR_NO_ERROR);
    CF_CHECK_EQ(gcry_mac_setkey(h, key, keysize), GPG_ERR_NO_ERROR);

end:
    /* noret */ gcry_mac_close(h);
}

int main(void)
{
    cmac(GCRY_MAC_CMAC_SERPENT, 64);
    cmac(GCRY_MAC_CMAC_IDEA, 32);
    cmac(GCRY_MAC_CMAC_RFC2268, 256);
    return 0;
}

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

[NIIBE Yutaka]

Hello,

Thank you for your report.

Guido Vranken via Gcrypt-devel <[hidden email]> wrote:
> I think the correct approach is to
> make gcry_mac_setkey() return an error code if the key has an inappropriate
> size.

Something like this?
--


diff --git a/cipher/idea.c b/cipher/idea.c
index 0a810818..7f706660 100644
--- a/cipher/idea.c
+++ b/cipher/idea.c
@@ -251,7 +251,9 @@ do_setkey( IDEA_context *c, const byte *key, unsigned int keylen )
     if( selftest_failed )
  return GPG_ERR_SELFTEST_FAILED;
 
-    assert(keylen == 16);
+    if (keylen != 16)
+      return GPG_ERR_INV_KEYLEN;
+
     c->have_dk = 0;
     expand_key( key, c->ek );
     invert_key( c->ek, c->dk );
diff --git a/cipher/rfc2268.c b/cipher/rfc2268.c
index f018b640..b093f022 100644
--- a/cipher/rfc2268.c
+++ b/cipher/rfc2268.c
@@ -228,6 +228,9 @@ setkey_core (void *context, const unsigned char *key, unsigned int keylen, int w
   if (keylen < 40 / 8) /* We want at least 40 bits. */
     return GPG_ERR_INV_KEYLEN;
 
+  if (keylen > 128)
+    return GPG_ERR_INV_KEYLEN;
+
   S = (unsigned char *) ctx->S;
 
   for (i = 0; i < keylen; i++)
diff --git a/cipher/serpent.c b/cipher/serpent.c
index 3c5eed2c..d2f7f16e 100644
--- a/cipher/serpent.c
+++ b/cipher/serpent.c
@@ -732,12 +732,15 @@ serpent_subkeys_generate (serpent_key_t key, serpent_subkeys_t subkeys)
 }
 
 /* Initialize CONTEXT with the key KEY of KEY_LENGTH bits.  */
-static void
+static gcry_err_code_t
 serpent_setkey_internal (serpent_context_t *context,
  const byte *key, unsigned int key_length)
 {
   serpent_key_t key_prepared;
 
+  if (key_length > 32)
+    return GPG_ERR_INV_KEYLEN;
+
   serpent_key_prepare (key, key_length, key_prepared);
   serpent_subkeys_generate (key_prepared, context->keys);
 
@@ -758,6 +761,7 @@ serpent_setkey_internal (serpent_context_t *context,
 #endif
 
   wipememory (key_prepared, sizeof(key_prepared));
+  return 0;
 }
 
 /* Initialize CTX with the key KEY of KEY_LENGTH bytes.  */
@@ -791,7 +795,7 @@ serpent_setkey (void *ctx,
   if (serpent_test_ret)
     ret = GPG_ERR_SELFTEST_FAILED;
   else
-    serpent_setkey_internal (context, key, key_length);
+    ret = serpent_setkey_internal (context, key, key_length);
 
   return ret;
 }

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

[NIIBE Yutaka]

NIIBE Yutaka <[hidden email]> wrote:
> Something like this?

After I confirmed it works correctly (returns an error), I applied and
pushed to libgcrypt master.
--

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

[Andreas Metzler-3]

On 2021-03-31 Guido Vranken via Gcrypt-devel <[hidden email]> wrote:
> In the program below, each of three calls to cmac() causes a different
> crash (use AddressSanitizer to be sure). I think the correct approach is to
> make gcry_mac_setkey() return an error code if the key has an inappropriate
> size.
[...]

Is this exploitable?

cu Andreas
--
`What a good friend you are to him, Dr. Maturin. His other friends are
so grateful to you.'
`I sew his ears on from time to time, sure'

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

[GnuPG - Libgcrypt - Dev mailing list]

In the case of IDEA, an oversized key only appears to cause a crash (assertion failure, so no memory corruption).

In the case of SERPENT, it appears to overwrite a stack buffer. That makes it unlikely to be exploitable when stack protector is enabled (which is the case for the binaries of most Linux distro's I think).

In the case of RC2, it appears to overwrite a heap buffer. That makes it more likely to be exploitable but you probably still have to deal with ASLR.

In addition to that the host application needs to allow setting oversized keys but applications usually only allow hardcoded, legal key lengths.

With that said, exploitation might be possible in specific circumstances.

On Fri, Apr 2, 2021 at 6:00 PM Andreas Metzler <[hidden email]> wrote:

On 2021-03-31 Guido Vranken via Gcrypt-devel <[hidden email]> wrote:
> In the program below, each of three calls to cmac() causes a different
> crash (use AddressSanitizer to be sure). I think the correct approach is to
> make gcry_mac_setkey() return an error code if the key has an inappropriate
> size.
[...]

Is this exploitable?

cu Andreas
--
`What a good friend you are to him, Dr. Maturin. His other friends are
so grateful to you.'
`I sew his ears on from time to time, sure'

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

[GnuPG - Libgcrypt - Dev mailing list]

On Fri,  2 Apr 2021 22:51, Guido Vranken said:

> With that said, exploitation might be possible in specific circumstances.

... and it would be much easier to attack the application than
Libgcrypt.  An application which does not take care from where it gets
the key has for sure a lot of other problems.


Shalom-Salam,

   Werner

--
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel