rebuild_yara_rule method does not correctly quote metadata fields about plyara HOT 9 CLOSED

Hi Tom. This is likely also related to #12 as the rebuilt rule comes from the parsed rule data. I'm not sure that is actually a blocker for this though. Will take a look.

from plyara.

This should be fixed in plyara v1.2.4 with @Taskr's changes (#18).

Thanks for the issue, and let us know if you find any more problems.

from plyara.

Hi,

Yep - this fixes it.

Thanks for the fast turn around!

Tom

from plyara.

Hey,

I found an edge case (possibly more than one) where the current fix will not correctly parse a rule, for example:

rule test
{
meta:
    field = "1"
    field = "2"
condition:
    false

}

Will fail on this check with a TypeError:

# Check for and handle correctly quoting string metadata
for k, v in rule['metadata'].items():
    try:
        if v in ('true', 'false') or int(v):
             pass
        except ValueError:
             v = '"{}"'.format(v)

An extra try/except would resolve the issue, like this:

except TypeError:
     if isinstance(v, list):
         v = '"{}"'.format(v)

Cheers,
Tom

from plyara.

Interesting... is a list what you'd expect these fields to be parsed as? This is what our dict looks like now:

    "metadata": {
        "field": [
            "1", 
            "2"
        ]
    }, 

Which means if all we do is v = '"{}"'.format(v), we'd end up with rebuild output like this:

field = "[1, 2]"

At the least we'd need to handle splitting that back into

field = 1
field = 2

But more than that, I'm not sure it makes sense to represent this as a list in the parsed dict object. What's the general use case for this? I haven't seen duplicate meta fields used before.

from plyara.

Hi,

Yea good point.

I have a large repository of rules from multiple contributors, some contributors have included a list of hashes in their rules, and define each hash in the metadata using:

hash = "$hash1"
hash = "$hash2"

That's how I came across this case.

I agree it makes more sense to split them back into each line as the original rule had them to stay in the spirit of rebuilding the original rule as it was.

Cheers,
Tom

from plyara.

Got it. In that case, it sounds like using a list internally is OK, since it's meant to be a list of reference hashes.

@Taskr do you want to handle this piece? Or I can probably get to it sometime this week.

from plyara.

@rshipp @tlansec New pull should fix the issue. I completely blanked on the list of same metadata entries. Thanks for the input @tlansec. Also added to test case to verify working correctly. Minimal changes to just the rebuild rule method so it won't impact any other part :)

from plyara.

Pushed as v1.2.5, let us know if you find anything else!

from plyara.