libtom / libtomcrypt

LibTomCrypt is a fairly comprehensive, modular and portable cryptographic toolkit that provides developers with a vast array of well known published block ciphers, one-way hash functions, chaining modes, pseudo-random number generators, public key cryptography and a plethora of other routines.
https://www.libtom.net
Other
1.55k stars 457 forks source link

test for issue #507 always passes #561

Open jamuir opened 3 years ago

jamuir commented 3 years ago

Description

There is a test vector for issue #507 in tests/der_test.c. However, if you revert the fix for #507, the test still passes (so the test in its current form doesn't really tell us much).

Steps to Reproduce

One way is to reset to d2027d60, revert 25c26a3b and then run the tests.

Another way is to revert the fix for #507 manually:

diff --git a/src/pk/asn1/der/utf8/der_decode_utf8_string.c b/src/pk/asn1/der/utf8/der_decode_utf8_string.c
index 93a5e5ed..c2b6bb6b 100644
--- a/src/pk/asn1/der/utf8/der_decode_utf8_string.c
+++ b/src/pk/asn1/der/utf8/der_decode_utf8_string.c
@@ -77,7 +77,7 @@ int der_decode_utf8_string(const unsigned char *in,  unsigned long inlen,
       for (z = 0; (tmp & 0x80) && (z <= 4); z++, tmp = (tmp << 1) & 0xFF);

       /* z should be in {0,2,3,4} */
-      if (z == 1 || z > 4) {
+      if (z > 4) {
          return CRYPT_INVALID_PACKET;
       }

@@ -85,7 +85,7 @@ int der_decode_utf8_string(const unsigned char *in,  unsigned long inlen,
       tmp >>= z;

       /* now update z so it equals the number of additional bytes to read */
-      if (z > 0) { --z; }
+      if (z > 1) { --z; }

       if (x + z > inlen) {
          return CRYPT_INVALID_PACKET;

To run the der test, I do this:

CFLAGS="-DUSE_LTM -DLTM_DESC -I../libtommath" EXTRALIBS="../libtommath/libtommath.a" make test && ./test der
jamuir commented 3 years ago

The test vector for #507 is expected to cause a failure (i.e. fail is good).

The reason it still fails when the fix is reverted/removed is because the ASN1 parser encounters invalid data when it reads beyond the test vector (typically it will read a byte that is not a valid ASN1 type id).

jamuir commented 3 years ago

I think the primary vulnerability uncovered in #507 is not in utf-8 decoding; it is the underflow of the unsigned computation "*inlen -= len;" inside src/pk/asn1/der/sequence/der_decode_sequence_flexi.c.

There should be an underflow check added inside der_decode_sequence_flexi.c. I have a change ready that does that, but we still need a good test for #507. This is why I opened the current issue.