From b7da5d2835cc4cf8a15027da30265addab1a4be5 Mon Sep 17 00:00:00 2001 From: "Victor M. Alvarez" Date: Thu, 26 Mar 2020 13:50:49 +0100 Subject: [PATCH] Fix issue #1180. This changes the way in which comparison operators, and also the "contains" and "matches" operators handle undefined operands. Before this change the result of such operations was undefined, after this change the result is false. --- docs/writingrules.rst | 23 ++++-- libyara/exec.c | 169 +++++++++++++++++++++++------------------- tests/test-rules.c | 22 +++++- 3 files changed, 129 insertions(+), 85 deletions(-) diff --git a/docs/writingrules.rst b/docs/writingrules.rst index f1a95676..ca46de2e 100644 --- a/docs/writingrules.rst +++ b/docs/writingrules.rst @@ -1405,7 +1405,7 @@ Undefined values Modules often leave variables in an undefined state, for example when the variable doesn't make sense in the current context (think of ``pe.entry_point`` -while scanning a non-PE file). YARA handles undefined values in way that allows +while scanning a non-PE file). YARA handles undefined values in a way that allows the rule to keep its meaningfulness. Take a look at this rule: .. code-block:: yara @@ -1431,10 +1431,23 @@ if the condition is changed to: $a or pe.entry_point == 0x1000 You would expect the rule to match in this case if the file contains the string, -even if it isn't a PE file. That's exactly how YARA behaves. The logic is -simple: any arithmetic, comparison, or boolean operation will result in an -undefined value if one of its operands is undefined, except for *OR* operations -where an undefined operand is interpreted as a False. +even if it isn't a PE file. That's exactly how YARA behaves. The logic is as +follows: + +* Arithmetic and bitwise operators return a undefined value if some of its + operands is undefined. + +* Boolean operators `and` and `or` will treat undefined operands as `false`. + +* Boolean `not` operator returns false if the operand is undefined. + +* Comparison operators and any other operator whose result is a boolean (like + the ``contains`` and ``matches`` operators) will return `false` if any of its + operands is undefined. + +In the expression above `pe.entry_point == 0x1000` will be false, because +`pe.entry_point` is undefined, and the `==` operator returns false if any of its +operand is undefined. External variables diff --git a/libyara/exec.c b/libyara/exec.c index 5b46d8c8..431461d8 100644 --- a/libyara/exec.c +++ b/libyara/exec.c @@ -1302,11 +1302,16 @@ int yr_execute_code( pop(r2); pop(r1); - ensure_defined(r1); - ensure_defined(r2); + if (is_undef(r1) || is_undef(r2)) + { + r1.i = false; + } + else + { + r1.i = memmem(r1.ss->c_string, r1.ss->length, + r2.ss->c_string, r2.ss->length) != NULL; + } - r1.i = memmem(r1.ss->c_string, r1.ss->length, - r2.ss->c_string, r2.ss->length) != NULL; push(r1); break; @@ -1322,14 +1327,10 @@ int yr_execute_code( break; case OP_MATCHES: - pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - - if (r1.ss->length == 0) + if (is_undef(r1) || is_undef(r2) || r1.ss->length == 0) { r1.i = false; push(r1); @@ -1337,15 +1338,15 @@ int yr_execute_code( } result = yr_re_exec( - context, - (uint8_t*) r2.re->code, - (uint8_t*) r1.ss->c_string, - r1.ss->length, - 0, - r2.re->flags | RE_FLAGS_SCAN, - NULL, - NULL, - &found); + context, + (uint8_t*) r2.re->code, + (uint8_t*) r1.ss->c_string, + r1.ss->length, + 0, + r2.re->flags | RE_FLAGS_SCAN, + NULL, + NULL, + &found); if (result != ERROR_SUCCESS) stop = true; @@ -1386,54 +1387,60 @@ int yr_execute_code( case OP_INT_EQ: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.i == r2.i; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.i == r2.i; push(r1); break; case OP_INT_NEQ: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.i != r2.i; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.i != r2.i; push(r1); break; case OP_INT_LT: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.i < r2.i; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.i < r2.i; push(r1); break; case OP_INT_GT: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.i > r2.i; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.i > r2.i; push(r1); break; case OP_INT_LE: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.i <= r2.i; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.i <= r2.i; push(r1); break; case OP_INT_GE: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.i >= r2.i; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.i >= r2.i; push(r1); break; @@ -1486,54 +1493,60 @@ int yr_execute_code( case OP_DBL_LT: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.d < r2.d; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.d < r2.d; push(r1); break; case OP_DBL_GT: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.d > r2.d; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.d > r2.d; push(r1); break; case OP_DBL_LE: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.d <= r2.d; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.d <= r2.d; push(r1); break; case OP_DBL_GE: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = r1.d >= r2.d; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = r1.d >= r2.d; push(r1); break; case OP_DBL_EQ: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = fabs(r1.d - r2.d) < DBL_EPSILON; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = fabs(r1.d - r2.d) < DBL_EPSILON; push(r1); break; case OP_DBL_NEQ: pop(r2); pop(r1); - ensure_defined(r2); - ensure_defined(r1); - r1.i = fabs(r1.d - r2.d) >= DBL_EPSILON; + if (is_undef(r1) || is_undef(r2)) + r1.i = false; + else + r1.i = fabs(r1.d - r2.d) >= DBL_EPSILON; push(r1); break; @@ -1590,29 +1603,33 @@ int yr_execute_code( pop(r2); pop(r1); - ensure_defined(r1); - ensure_defined(r2); - - switch(opcode) + if (is_undef(r1) || is_undef(r2)) { - case OP_STR_EQ: - r1.i = (sized_string_cmp(r1.ss, r2.ss) == 0); - break; - case OP_STR_NEQ: - r1.i = (sized_string_cmp(r1.ss, r2.ss) != 0); - break; - case OP_STR_LT: - r1.i = (sized_string_cmp(r1.ss, r2.ss) < 0); - break; - case OP_STR_LE: - r1.i = (sized_string_cmp(r1.ss, r2.ss) <= 0); - break; - case OP_STR_GT: - r1.i = (sized_string_cmp(r1.ss, r2.ss) > 0); - break; - case OP_STR_GE: - r1.i = (sized_string_cmp(r1.ss, r2.ss) >= 0); - break; + r1.i = false; + } + else + { + switch(opcode) + { + case OP_STR_EQ: + r1.i = (sized_string_cmp(r1.ss, r2.ss) == 0); + break; + case OP_STR_NEQ: + r1.i = (sized_string_cmp(r1.ss, r2.ss) != 0); + break; + case OP_STR_LT: + r1.i = (sized_string_cmp(r1.ss, r2.ss) < 0); + break; + case OP_STR_LE: + r1.i = (sized_string_cmp(r1.ss, r2.ss) <= 0); + break; + case OP_STR_GT: + r1.i = (sized_string_cmp(r1.ss, r2.ss) > 0); + break; + case OP_STR_GE: + r1.i = (sized_string_cmp(r1.ss, r2.ss) >= 0); + break; + } } push(r1); diff --git a/tests/test-rules.c b/tests/test-rules.c index ce6a8310..dabfa76a 100644 --- a/tests/test-rules.c +++ b/tests/test-rules.c @@ -1492,7 +1492,6 @@ void test_for() }", NULL); - assert_false_rule( "import \"tests\" \ rule test { \ @@ -1524,16 +1523,31 @@ void test_for() }", NULL); + // Test case for issue #1180. assert_true_rule( "import \"tests\" \ rule test { \ condition: \ - for 0 i in (1..3) : ( \ - false \ - ) \ + not tests.string_array[4] contains \"foo\" \ }", NULL); + // Test case for issue #1180. + assert_true_rule( + "import \"tests\" \ + rule test { \ + condition: \ + not tests.string_dict[\"undefined\"] matches /foo/ \ + }", + NULL); + + assert_false_rule( + "import \"tests\" \ + rule test { \ + condition: \ + not tests.undefined.i \ + }", + NULL); assert_true_rule( "import \"tests\" \