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 8 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 8 months ago
- Status changed from New to In Review
- Label Needs backport to 7.0 added
Gitlab MR
Updated by Philippe Antoine 8 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 8 months ago
- Assignee changed from Shivani Bhardwaj to Philippe Antoine
- Target version changed from TBD to 8.0.0-beta1
Updated by Philippe Antoine 8 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 8 months ago
Found by oss-fuzz as
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67847
Updated by Victor Julien 7 months ago
- Status changed from In Review to Closed
- Git IDs updated (diff)
Updated by Victor Julien 7 months ago
- Private changed from Yes to No