Synchronise modules (issue #46) #51

TadeasKucera · 2020-01-08T16:01:38Z

Synchronise symbols with yara downstream and enable use of symbols from yara upstream only:

We have compared current yaramod modules with modules in Avast yara downstream and we make it consistent in this PR.
We also change yaramod::ParserDriver to be able to avoid Avast-specific or VirusTotal-specific symbols when importing. Hence Add switch for turning on/off Avast and VirusTotal specific symbols #46 Is solved. This will be benefitial for people outside Avast that need only symbols present in yara upstream.

…-specific symbols

…importing a module

(void) cast unused variables

metthal · 2020-01-15T09:28:26Z

include/yaramod/parser/parser_driver.h

-  	ParserDriver(ParserMode parserMode);
-	explicit ParserDriver(const std::string& filePath, ParserMode parserMode = ParserMode::Regular);
-	explicit ParserDriver(std::istream& input, ParserMode parserMode = ParserMode::Regular);
+  	ParserDriver(ParserMode parserMode, bool avastSpecific = true, bool vtSpecific = true);


It would be best to avoid just boolean parameters if it's unclear from the context of the function name of what they suppose to mean. Calls to such functions/methods then look like this ParserDriver(..., true, false). What exactly those true and false mean? The best way to do it would be through bitmasks because then you can do something like ParserDriver(..., Features::VirusTotal | Features::Avast).

OK, I implemented this in the last commit.

metthal · 2020-01-15T09:30:54Z

src/types/modules/module.cpp

 {
+	if (!avastSpecific && (name == "androguard" || name == "phish")) // the androguard and phish modules are completely avast-specific


I would like this to be an attribute of the module itself, like which features you require in order to load? This feels like a hack because information about a module is taken out from it.

Understood, instead of specifying here names of unsupported modules we get this information out of the module itself. Implemented here.

src/types/modules/time_module.cpp

metthal · 2020-01-15T09:35:47Z

tests/cpp/parser_tests.cpp

+}
+)");
+
+	Module::reset("androguard");


Will user need to call Module::reset() manually if they want to create more instances of yaramod? That doesn't sound very user friendly. This needs to happen all under the hood without user being aware of that. Please, change it so that user won't need to do this.

The user does not need to call the reset method, we only need it in our tests where we construct more ParserDriver instances - all sharing the same modules, because modules are stored as global variables in module.cpp. These modules need to be reseted if we need ParserDriver with different Features. This is something that the user will never need, because they will use the same Features among their ParserDrivers. Maybe we could stop storing modules as global variables but store them either inside Yaramod or inside ParserDriver classes.

But what if I want to create Yaramod instance once with VirusTotal features and once with no additional features enabled? Would I need to do that?

No, that would not require you calling the reset method. However, if you decided to use two different Yaramod instances, first one with ImportFeature::Avast and then second one with ImportFeature::Basic then yes, you would have to call the static method Module::reset(<module_name>). The problem is, that both ParserDriver instances share the same modules. In my opinien, it is a good thing that the modules are shared between the instances. The whole mean of issue #46 was to reduce the number of symbols stored and the complexity of modules. Therefore I propose a solution which would keep it:

Along with the globally stored modules we will store also global Feature instance named modules_state. Whenever new instance of Yaramod with parameter ImportFeature feature is created, we look at modules_state and update it:
modules_state |= feature
possibly enabling new modules/symbols. With this approach, when loading a module, we will only look at the global variable modules_state.

What do you think?

However, if you decided to use two different Yaramod instances, first one with ImportFeature::Avast and then second one with ImportFeature::Basic then yes, you would have to call the static method Module::reset(<module_name>).

This is what I meant and I don't personally like it because it is so easy to misuse from user standpoint.

In my opinien, it is a good thing that the modules are shared between the instances.

Why do you think it is a good idea to keep the modules shared? I personally think that it was good idea when modules were immutable but I don't see any benefit when modules are possibly mutable.

I would propose to move from global modules to modules that are only local to certain ParserDriver instance. All the modules will have to be initialized multiple times when creating multiple Yaramod instances but I think that benefits of this solution outweigh benefits of having global modules.

I thought about it and I agree with you. We will get rid of global variables, we will get rid of the reset function or some hacks I proposed. It will be much cleaner and use just a liitle bit more memory to store the modules for each instance independently. I will implement it soon. Thank you for this discussion.

Done: In commit we use new class ModulesPool which stores the modules. Each ParserDriver or YaraFileBuilder instance is assigned its own instance of ModulesPool class.

…or YaraFileBuilder independently.

metthal · 2020-01-16T12:00:14Z

Thank you very much for the changes. Let's wait for the builds to finish and we'll merge it.

TadeasKucera · 2020-01-16T13:27:49Z

Thank you for your suggestions! I believe we are ready for merge.

TadeasKucera force-pushed the update_modules_from_downstream branch from 9c4ed35 to 1f64914 Compare January 9, 2020 08:12

TadeasKucera changed the title ~~Synchronise modules~~ Synchronise modules (issue #46) Jan 13, 2020

TadeasKucera added 8 commits January 13, 2020 16:14

remove symbol signature.name for it has been deleted from downstream

d08f600

Add missing math.min and math.max symbols

7aa6065

Add NeededSymbols to module.h to delegate whether to include avast/tv…

8b3af84

…-specific symbols

Use NeededSymbols: ParserDriver now offers to skip some symbols when …

d21c1f3

…importing a module

Synchronise symbols in modules with yara upstream

f86ea36

(void) cast unused variables

Add module resetting and tests of avast-specific symbols imports

84046ba

Allow to skip VirusTotal-specific symbols (those in yara_file.cpp)

b5ef2d3

Improve tests of phish and androguard modules

ea31ce2

TadeasKucera force-pushed the update_modules_from_downstream branch from 66de6d0 to ea31ce2 Compare January 13, 2020 15:34

TadeasKucera marked this pull request as ready for review January 13, 2020 16:58

TadeasKucera assigned metthal Jan 13, 2020

TadeasKucera force-pushed the update_modules_from_downstream branch from 82ef8bc to ea31ce2 Compare January 15, 2020 08:27

metthal requested changes Jan 15, 2020

View reviewed changes

TadeasKucera added 2 commits January 16, 2020 08:40

Use enum Features instead of bool avastSpecific and vtSpecific

591e849

Instead of having global modules, store modules in each ParserDriver …

7afaecc

…or YaraFileBuilder independently.

TadeasKucera force-pushed the update_modules_from_downstream branch from ef2a4f3 to 7afaecc Compare January 16, 2020 11:34

metthal approved these changes Jan 16, 2020

View reviewed changes

Fix Syntax and Typos

7df3108

metthal merged commit 6c9ba54 into master Jan 16, 2020

metthal deleted the update_modules_from_downstream branch January 16, 2020 14:32

metthal added a commit that referenced this pull request Jan 17, 2020

Added changelog entries for #51, #52, #53

75bee39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Synchronise modules (issue #46) #51

Synchronise modules (issue #46) #51

Uh oh!

TadeasKucera commented Jan 8, 2020 •

edited

Loading

Uh oh!

metthal Jan 15, 2020

Uh oh!

TadeasKucera Jan 16, 2020

Uh oh!

metthal Jan 15, 2020

Uh oh!

TadeasKucera Jan 16, 2020

Uh oh!

Uh oh!

metthal Jan 15, 2020

Uh oh!

TadeasKucera Jan 15, 2020

Uh oh!

metthal Jan 15, 2020

Uh oh!

TadeasKucera Jan 15, 2020

Uh oh!

metthal Jan 15, 2020

Uh oh!

metthal Jan 15, 2020

Uh oh!

TadeasKucera Jan 16, 2020

Uh oh!

TadeasKucera Jan 16, 2020

Uh oh!

metthal commented Jan 16, 2020

Uh oh!

TadeasKucera commented Jan 16, 2020

Uh oh!

Uh oh!

		{
		if (!avastSpecific && (name == "androguard" \|\| name == "phish")) // the androguard and phish modules are completely avast-specific

Synchronise modules (issue #46) #51

Synchronise modules (issue #46) #51

Uh oh!

Conversation

TadeasKucera commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metthal commented Jan 16, 2020

Uh oh!

TadeasKucera commented Jan 16, 2020

Uh oh!

Uh oh!

TadeasKucera commented Jan 8, 2020 •

edited

Loading