From bb242a04beebf09ff82e36eeaf738504cac985a9 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Fri, 17 Mar 2023 17:45:32 +0530 Subject: [PATCH 1/9] util/base64: add test for long string w RFC4648 (cherry picked from commit 85ae1d25e4998d19cb1f7fd714027b3da1c8aa4e) --- src/util-base64.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/util-base64.c b/src/util-base64.c index e84224e5b01d..aa07f79f4e57 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -347,6 +347,9 @@ static int B64TestVectorsRFC4648(void) const char *src9 = "Zm$9vYm.Fy"; const char *fin_str9 = "f"; + const char *src10 = "Y21Wd2IzSjBaVzFoYVd4bWNtRjFaRUJoZEc4dVoyOTJMbUYxOmpqcHh4b3Rhb2w%3D"; + const char *fin_str10 = "cmVwb3J0ZW1haWxmcmF1ZEBhdG8uZ292LmF1:jjpxxotaol"; + TEST_RFC4648(src1, fin_str1, ASCII_BLOCK * 2, strlen(fin_str1), strlen(src1), BASE64_ECODE_OK); TEST_RFC4648(src2, fin_str2, ASCII_BLOCK * 2, strlen(fin_str2), strlen(src2), BASE64_ECODE_OK); TEST_RFC4648(src3, fin_str3, ASCII_BLOCK * 2, strlen(fin_str3), strlen(src3), BASE64_ECODE_OK); @@ -356,6 +359,8 @@ static int B64TestVectorsRFC4648(void) TEST_RFC4648(src7, fin_str7, ASCII_BLOCK * 2, strlen(fin_str7), strlen(src7), BASE64_ECODE_OK); TEST_RFC4648(src8, fin_str8, ASCII_BLOCK * 2, 1 /* f */, 2 /* Zm */, BASE64_ECODE_ERR); TEST_RFC4648(src9, fin_str9, ASCII_BLOCK * 2, 1 /* f */, 2 /* Zm */, BASE64_ECODE_ERR); + TEST_RFC4648(src10, fin_str10, strlen(fin_str10) + 1, strlen(fin_str10), strlen(src10) - 3, + BASE64_ECODE_ERR); PASS; } From 328e2474d3a7394227586952281975d027b32a14 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Fri, 17 Mar 2023 17:48:35 +0530 Subject: [PATCH 2/9] util/base64: skip any invalid char for RFC2045 RFC 2045 states that any invalid character should be skipped over, this is the RFC used by mime handler in Suricata code to deal with base64 encoded data. So far, only spaces were skipped as a part of implementation of this RFC, extend it to also skip over any other invalid character. Add corresponding test. (cherry picked from commit e46b0337e50897424d05371aa26d5f20e172f749) --- src/util-base64.c | 18 +++++++++++------- src/util-base64.h | 4 ++-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/util-base64.c b/src/util-base64.c index aa07f79f4e57..ab9ed6d8b693 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -106,9 +106,9 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, /* Get decimal representation */ val = GetBase64Value(src[i]); if (val < 0) { - if ((mode == BASE64_MODE_RFC2045) && (src[i] == ' ')) { + if (mode == BASE64_MODE_RFC2045 && src[i] != '=') { if (bbidx == 0) { - /* Special case where last block of data has a leading space */ + /* Special case where last block of data has a leading space or invalid char */ leading_sp++; } sp++; @@ -155,7 +155,7 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, } } - if (!valid && mode == BASE64_MODE_RFC4648) { + if (bbidx > 0 && bbidx < 4 && ((!valid && mode == BASE64_MODE_RFC4648))) { padding = B64_BLOCK - bbidx; *decoded_bytes += ASCII_BLOCK - padding; DecodeBase64Block(dptr, b64); @@ -303,9 +303,12 @@ static int B64TestVectorsRFC2045(void) const char *fin_str8 = "foobar"; const char *src9 = "Zm$9vYm.Fy"; - const char *fin_str9 = ""; + const char *fin_str9 = "foobar"; - TEST_RFC2045(src1, fin_str1, strlen(fin_str1), strlen(fin_str1), strlen(src1), BASE64_ECODE_OK); + const char *src10 = "Y21Wd2IzSjBaVzFoYVd4bWNtRjFaRUJoZEc4dVoyOTJMbUYxOmpqcHh4b3Rhb2w%5d"; + const char *fin_str10 = "cmVwb3J0ZW1haWxmcmF1ZEBhdG8uZ292LmF1:jjpxxotaol9t"; + + TEST_RFC2045(src1, fin_str1, ASCII_BLOCK * 2, strlen(fin_str1), strlen(src1), BASE64_ECODE_OK); TEST_RFC2045(src2, fin_str2, ASCII_BLOCK * 2, strlen(fin_str2), strlen(src2), BASE64_ECODE_OK); TEST_RFC2045(src3, fin_str3, ASCII_BLOCK * 2, strlen(fin_str3), strlen(src3), BASE64_ECODE_OK); TEST_RFC2045(src4, fin_str4, ASCII_BLOCK * 2, strlen(fin_str4), strlen(src4), BASE64_ECODE_OK); @@ -313,8 +316,9 @@ static int B64TestVectorsRFC2045(void) TEST_RFC2045(src6, fin_str6, ASCII_BLOCK * 2, strlen(fin_str6), strlen(src6), BASE64_ECODE_OK); TEST_RFC2045(src7, fin_str7, ASCII_BLOCK * 2, strlen(fin_str7), strlen(src7), BASE64_ECODE_OK); TEST_RFC2045(src8, fin_str8, ASCII_BLOCK * 2, strlen(fin_str8), strlen(src8), BASE64_ECODE_OK); - TEST_RFC2045(src9, fin_str9, ASCII_BLOCK * 2, 0, 0, - BASE64_ECODE_ERR); // TODO this should be accepted just like the previous string + TEST_RFC2045(src9, fin_str9, ASCII_BLOCK * 2, strlen(fin_str9), strlen(src9), BASE64_ECODE_OK); + TEST_RFC2045(src10, fin_str10, strlen(fin_str10) + 3, strlen(fin_str10), strlen(src10), + BASE64_ECODE_OK); PASS; } diff --git a/src/util-base64.h b/src/util-base64.h index 57bf7135a86b..fdaf9d6fa012 100644 --- a/src/util-base64.h +++ b/src/util-base64.h @@ -62,8 +62,8 @@ typedef enum { * BASE64("fooba") = "Zm9vYmE=" * BASE64("foobar") = "Zm9vYmFy" * BASE64("foobar") = "Zm 9v Ym Fy" <-- Notice how the spaces are ignored - * BASE64("f") = "Zm$9vYm.Fy" # TODO according to RFC, All line breaks or *other characters* - * not found in base64 alphabet must be ignored by decoding software + * BASE64("foobar") = "Zm$9vYm.Fy" # According to RFC 2045, All line breaks or *other + * characters* not found in base64 alphabet must be ignored by decoding software * */ BASE64_MODE_RFC2045, /* SPs are allowed during transfer but must be skipped by Decoder */ BASE64_MODE_STRICT, From f7b5bda27275b24bcf05872519fb9cc6bd9230b3 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 30 Mar 2023 12:54:29 +0530 Subject: [PATCH 3/9] util/base64: fix padding bytes for trailing data Padding bytes for the last remainder data should be as follows: Case | Remainder bytes | Padding ---------------------------------------------- I | 1 | 3 II | 2 | 2 III | 3 | 1 However, we calculate the decoded_bytes with the formula: decoded_bytes = ASCII_BLOCK - padding this means for Case I when padding is 3 bytes, the decoded_bytes would be 0. This is incorrect for any trailing data. In any of the above cases, if the parsing was successful, there should at least be 1 decoded byte. (cherry picked from commit 095c335c72befec2cfcd43390f86d116926bcd17) --- src/util-base64.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util-base64.c b/src/util-base64.c index ab9ed6d8b693..1c99dc636724 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -156,7 +156,8 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, } if (bbidx > 0 && bbidx < 4 && ((!valid && mode == BASE64_MODE_RFC4648))) { - padding = B64_BLOCK - bbidx; + /* Decoded bytes for 1 or 2 base64 encoded bytes is 1 */ + padding = bbidx > 1 ? B64_BLOCK - bbidx : 2; *decoded_bytes += ASCII_BLOCK - padding; DecodeBase64Block(dptr, b64); *consumed_bytes += bbidx; From cb9dd4be1d84a1b04e8aa55d9d20db4ddd83547b Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 30 Mar 2023 13:11:12 +0530 Subject: [PATCH 4/9] util/base64: check for dest buf size in last block Just like the check for destination buffer size done previously for complete data, it should also be done for the trailing data to avoid goind out of bounds. (cherry picked from commit 0e8b451699218b3f3430d7614f76cffed7ba991c) --- src/util-base64.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/util-base64.c b/src/util-base64.c index 1c99dc636724..678bc14c66ba 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -158,7 +158,13 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, if (bbidx > 0 && bbidx < 4 && ((!valid && mode == BASE64_MODE_RFC4648))) { /* Decoded bytes for 1 or 2 base64 encoded bytes is 1 */ padding = bbidx > 1 ? B64_BLOCK - bbidx : 2; - *decoded_bytes += ASCII_BLOCK - padding; + uint32_t numDecoded_blk = ASCII_BLOCK - (padding < B64_BLOCK ? padding : ASCII_BLOCK); + if (dest_size < *decoded_bytes + numDecoded_blk) { + SCLogDebug("Destination buffer full"); + ecode = BASE64_ECODE_BUF; + return ecode; + } + *decoded_bytes += numDecoded_blk; DecodeBase64Block(dptr, b64); *consumed_bytes += bbidx; } From 9308a6c7a8ef172eec225f478dd0337dbb49dbe9 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 30 Mar 2023 13:13:08 +0530 Subject: [PATCH 5/9] util/base64: check dest buf size to hold 3Bytes The destination buffer should be able to hold at least 3 Bytes during the processing of the last block of data. If it cannot hold at least 3 Bytes, then that may lead to dynamic buffer overflow while decoding. (cherry picked from commit 62d782156caddec0b4ca795d7236c6483d02efff) --- src/util-base64.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/util-base64.c b/src/util-base64.c index 678bc14c66ba..95a260ea7db4 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -164,6 +164,11 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, ecode = BASE64_ECODE_BUF; return ecode; } + /* if the destination size is not at least 3 Bytes long, it'll give a dynamic + * buffer overflow while decoding, so, return and let the caller take care of the + * remaining bytes to be decoded which should always be < 4 at this stage */ + if (dest_size - *decoded_bytes < 3) + return BASE64_ECODE_BUF; *decoded_bytes += numDecoded_blk; DecodeBase64Block(dptr, b64); *consumed_bytes += bbidx; From 3661231c9af02e2c9b56832f4e30552759f80f2d Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 30 Mar 2023 13:19:31 +0530 Subject: [PATCH 6/9] util/base64: update test macro to use user data (cherry picked from commit c2ae87eecfafe6d46180c207c35c038035fe9c16) --- src/util-base64.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util-base64.c b/src/util-base64.c index 95a260ea7db4..1ad279186e3a 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -195,7 +195,7 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, { \ uint32_t consumed_bytes = 0, num_decoded = 0; \ uint8_t dst[dest_size]; \ - Base64Ecode code = DecodeBase64(dst, strlen(fin_str), (const uint8_t *)src, strlen(src), \ + Base64Ecode code = DecodeBase64(dst, dest_size, (const uint8_t *)src, strlen(src), \ &consumed_bytes, &num_decoded, BASE64_MODE_RFC2045); \ FAIL_IF(code != ecode); \ FAIL_IF(memcmp(dst, fin_str, strlen(fin_str)) != 0); \ @@ -207,7 +207,7 @@ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, { \ uint32_t consumed_bytes = 0, num_decoded = 0; \ uint8_t dst[dest_size]; \ - Base64Ecode code = DecodeBase64(dst, strlen(fin_str), (const uint8_t *)src, strlen(src), \ + Base64Ecode code = DecodeBase64(dst, dest_size, (const uint8_t *)src, strlen(src), \ &consumed_bytes, &num_decoded, BASE64_MODE_RFC4648); \ FAIL_IF(code != ecode); \ FAIL_IF(memcmp(dst, fin_str, strlen(fin_str)) != 0); \ From 6b42c09634da63ba5e7cf436a1f8c81f1acb1455 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 30 Mar 2023 13:41:08 +0530 Subject: [PATCH 7/9] util/base64: fix tests for RFC2045 (cherry picked from commit 49ad38329a3a96ba22e73da38b4594ebf8759ec9) --- src/util-base64.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/util-base64.c b/src/util-base64.c index 1ad279186e3a..f2abfe4a2e26 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -232,7 +232,7 @@ static int B64DecodeInCompleteString(void) * SGVsbG8gV29ybGR6 : Hello Worldz * */ const char *src = "SGVsbG8gV29ybGR"; - const char *fin_str = "Hello Wor"; // bc it'll error out on last 3 bytes + const char *fin_str = "Hello Wor"; TEST_RFC2045(src, fin_str, strlen(fin_str), strlen(fin_str), strlen(src) - 3, BASE64_ECODE_OK); PASS; } @@ -245,7 +245,7 @@ static int B64DecodeCompleteStringWSp(void) const char *src = "SGVs bG8 gV29y bGQ="; const char *fin_str = "Hello World"; - TEST_RFC2045(src, fin_str, strlen(fin_str) + 1, strlen(fin_str), strlen(src), BASE64_ECODE_OK); + TEST_RFC2045(src, fin_str, strlen(fin_str) + 3, strlen(fin_str), strlen(src), BASE64_ECODE_OK); PASS; } @@ -258,7 +258,8 @@ static int B64DecodeInCompleteStringWSp(void) const char *src = "SGVs bG8 gV29y bGQ"; const char *fin_str = "Hello Wor"; - TEST_RFC2045(src, fin_str, strlen(fin_str), strlen(fin_str), strlen(src) - 3, BASE64_ECODE_OK); + TEST_RFC2045(src, fin_str, strlen(fin_str) + 1 /* 12 B in dest_size */, strlen(fin_str), + strlen(src) - 3, BASE64_ECODE_OK); PASS; } @@ -317,8 +318,8 @@ static int B64TestVectorsRFC2045(void) const char *src9 = "Zm$9vYm.Fy"; const char *fin_str9 = "foobar"; - const char *src10 = "Y21Wd2IzSjBaVzFoYVd4bWNtRjFaRUJoZEc4dVoyOTJMbUYxOmpqcHh4b3Rhb2w%5d"; - const char *fin_str10 = "cmVwb3J0ZW1haWxmcmF1ZEBhdG8uZ292LmF1:jjpxxotaol9t"; + const char *src10 = "Y21Wd2IzSjBaVzFoYVd4bWNtRjFaRUJoZEc4dVoyOTJMbUYxOmpqcHh4b3Rhb2w%5"; + const char *fin_str10 = "cmVwb3J0ZW1haWxmcmF1ZEBhdG8uZ292LmF1:jjpxxotaol9"; TEST_RFC2045(src1, fin_str1, ASCII_BLOCK * 2, strlen(fin_str1), strlen(src1), BASE64_ECODE_OK); TEST_RFC2045(src2, fin_str2, ASCII_BLOCK * 2, strlen(fin_str2), strlen(src2), BASE64_ECODE_OK); @@ -329,7 +330,7 @@ static int B64TestVectorsRFC2045(void) TEST_RFC2045(src7, fin_str7, ASCII_BLOCK * 2, strlen(fin_str7), strlen(src7), BASE64_ECODE_OK); TEST_RFC2045(src8, fin_str8, ASCII_BLOCK * 2, strlen(fin_str8), strlen(src8), BASE64_ECODE_OK); TEST_RFC2045(src9, fin_str9, ASCII_BLOCK * 2, strlen(fin_str9), strlen(src9), BASE64_ECODE_OK); - TEST_RFC2045(src10, fin_str10, strlen(fin_str10) + 3, strlen(fin_str10), strlen(src10), + TEST_RFC2045(src10, fin_str10, strlen(fin_str10) + 2, strlen(fin_str10), strlen(src10), BASE64_ECODE_OK); PASS; } From cdd6e20fbafadbfbae41ae44d7545eb6a93b7b45 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Tue, 11 Jul 2023 14:42:05 +0530 Subject: [PATCH 8/9] util/mime: skip over any invalid char For certain edge case handling for spaces, spaces were handled particularly in the remainder processing functions. Make sure that now that as per RFC 2045, util-base64 would skip over any invalid char, the edge cases in MIME processor also be handled the same way. This completes the work done in e46b033. Ticket 6135 Ticket 6207 (cherry picked from commit 789353bc1e1aa23d075f16af25df84df00c68682) --- src/util-base64.c | 15 +++++++++++++++ src/util-base64.h | 1 + src/util-decode-mime.c | 7 ++++--- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/util-base64.c b/src/util-base64.c index f2abfe4a2e26..13a8e1eabb23 100644 --- a/src/util-base64.c +++ b/src/util-base64.c @@ -62,6 +62,21 @@ static inline int GetBase64Value(uint8_t c) return val; } +/** + * \brief Checks if the given char in a byte array is Base64 alphabet + * + * \param Char that needs to be checked + * + * \return True if the char was Base64 alphabet, False otherwise + */ +bool IsBase64Alphabet(uint8_t encoded_byte) +{ + if (GetBase64Value(encoded_byte) < 0 && encoded_byte != '=') { + return false; + } + return true; +} + /** * \brief Decodes a 4-byte base64-encoded block into a 3-byte ascii-encoded block * diff --git a/src/util-base64.h b/src/util-base64.h index fdaf9d6fa012..ce490d46584a 100644 --- a/src/util-base64.h +++ b/src/util-base64.h @@ -95,6 +95,7 @@ typedef enum { /* Function prototypes */ Base64Ecode DecodeBase64(uint8_t *dest, uint32_t dest_size, const uint8_t *src, uint32_t len, uint32_t *consumed_bytes, uint32_t *decoded_bytes, Base64Mode mode); +bool IsBase64Alphabet(uint8_t encoded_byte); #endif diff --git a/src/util-decode-mime.c b/src/util-decode-mime.c index d5e2f1c2e5a9..b6c231b36374 100644 --- a/src/util-decode-mime.c +++ b/src/util-decode-mime.c @@ -1197,7 +1197,7 @@ static uint32_t ProcessBase64Remainder( /* Strip spaces in remainder */ for (uint8_t i = 0; i < state->bvr_len; i++) { - if (state->bvremain[i] != ' ') { + if (IsBase64Alphabet(state->bvremain[i])) { block[cnt++] = state->bvremain[i]; } } @@ -1205,7 +1205,7 @@ static uint32_t ProcessBase64Remainder( /* if we don't have 4 bytes see if we can fill it from `buf` */ if (buf && len > 0 && cnt != B64_BLOCK) { for (uint32_t i = 0; i < len && cnt < B64_BLOCK; i++) { - if (buf[i] != ' ') { + if (IsBase64Alphabet(buf[i])) { block[cnt++] = buf[i]; } buf_consumed++; @@ -1289,7 +1289,8 @@ static inline MimeDecRetCode ProcessBase64BodyLineCopyRemainder( return MIME_DEC_ERR_DATA; for (uint32_t i = offset; i < buf_len; i++) { - if (buf[i] != ' ') { + // Skip any characters outside of the base64 alphabet as per RFC 2045 + if (IsBase64Alphabet(buf[i])) { DEBUG_VALIDATE_BUG_ON(state->bvr_len >= B64_BLOCK); if (state->bvr_len >= B64_BLOCK) return MIME_DEC_ERR_DATA; From 77faa8833f251d0a8f7f473070c130b90d96a770 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Wed, 26 Jul 2023 15:32:29 +0530 Subject: [PATCH 9/9] workflows: use debug-failed for s-v run --- .github/workflows/builds.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 6e8a0d2824d9..b3eea82c7f8b 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -504,7 +504,7 @@ jobs: - name: Extracting suricata-verify run: tar xf prep/suricata-verify.tar.gz - name: Running suricata-verify - run: python3 ./suricata-verify/run.py -q + run: python3 ./suricata-verify/run.py -q --debug-failed # Now install and make sure headers and libraries aren't # installed until requested. - run: make install @@ -581,7 +581,7 @@ jobs: - name: Extracting suricata-verify run: tar xf prep/suricata-verify.tar.gz - name: Running suricata-verify - run: python3 ./suricata-verify/run.py -q + run: python3 ./suricata-verify/run.py -q --debug-failed - run: make install - run: suricata-update -V - run: suricatasc -h @@ -671,7 +671,7 @@ jobs: - name: Extracting suricata-verify run: tar xf prep/suricata-verify.tar.gz - name: Running suricata-verify - run: python3 ./suricata-verify/run.py -q + run: python3 ./suricata-verify/run.py -q --debug-failed - run: make install - run: suricata-update -V - run: suricatasc -h @@ -751,7 +751,7 @@ jobs: - name: Extracting suricata-verify run: tar xf prep/suricata-verify.tar.gz - name: Running suricata-verify - run: python3 ./suricata-verify/run.py -q + run: python3 ./suricata-verify/run.py -q --debug-failed - run: make install - run: suricata-update -V - run: suricatasc -h @@ -1531,7 +1531,7 @@ jobs: run: | ./src/suricata --build-info ./src/suricata -u -l /tmp/ - python3 ./suricata-verify/run.py -q + python3 ./suricata-verify/run.py -q --debug-failed windows-msys2-mingw64-windivert: name: Windows MSYS2 MINGW64 (WinDivert)