diff --git a/libyara/re.c b/libyara/re.c index 6174af1478..d161acf031 100644 --- a/libyara/re.c +++ b/libyara/re.c @@ -1573,6 +1573,16 @@ static int _yr_re_fiber_sync( case RE_OPCODE_REPEAT_END_UNGREEDY: repeat_args = (RE_REPEAT_ARGS*) (fiber->ip + 1); + +#if YR_PARANOID_EXEC + // A REPEAT_END with no matching REPEAT_START leaves sp at -1 and the + // access below would read and write before the stack. In normal + // conditions this never happens, but it can with compiled rules that + // have been hand-crafted by a malicious actor. + if (fiber->sp < 0) + return ERROR_INTERNAL_FATAL_ERROR; +#endif + fiber->stack[fiber->sp]++; if (fiber->stack[fiber->sp] < repeat_args->min) diff --git a/tests/test-re-split.c b/tests/test-re-split.c index 0f49f86715..52fe94acba 100644 --- a/tests/test-re-split.c +++ b/tests/test-re-split.c @@ -149,6 +149,58 @@ static void test_repeat_stack_overflow(void) } } +// A compiled rule hand-crafted by a malicious actor can carry a regexp whose +// bytecode reaches a REPEAT_END with no matching REPEAT_START. The compiler +// always emits a REPEAT_START (which pushes onto the fiber's repeat stack) +// before the matching REPEAT_END, so the stack pointer is never negative for +// legitimate rules, but loaded bytecode is not validated. _yr_re_fiber_sync +// increments stack[sp] on REPEAT_END, so such a stream must be rejected +// instead of writing before the stack with sp still -1. +static void test_repeat_stack_underflow(void) +{ + // A single REPEAT_END (opcode byte followed by RE_REPEAT_ARGS, a packed + // { uint16 min; uint16 max; int32 offset } of 8 bytes) with no preceding + // REPEAT_START, followed by a MATCH. + uint8_t code[1 + 8 + 1]; + uint16_t min = 1; + uint16_t max = 2; + int32_t offset = 0; + + code[0] = RE_OPCODE_REPEAT_END_GREEDY; + memcpy(code + 1, &min, sizeof(min)); + memcpy(code + 3, &max, sizeof(max)); + memcpy(code + 5, &offset, sizeof(offset)); + code[9] = RE_OPCODE_MATCH; + + YR_SCAN_CONTEXT context; + memset(&context, 0, sizeof(context)); + + uint8_t input[1] = {0}; + int matches = 0; + + int result = yr_re_exec( + &context, + code, + input, + sizeof(input), + 0, + RE_FLAGS_SCAN, + re_match_callback, + NULL, + &matches); + + assert(result == ERROR_INTERNAL_FATAL_ERROR); + + RE_FIBER* fiber = context.re_fiber_pool.fibers.head; + + while (fiber != NULL) + { + RE_FIBER* next = fiber->next; + yr_free(fiber); + fiber = next; + } +} + int main(int argc, char** argv) { int result = 0; @@ -201,6 +253,7 @@ int main(int argc, char** argv) test_split_id_overflow(); test_repeat_stack_overflow(); + test_repeat_stack_underflow(); yr_finalize();