Allow to add a description for every feature #39

Ana06 · 2020-06-30T12:44:33Z

Enable associate context for all features. This was called symbol before and only enabled for number, offset and bytes. Example:

  features:
    - and:
      - or:
        - api: kernel32.LoadLibrary = I am an inline description
        - api: kernel32.GetModuleHandle
        - api: kernel32.GetModuleHandleEx
        - api: ntdll.LdrLoadDll
      - or:
        - api: kernel32.GetProcAddress
        - api: ntdll.LdrGetProcedureAddress
      - optional:
        - characteristic(indirect call): true
		  description: I am a description

Inline syntax is not supported for strings.

This is not enabled for strings with regular expressions, as they are not a feature. It is needed to either make RegExp a feature or implement this for RegExp as well. I haven't implement this yet because I have some things to discuss about this implementation. I'll add it once is this is clarified (it can be as part of this PR or a different one).

Closes #5

capa/features/__init__.py

Ana06 · 2020-06-30T13:03:38Z

I am thinking that it would be a good idea to use a different color for all the addresses. This would also help making clearer which the comment is.

For example:

instead of:

@williballenthin @mr-tz @mike-hunhoff what do you think? 🤔

mr-tz

nice, can you please add tests showing all features support descriptions now as expected

README.md

capa/features/__init__.py

capa/render/__init__.py

capa/rules.py

capa/features/__init__.py

capa/render/vverbose.py

capa/rules.py

capa/features/__init__.py

Ana06 · 2020-06-30T16:32:39Z

@mr-tz

nice, can you please add tests showing all features support descriptions now as expected

That's a great idea! 👍 However I am not sure how to test it. I could write a tests in test_rules.py checking that the evaluation of the rule still works if there are descriptions, but that is not really testing that the descriptions work. I think we do not have tests to test the json output, do we? Because that would be a good place to test that the description is included. 🤔 Any idea how/where to write tests for this? 🙏

capa/features/insn.py

Ana06 · 2020-07-01T13:07:39Z

With this changes the json for characteristic looks like:
{"feature": {"characteristic": "indirect call(True)", "type": "characteristic"}, "type": "feature"}
Is this ok? It makes sense to me. It is a string in all other features, so I think having an array only for this case, just complicate using capa.

And the output with the -vv option:
characteristic(indirect call(True))

I would however change it to
characteristic: indirect call(True)

for consistency with other features. For example:
api: kernel32.TerminateProcess

README.md

capa/rules.py

mr-tz

muchas gracias!

mr-tz · 2020-07-01T14:47:57Z

@mr-tz

nice, can you please add tests showing all features support descriptions now as expected

That's a great idea! 👍 However I am not sure how to test it. I could write a tests in test_rules.py checking that the evaluation of the rule still works if there are descriptions, but that is not really testing that the descriptions work. I think we do not have tests to test the json output, do we? Because that would be a good place to test that the description is included. 🤔 Any idea how/where to write tests for this? 🙏

For now testing if the parsing for all features works should be good. This could be done in test_rules.py

mr-tz · 2020-07-01T14:56:06Z

With this changes the json for characteristic looks like:
{"feature": {"characteristic": "indirect call(True)", "type": "characteristic"}, "type": "feature"}
Is this ok? It makes sense to me. It is a string in all other features, so I think having an array only for this case, just complicate using capa.

And the output with the -vv option:
characteristic(indirect call(True))

I would however change it to
characteristic: indirect call(True)

for consistency with other features. For example:
api: kernel32.TerminateProcess

Yes, I like the consistent format for the JSON. Didn't we try to remove the True part before when displaying? May be related to #39 (comment)?

Ana06 · 2020-07-01T15:04:15Z

@mr-tz

Didn't we try to remove the True part before when displaying? May be related to #39 (comment)?

It was actually removed from the -vv option, but I understood that @williballenthin wanted back from this comment:
#39 (comment)

But I think I got it wrong. Maybe he meant the json... but shouldn't we remove it from the json if it is only used internally?

williballenthin · 2020-07-01T16:09:32Z

for -vv format, i think the best case is characteristic(indirect call) because it mirrors what would be written in the rule (actually its characteristic(indirect call): True but i think we decided True is currently redundant).

i think this one is ok: characteristic: indirect call and i do like how it mirrors other features (sidebar: should we port this syntax to rules? it relies on the assumption that characteristics are always True, which has been consistent recently).

in the json, i'd like to see the True separated from the indirect call. this is so that a renderer can decide how to format the value, without parsing the string and looking for ( characters. as discussed above, we can maybe even remove the True all together, though keeping it around is a little more explicit.

does this make sense? is it reasonable?

mr-tz

characteristic changes almost identical to #63! see comments, not sure how to best deal with this race-condition 😄

capa/features/__init__.py

capa/rules.py

Enable associate context for all features. This was called symbol before and only enabled for `number`, `offset` and `bytes`. This is not enabled for strings with regular expressions, as they are not a feature.

Ana06 · 2020-07-02T12:24:37Z

I finished the deletion of value for Characteristic. I took the freeze_serialize function from @williballenthin's PR and added @williballenthin as co-author.

I have also added support for inline descriptions in count and tests

capa/features/extractors/__init__.py

Ana06 · 2020-07-02T12:40:12Z

There are still a few things missing related to this PR, but I would prefer to merge this first and open issues/PRs for them afterwards:

Supporting descriptions for RegExp. This is not done yet as they are not a feature. It is needed to either make RegExp a feature or implement this for RegExp as well.
Use a different color for all the addresses. This would also help making clearer which the comment is.
rather than using an inline string ' = ' that is prone to typo and cannot be used with "find references", use a constant like DESCRIPTION_SEPARATOR = "' = "' and use this throughout the code. (from @williballenthin's comment)

This PR is modifying/refactoring core parts of code and the merge of almost any other PR causes merge conflicts.

capa/features/__init__.py

README.md

tests/test_rules.py

mr-tz

see my comments, otherwise looks good to me

capa/features/__init__.py

capa/features/insn.py

As the `__str__` method is not used anymore in the output, the description implementation needs to be adapted.

Get rid of `True` in characteristic (rules, output and json) as it is implicit. This way, the same syntax is used for characteristic as for the rest of the features. Co-authored-by: William Ballenthin <william.ballenthin@fireeye.com>

``` count(number(2 = AF_INET/SOCK_DGRAM)): 2 ```

Test if the parsing of feature succeeds with every time of description.

williballenthin · 2020-07-02T15:11:06Z

🪂

Ana06 · 2020-07-02T15:19:32Z

💃

Ana06 added the enhancement New feature or request label Jun 30, 2020

Ana06 requested review from williballenthin and mr-tz June 30, 2020 12:44

Ana06 commented Jun 30, 2020

View reviewed changes

capa/features/__init__.py Show resolved Hide resolved

Ana06 requested a review from mike-hunhoff June 30, 2020 12:52

mr-tz reviewed Jun 30, 2020

View reviewed changes