Security #6902
closedbase64: off-by-three overflow in DecodeBase64()
fd47e67dc65f9111895c88fb406c938b1f857325
Description
static inline void DecodeBase64Block(uint8_t ascii[ASCII_BLOCK], uint8_t b64[B64_BLOCK]) { ascii[0] = (uint8_t) (b64[0] << 2) | (b64[1] >> 4); ascii[1] = (uint8_t) (b64[1] << 4) | (b64[2] >> 2); ascii[2] = (uint8_t) (b64[2] << 6) | (b64[3]); } 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) { int val; uint32_t padding = 0, bbidx = 0, sp = 0, leading_sp = 0; uint8_t *dptr = dest; uint8_t b64[B64_BLOCK] = { 0,0,0,0 }; bool valid = true; Base64Ecode ecode = BASE64_ECODE_OK; *decoded_bytes = 0; for (uint32_t i = 0; i < len; i++) { val = GetBase64Value(src[i]); if (val < 0) { if (mode == BASE64_MODE_RFC2045 && src[i] != '=') { if (bbidx == 0) { leading_sp++; } sp++; continue; } if (src[i] != '=') { valid = false; ecode = BASE64_ECODE_ERR; if (mode == BASE64_MODE_STRICT) { *decoded_bytes = 0; } break; } padding++; } b64[bbidx++] = (val > 0 ? (uint8_t)val : 0); if (bbidx == B64_BLOCK) { [1] uint32_t numDecoded_blk = ASCII_BLOCK - (padding < B64_BLOCK ? padding : ASCII_BLOCK); [2] if (dest_size < *decoded_bytes + numDecoded_blk) { SCLogDebug("Destination buffer full"); ecode = BASE64_ECODE_BUF; break; } [3] DecodeBase64Block(dptr, b64); dptr += numDecoded_blk; *decoded_bytes += numDecoded_blk; bbidx = 0; padding = 0; *consumed_bytes += B64_BLOCK + sp; sp = 0; leading_sp = 0; memset(&b64, 0, sizeof(b64)); } } ... if (valid && bbidx > 0 && (mode != BASE64_MODE_RFC2045)) { *decoded_bytes += ASCII_BLOCK - (B64_BLOCK - bbidx); [4] DecodeBase64Block(dptr, b64); } .. }
Consider the situation when destination buffer is full, ie 'dest_size' is equal to '*decoded_bytes'.
1. - in case 'padding' is equal to B64_BLOCK (4), 'numDecoded_blk' will be set to 0
2. - we will pass this check, as 'dest_size' is equal to '*decoded_bytes'
3. - 3 bytes will be written past the end of buffer 'dptr'
4. - similar issue
How to verify:
1) get source code
$ git clone https://github.com/OISF/suricata
2) Edit src/util-base64.c and change B64DecodeCompleteString() test.
Change string "SGVsbG8gV29ybGR6" to "SGVsbG8gV29ybGR6===="
3) build
$ export CFLAGS="-ggdb3 -Werror -Wchar-subscripts -fno-strict-aliasing -fstack-protector-all -fsanitize=address -fno-omit-frame-pointer -Wno-unused-parameter -Wno-unused-function"
$ ./autoconf.sh
$ ./configure --enable-unittests
$ make
4) run
$ ./src/suricata -U B64DecodeCompleteString -u
ASAN LOG:
==77257==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffcc360882c at pc 0x55907c573699 bp 0x7ffcc36086c0 sp 0x7ffcc36086b0 WRITE of size 1 at 0x7ffcc360882c thread T0 (Suricata-Main) #0 0x55907c573698 in DecodeBase64Block suricata/src/util-base64.c:91 #1 0x55907c573c75 in DecodeBase64 suricata/src/util-base64.c:161 #2 0x55907c574378 in B64DecodeCompleteString suricata/src/util-base64.c:241 #3 0x55907bc7a22c in UtRunTests suricata/src/util-unittest.c:212 #4 0x55907c43fffb in RunUnittests suricata/src/runmode-unittests.c:286 #5 0x55907bbd6460 in StartInternalRunMode suricata/src/suricata.c:2335 #6 0x55907bbd8fc6 in SuricataMain suricata/src/suricata.c:2901 #7 0x55907bbc98eb in main suricata/src/main.c:22 #8 0x7f3b31a29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #9 0x7f3b31a29e3f in __libc_start_main_impl ../csu/libc-start.c:392 #10 0x55907bbc97f4 in _start (suricata/src/.libs/suricata+0x3107f4)
Updated by Philippe Antoine 10 months ago
Looks like the third call to DecodeBase64Block
has the right bounds check
if (dest_size - *decoded_bytes < 3) return BASE64_ECODE_BUF;
Updated by Philippe Antoine 10 months ago
- Status changed from New to In Review
- Label Needs backport to 7.0 added
Gitlab MR
Updated by Philippe Antoine 10 months ago
Assessment :
can be reached from:
- datasets loading
- from base64_decode
but only if all the rules using this keyword use bytes option with values 1, 2 or 5 !
- from smtp/mime traffic, but as it can only overwrite 3 bytes of data (and only with value 0), that is only md5_ctx
pointer that can get changed, so it affects only if you have the non-default app-layer.protocols.smtp.mime.body-md5
config option set to yes
So, I would say severity is high
Updated by Shivani Bhardwaj 10 months ago
- Assignee changed from Shivani Bhardwaj to Philippe Antoine
- Target version changed from TBD to 8.0.0-beta1
Updated by Philippe Antoine 10 months ago
Just some quick note about why this was not found by fuzzing :
- there is no fuzz targets for datasets loading...
- keyword base64_decode
does not look fuzzing-friendly as it is not stateless : the size of the buffer is a global maxed one, instead of one per rule
- hard to reach from SMTP, because you need exactly 3072 (big number) bytes of base64 decoded without problem, then your evil pattern
Updated by Philippe Antoine 10 months ago
Found by oss-fuzz as
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67847
Updated by Victor Julien 9 months ago
- Status changed from In Review to Closed
- Git IDs updated (diff)
Updated by Victor Julien 9 months ago
- Private changed from Yes to No