Neo23x0 / Raccine

A Simple Ransomware Vaccine
The Unlicense
942 stars 123 forks source link

Yara support now only reports the first matching rule. #59

Closed JohnLaTwC closed 3 years ago

JohnLaTwC commented 3 years ago

The first implementation of Yara support allowed multiple rule files to match against a command line. And the runyara.bat appended to the output:

@yara64.exe %1 %2 >> %2.out Thus when the matches were logged, it logged all YARA files and all matching rules.

This PR [1] exits after the first match, so no other rule matches are returned. I could see this being an issue if one had a yara file for suspicious_powershell_commands.yar and another for ryuk_ransomware.yar. With this change, you would only see the first yara rule match and potentially not realize it's a more serious infection. I think we should change to match against all rules.

See: https://github.com/Neo23x0/Raccine/blob/e558e063ccf157ffcdada9a1a464878f691167f1/source/RaccineLib/YaraRuleRunner.cpp#L19

    for (const std::filesystem::path& yara_rule : m_yara_rules) {
        if (run_yara_rule_on_file(yara_rule, target_file, command_line, out_yara_output)) {
            return true;  <<<<<<< exits after first match
        }
    }

[1] https://github.com/Neo23x0/Raccine/commit/4462922bb9297791914f21188739db404d3c7fbe#diff-9cace7ee141689932f0a4ad89e8581bdbb8d13d9522a9c7b20b5cc40657c637b

Neo23x0 commented 3 years ago

An even better way to handle this would be to concatenate all YARA rules before applying the rule set as a single file. I can do the followiung during the installation.

Instead of

COPY yara\*.yar "%ProgramData%\Raccine\yara"

I'll do

TYPE yara\*.yar >> "%ProgramData%\Raccine\yara\all-rules.yar"

This way we avoid too many loops over different rule sets and the mentioned issue above. But I think we should handle it nonetheless, since users could drop their own rules into that folder in expectation that they would trigger. Well, they do, but only if none of the rules in the all-rules.yar set didn't trigger before.

My solution bears the risk of duplicate rules that could lead to an error. I think this should be covered in some of the tests.

Neo23x0 commented 3 years ago

Fixed in branch yara-ext-vars https://github.com/Neo23x0/Raccine/blob/7a95739a52f78738b94d132e42627043b91dabcb/source/RaccineLib/YaraRuleRunner.cpp#L20

JohnLaTwC commented 3 years ago

Another potential gotcha with concatting all the rules is that it will need to be done everytime we sync the rules from github (which is TBD functionality). Also, if users drop rules in that folder, they'll need to know to concat all of them. This is why I think the change you did to revert back to always running all the rules is the simpler model. It adds a lot of process creation overhead with running yara multiple times but, that is temporary until we get a yaralib linked into raccine.

Neo23x0 commented 3 years ago

Yes, correct. I already reverted that change in the feature branch. Another problem was that with a single rule that uses external variables the whole set is unusable when yara64.exe gets invoked without the external variables set in the command line.