Fix bug that broke global rules.

PR #1927 didn't take into account that as a side-effect of skipping the evaluation of global rules, the corresponding namespace was not flagged as unsatisfied (i.e: a global rule that evaluates to false prevents all other rules in the namespace from evaluating to true). The issue was not caught by any test case because the test case for global rules were using rules that didn't required any strings to match, and therefore were unaffected by the changes introduced in #1927.

This change makes sure that any global rule that is skipped is considered false, and therefore its namespace is flagged as unsatisfied.

Closes #2022.

libyara/exec.c

@@ -1166,14 +1166,28 @@ int yr_execute_code(YR_SCAN_CONTEXT* context)
 
       current_rule = &context->rules->rules_table[current_rule_idx];
 
-      // If the rule is disabled let's skip its code.
-      bool disabled = RULE_IS_DISABLED(current_rule) || yr_bitmask_is_not_set(context->rule_evaluate_condition_flags, current_rule_idx);
-      ip = jmp_if(disabled, ip);
+      // If the rule is disabled, let's skip its code.
+      bool skip_rule = RULE_IS_DISABLED(current_rule);
 
-      // Skip the bytes corresponding to the rule's index, but only if not
-      // taking the jump.
-      if (!disabled)
+      // The rule is also skipped if it is not required to be evaluated.
+      skip_rule |= yr_bitmask_is_not_set(
+          context->required_eval, current_rule_idx);
+
+      ip = jmp_if(skip_rule, ip);
+
+      if (skip_rule)
+      {
+        // If the rule is skipped it is false, and if a global rule is false
+        // we must mark its namespace as unsatisfied.
+        if (RULE_IS_GLOBAL(current_rule))
+          yr_bitmask_set(context->ns_unsatisfied_flags, current_rule->ns->idx);
+      }
+      else
+      {
+        // If not taking the jump, skip the bytes corresponding to the
+        // rule's index.
         ip += sizeof(uint32_t);
+      }
 
       break;
 

libyara/include/yara/types.h

@@ -615,7 +615,7 @@ struct YR_RULES
 
   // A bitmap with one bit per rule, bit N is set when the condition for rule
   // might evaluate to true even without any string matches.
-  YR_BITMASK* rule_evaluate_condition_flags;
+  YR_BITMASK* no_required_strings;
 
   // Total number of rules.
   uint32_t num_rules;
@@ -823,9 +823,9 @@ struct YR_SCAN_CONTEXT
   // until they can be confirmed or discarded.
   YR_MATCHES* unconfirmed_matches;
 
-  // A bitmap with one bit per rule, bit N is unset when the condition for rule
-  // with index N is guaranteed to evaluate to false.
-  YR_BITMASK* rule_evaluate_condition_flags;
+  // A bitmap with one bit per rule, bit N is set if the corresponding rule
+  // must evaluated.
+  YR_BITMASK* required_eval;
 
   // profiling_info is a pointer to an array of YR_PROFILING_INFO structures,
   // one per rule. Entry N has the profiling information for rule with index N.

libyara/rules.c

@@ -336,9 +336,10 @@ int yr_rules_from_arena(YR_ARENA* arena, YR_RULES** rules)
   if (new_rules == NULL)
     return ERROR_INSUFFICIENT_MEMORY;
 
-  new_rules->rule_evaluate_condition_flags = (YR_BITMASK*) yr_calloc(
+  new_rules->no_required_strings = (YR_BITMASK*) yr_calloc(
       sizeof(YR_BITMASK), YR_BITMASK_SIZE(summary->num_rules));
-  if (new_rules->rule_evaluate_condition_flags == NULL)
+
+  if (new_rules->no_required_strings == NULL)
   {
     yr_free(new_rules);
     return ERROR_INSUFFICIENT_MEMORY;
@@ -378,9 +379,7 @@ int yr_rules_from_arena(YR_ARENA* arena, YR_RULES** rules)
   for (int i = 0; i < new_rules->num_rules; i++)
   {
     if (new_rules->rules_table[i].required_strings == 0)
-    {
-      yr_bitmask_set(new_rules->rule_evaluate_condition_flags, i);
-    }
+      yr_bitmask_set(new_rules->no_required_strings, i);
   }
 
   *rules = new_rules;
@@ -543,8 +542,7 @@ YR_API int yr_rules_destroy(YR_RULES* rules)
     external++;
   }
 
-  yr_free(rules->rule_evaluate_condition_flags);
-
+  yr_free(rules->no_required_strings);
   yr_arena_release(rules->arena);
   yr_free(rules);
 

libyara/scan.c

@@ -558,8 +558,8 @@ static int _yr_scan_verify_chained_string_match(
           _yr_scan_remove_match_from_list(
               match, &context->unconfirmed_matches[string->idx]);
 
-          match->match_length =
-              (int32_t) (match_offset - match->offset + match_length);
+          match->match_length = (int32_t) (match_offset - match->offset +
+                                           match_length);
 
           match->data_length = yr_min(
               match->match_length, (int32_t) max_match_data);
@@ -575,8 +575,9 @@ static int _yr_scan_verify_chained_string_match(
               match_data - match_offset + match->offset,
               match->data_length);
 
-          yr_bitmask_set(
-              context->rule_evaluate_condition_flags, string->rule_idx);
+          // Once a string is found, the rule containing that string is
+          // required to be evaluated.
+          yr_bitmask_set(context->required_eval, string->rule_idx);
 
           FAIL_ON_ERROR(_yr_scan_add_match_to_list(
               match, &context->matches[string->idx], false));
@@ -753,9 +754,7 @@ static int _yr_scan_match_callback(
       new_match->is_private = STRING_IS_PRIVATE(string);
       new_match->xor_key = callback_args->xor_key;
 
-      yr_bitmask_set(
-          callback_args->context->rule_evaluate_condition_flags,
-          string->rule_idx);
+      yr_bitmask_set(callback_args->context->required_eval, string->rule_idx);
 
       FAIL_ON_ERROR(_yr_scan_add_match_to_list(
           new_match,

libyara/scanner.c

@@ -172,17 +172,16 @@ static int _yr_scanner_scan_mem_block(
     }
   }
 
-  if (rule != NULL &&
-      scanner->matches->count >= YR_SLOW_STRING_MATCHES &&
+  if (rule != NULL && scanner->matches->count >= YR_SLOW_STRING_MATCHES &&
       scanner->matches->count < YR_MAX_STRING_MATCHES)
   {
     if (rule != NULL && report_string != NULL)
     {
       result = scanner->callback(
-        scanner,
-        CALLBACK_MSG_TOO_SLOW_SCANNING,
-        (void*) report_string,
-        scanner->user_data);
+          scanner,
+          CALLBACK_MSG_TOO_SLOW_SCANNING,
+          (void*) report_string,
+          scanner->user_data);
       if (result != CALLBACK_CONTINUE)
       {
         result = ERROR_TOO_SLOW_SCANNING;
@@ -214,7 +213,7 @@ static void _yr_scanner_clean_matches(YR_SCANNER* scanner)
       sizeof(YR_BITMASK) * YR_BITMASK_SIZE(scanner->rules->num_rules));
 
   memset(
-      scanner->rule_evaluate_condition_flags,
+      scanner->required_eval,
       0,
       sizeof(YR_BITMASK) * YR_BITMASK_SIZE(scanner->rules->num_rules));
 
@@ -264,7 +263,7 @@ YR_API int yr_scanner_create(YR_RULES* rules, YR_SCANNER** scanner)
   new_scanner->rule_matches_flags = (YR_BITMASK*) yr_calloc(
       sizeof(YR_BITMASK), YR_BITMASK_SIZE(rules->num_rules));
 
-  new_scanner->rule_evaluate_condition_flags = (YR_BITMASK*) yr_calloc(
+  new_scanner->required_eval = (YR_BITMASK*) yr_calloc(
       sizeof(YR_BITMASK), YR_BITMASK_SIZE(rules->num_rules));
 
   new_scanner->ns_unsatisfied_flags = (YR_BITMASK*) yr_calloc(
@@ -280,7 +279,7 @@ YR_API int yr_scanner_create(YR_RULES* rules, YR_SCANNER** scanner)
       rules->num_strings, sizeof(YR_MATCHES));
 
   if (new_scanner->rule_matches_flags == NULL ||
-      new_scanner->rule_evaluate_condition_flags == NULL ||
+      new_scanner->required_eval == NULL ||
       new_scanner->ns_unsatisfied_flags == NULL ||
       new_scanner->strings_temp_disabled == NULL ||
       (new_scanner->matches == NULL && rules->num_strings > 0) ||
@@ -367,7 +366,7 @@ YR_API void yr_scanner_destroy(YR_SCANNER* scanner)
 
   yr_free(scanner->rule_matches_flags);
   yr_free(scanner->ns_unsatisfied_flags);
-  yr_free(scanner->rule_evaluate_condition_flags);
+  yr_free(scanner->required_eval);
   yr_free(scanner->strings_temp_disabled);
   yr_free(scanner->matches);
   yr_free(scanner->unconfirmed_matches);
@@ -508,10 +507,12 @@ YR_API int yr_scanner_scan_mem_blocks(
     if (result != ERROR_SUCCESS)
       goto _exit;
 
+    // Every rule that doesn't require a matching string must be evaluated
+    // regardless of whether a string matched or not.
     memcpy(
-      scanner->rule_evaluate_condition_flags,
-      scanner->rules->rule_evaluate_condition_flags,
-      sizeof(YR_BITMASK) * YR_BITMASK_SIZE(rules->num_rules));
+        scanner->required_eval,
+        scanner->rules->no_required_strings,
+        sizeof(YR_BITMASK) * YR_BITMASK_SIZE(rules->num_rules));
 
     yr_stopwatch_start(&scanner->stopwatch);
 
@@ -690,8 +691,9 @@ YR_API const uint8_t* yr_fetch_block_data(YR_MEMORY_BLOCK* block)
   {
     return NULL;
   }
-  jumpinfo* info = (jumpinfo*) yr_thread_storage_get_value(&yr_trycatch_trampoline_tls);
-  if (info == NULL) // Not called from YR_TRYCATCH
+  jumpinfo* info = (jumpinfo*) yr_thread_storage_get_value(
+      &yr_trycatch_trampoline_tls);
+  if (info == NULL)  // Not called from YR_TRYCATCH
   {
     return data;
   }
@@ -728,15 +730,18 @@ YR_API int yr_scanner_scan_mem(
   iterator.file_size = _yr_get_file_size;
   iterator.last_error = ERROR_SUCCESS;
 
-  // Detect cases where every byte of input is checked for match and input size is bigger then 0.2 MB
-  if (scanner->rules->ac_match_table[YR_AC_ROOT_STATE] != 0 && buffer_size > YR_FILE_SIZE_THRESHOLD)
+  // Detect cases where every byte of input is checked for match and input size
+  // is bigger then 0.2 MB
+  if (scanner->rules->ac_match_table[YR_AC_ROOT_STATE] != 0 &&
+      buffer_size > YR_FILE_SIZE_THRESHOLD)
   {
-    YR_STRING* report_string = scanner->rules->ac_match_pool[YR_AC_ROOT_STATE].string;
+    YR_STRING* report_string =
+        scanner->rules->ac_match_pool[YR_AC_ROOT_STATE].string;
     result = scanner->callback(
-          scanner,
-          CALLBACK_MSG_TOO_SLOW_SCANNING,
-          (void*) report_string,
-          scanner->user_data);
+        scanner,
+        CALLBACK_MSG_TOO_SLOW_SCANNING,
+        (void*) report_string,
+        scanner->user_data);
     if (result != CALLBACK_CONTINUE)
       return ERROR_TOO_SLOW_SCANNING;
   }

tests/test-rules.c

@@ -2980,6 +2980,36 @@ static void test_global_rules()
       }",
       NULL);
 
+  assert_false_rule(
+      "global private rule global_rule { \
+        strings: \
+          $a = \"foo\" \
+        condition: \
+          $a \
+      } \
+      rule test { \
+       strings: \
+          $a = \"bar\" \
+        condition: \
+          $a \
+      }",
+      "bar");
+
+  assert_true_rule(
+      "global private rule global_rule { \
+        strings: \
+          $a = \"foo\" \
+        condition: \
+          $a \
+      } \
+      rule test { \
+       strings: \
+          $a = \"bar\" \
+        condition: \
+          $a \
+      }",
+      "foobar");
+
   YR_DEBUG_FPRINTF(1, stderr, "} // %s()\n", __FUNCTION__);
 }