tighten restrictions on decoding

* reject indefinite length forms for all tags
* reject lengths which are non minimal

diff --git a/src/protocols/der/decode.c b/src/protocols/der/decode.c
index ac8752c43e14e7db2f091a1c37c95098005760e5..a53c8f71f710d79f8487191e82b12df3923f5e12 100644 (file)
--- a/src/protocols/der/decode.c
+++ b/src/protocols/der/decode.c
@@ -2011,21 +2011,46 @@ static ssize_t fr_der_decode_hdr(fr_dict_attr_t const *parent, fr_dbuff_t *in, u
                *len            = 0;
 
                /*
-                *      Length bits of zero is an indeterminate length field where
-                *      the length is encoded in the data instead.
+                *      Length-of-length of zero is the BER indefinite-length form.  DER (X.690 Section 10.1)
+                *      forbids it for both primitive and constructed encodings.  If we accept it, then an
+                *      attacker can hide a zero-length value with trailing bytes, which then gets reparsed as
+                *      a sibling TLV.
                 */
-               if (len_len > 0) {
-                       if (unlikely(len_len > sizeof(*len))) {
-                               fr_strerror_printf_push("Length field too large (%" PRIu32 ")", len_len);
-                               return -1;
-                       }
+               if (unlikely(len_len == 0)) {
+                       fr_strerror_const_push("Indefinite-length form is forbidden in DER");
+                       return -1;
+               }
 
-                       while (len_len--) {
-                               if (fr_dbuff_out(&len_byte, &our_in) < 0) goto error;
-                               *len = (*len << 8) | len_byte;
-                       }
-               } else if (!constructed) {
-                       fr_strerror_const_push("Primitive data with indefinite form length field is invalid");
+               if (unlikely(len_len > sizeof(*len))) {
+                       fr_strerror_printf_push("Length field too large (%" PRIu32 ")", len_len);
+                       return -1;
+               }
+
+               /*
+                *      DER (X.690 Section 10.1) mandates minimal length encoding.  In the long form, the
+                *      leading length octet must not be zero, otherwise the length could be expressed in
+                *      fewer octets.
+                */
+               if (fr_dbuff_out(&len_byte, &our_in) < 0) goto error;
+               if (unlikely(len_byte == 0)) {
+                       fr_strerror_const_push("Non-minimal DER length encoding (leading zero in long form)");
+                       return -1;
+               }
+               *len = len_byte;
+               len_len--;
+
+               while (len_len--) {
+                       if (fr_dbuff_out(&len_byte, &our_in) < 0) goto error;
+                       *len = (*len << 8) | len_byte;
+               }
+
+               /*
+                *      DER also mandates the short form whenever the
+                *      length fits in 7 bits.  Reject the long form when
+                *      the value is < 128.
+                */
+               if (unlikely(*len < 128)) {
+                       fr_strerror_printf_push("Non-minimal DER length encoding (long form used for length %zu)", *len);
                        return -1;
                }
 

diff --git a/src/tests/unit/protocols/der/error.txt b/src/tests/unit/protocols/der/error.txt
index 5d7b9aaa1fe3b08720bd0a46a1de36c0ee9bdb11..c1180d34597ec10c97cb247054bd3dbe3c5e3ba5 100644 (file)
--- a/src/tests/unit/protocols/der/error.txt
+++ b/src/tests/unit/protocols/der/error.txt
@@ -52,7 +52,7 @@ match Constructed flag mismatch for tag 1: Failed decoding Test-Boolean header
 proto-dictionary-root Test-Integer
 
 decode-pair 02 80 02 01 05 00 00
-match Primitive data with indefinite form length field is invalid: Failed decoding Test-Integer header
+match Indefinite-length form is forbidden in DER: Failed decoding Test-Integer header
 
 #
 #  ----  NULL errors  ----