check argv[] before doing any decoding of packets.

diff --git a/src/protocols/tacacs/decode.c b/src/protocols/tacacs/decode.c
index 70cba504a255b67864b00788ba7864494e4bf899..3c53d9e30f6c7635e88b3096ee65c6a1803a67e6 100644 (file)
--- a/src/protocols/tacacs/decode.c
+++ b/src/protocols/tacacs/decode.c
@@ -133,12 +133,20 @@ int fr_tacacs_packet_to_code(fr_tacacs_packet_t const *pkt)
        } \
 } while (0)
 
-#define ARG_COUNT_CHECK(_msg, _arg_cnt) do { \
-       if ((p + _arg_cnt) > end) { \
-               fr_strerror_printf("Argument count %u overflows the remaining data (%zu) in the %s packet", _arg_cnt, end - p, _msg); \
+#define ARG_COUNT_CHECK(_msg, _hdr) do { \
+       if ((p + _hdr.arg_cnt) > end) { \
+               fr_strerror_printf("Argument count %u overflows the remaining data (%zu) in the %s packet", _hdr.arg_cnt, end - p, _msg); \
                goto fail; \
        } \
-       p += _arg_cnt; \
+       p += _hdr.arg_cnt; \
+       data_len = 0; \
+       for (int i = 0; i < _hdr.arg_cnt; i++) { \
+               data_len += _hdr.arg_len[i]; \
+               if (data_len > (size_t) (end - p)) { \
+                       fr_strerror_printf("Argument %u length %u overflows packet", i, _hdr.arg_len[i]); \
+                       goto fail; \
+               } \
+       } \
 } while (0)
 
 #define DECODE_FIELD_UINT8(_da, _field) do { \
@@ -160,11 +168,11 @@ int fr_tacacs_packet_to_code(fr_tacacs_packet_t const *pkt)
 
 #define BODY(_x) (((uint8_t const *) pkt) + sizeof(pkt->hdr) + sizeof(pkt->_x))
 
-/**
- *     Decode a TACACS+ 'arg_N' fields.
+/** Decode a TACACS+ 'arg_N' fields.
+ *
  */
 static int tacacs_decode_args(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr_t const *parent,
-                             uint8_t arg_cnt, uint8_t const *arg_list, uint8_t const *data, uint8_t const *end)
+                             uint8_t arg_cnt, uint8_t const *arg_list, uint8_t const *data, NDEBUG_UNUSED uint8_t const *end)
 {
        uint8_t i;
        uint8_t const *p = data;
@@ -175,24 +183,7 @@ static int tacacs_decode_args(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr
         */
        if (!arg_cnt) return 0;
 
-       if ((p + arg_cnt) > end) {
-               fr_strerror_printf("Argument count %u overflows the remaining data (%zu) in the packet", arg_cnt, p - end);
-               return -1;
-       }
-
-       /*
-        *      Check for malformed packets before anything else.
-        */
-       for (i = 0; i < arg_cnt; i++) {
-               if ((p + arg_list[i]) > end) {
-                       fr_strerror_printf("'%s' argument %u length %u overflows the remaining data (%zu) in the packet",
-                                          parent->name, i, arg_list[i], end - p);
-                       return -1;
-               }
-
-               p += arg_list[i];
-       }
-       p = data;
+       fr_assert((p + arg_cnt) < end);
 
        /*
         *      Then, do the dirty job of creating attributes.
@@ -202,6 +193,9 @@ static int tacacs_decode_args(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr
                fr_dict_attr_t const *da;
                uint8_t buffer[256];
 
+               fr_assert(p < end);
+               fr_assert((p + arg_list[i]) <= end);
+
                if (arg_list[i] < 2) goto next; /* skip malformed */
 
                memcpy(buffer, p, arg_list[i]);
@@ -771,7 +765,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu
                        /* can't check for underflow, as we have argv[argc] */
 
                        p = BODY(author_req);
-                       ARG_COUNT_CHECK("Authorization-Request", pkt->author_req.arg_cnt);
+                       ARG_COUNT_CHECK("Authorization-Request", pkt->author_req);
                        DECODE_FIELD_UINT8(attr_tacacs_packet_body_type, FR_PACKET_BODY_TYPE_REQUEST);
 
                        /*
@@ -830,7 +824,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu
                        /* can't check for underflow, as we have argv[argc] */
 
                        p = BODY(author_reply);
-                       ARG_COUNT_CHECK("Authorization-Reply", pkt->author_reply.arg_cnt);
+                       ARG_COUNT_CHECK("Authorization-Reply", pkt->author_reply);
                        DECODE_FIELD_UINT8(attr_tacacs_packet_body_type, FR_PACKET_BODY_TYPE_RESPONSE);
 
                        /*
@@ -893,7 +887,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu
                        /* can't check for underflow, as we have argv[argc] */
 
                        p = BODY(acct_req);
-                       ARG_COUNT_CHECK("Accounting-Request", pkt->acct_req.arg_cnt);
+                       ARG_COUNT_CHECK("Accounting-Request", pkt->acct_req);
                        DECODE_FIELD_UINT8(attr_tacacs_packet_body_type, FR_PACKET_BODY_TYPE_REQUEST);
 
                        /*

diff --git a/src/tests/unit/protocols/tacacs/base.txt b/src/tests/unit/protocols/tacacs/base.txt
index ad505253d1aa2557ccd6331080ecc1282e38f3b7..7450b934684c6a696d9aaed0ab2534a6fd92ae7c 100644 (file)
--- a/src/tests/unit/protocols/tacacs/base.txt
+++ b/src/tests/unit/protocols/tacacs/base.txt
@@ -67,7 +67,7 @@ encode-proto -
 match c0 03 02 00 07 9b 35 d9 00 00 00 05 49 d8 e5 4a 73
 
 decode-proto c002 20ff 2020 2020 0000 0043 2009 0000 0009 000a 2120 2020 2020 2020 2020 20ff ff20 2020 2020 2020 ffff ffff 2020 4441 5461 2a30 7820 2020 2020 2020 2020 2020 2020 2020 20ff ffff 20ff ff20 2020 20
-match 'Argument-List' argument 3 length 32 overflows the remaining data (0) in the packet
+match Argument 3 length 32 overflows packet
 
 count
 match 29