Implemented computation of export hashes #321

JakubPruzinec · 2018-06-03T17:40:35Z

impHashBytes relict removed
expHashes added in both, fileformat and fileinfo
plain/json getters for presentation modified accordingly

expHashes are calculated after the table gets sorted alphabetically. ordinals are not converted to names rather to form of "ord<ord_num>"

metthal

Code doesn't look bad but spaces need to be replaced with tabs and few return by values can be replaced with return by const reference.

This PR is however incomplete. It is missing some files, for example I don't see any changes in src/fileformat/types/export_table but there should be since header is changed. Presentation to JSON is also missing.

metthal · 2018-06-03T17:43:32Z

include/retdec/fileformat/file_format/file_format.h

@@ -116,6 +116,7 @@ class FileFormat : public retdec::utils::ByteValueStorage, private retdec::utils
 		void loadStrings(StringType type, std::size_t charSize);
 		void loadStrings(StringType type, std::size_t charSize, const SecSeg* secSeg);
 		void loadImpHash();
+        void loadExpHash();


Use tabs for indentation.

metthal · 2018-06-03T17:44:35Z

include/retdec/fileformat/types/export_table/export.h

@@ -42,6 +42,7 @@ class Export

 		/// @name Other methods
 		/// @{
+        virtual bool isUsedForImphash() const;


Use tabs for indentation. Also, this method shouldn't be here.

metthal · 2018-06-03T17:45:51Z

include/retdec/fileformat/types/export_table/export_table.h

 	public:
 		ExportTable();
 		~ExportTable();

 		/// @name Getters
 		/// @{
 		std::size_t getNumberOfExports() const;
+		std::string getExphashCrc32() const;


This should probably return const std::string&.

metthal · 2018-06-03T17:46:15Z

src/fileformat/file_format/pe/pe_format.cpp

-		loadImports();
-		loadExports();
+
+    if(stateIsValid)


Use tabs for indentation.

metthal · 2018-06-03T17:46:24Z

src/fileformat/file_format/pe/pe_format.cpp

@@ -733,6 +734,8 @@ void PeFormat::loadExports()
 		exportTable->addExport(newExport);
 	}

+    loadExpHash();


Use tabs for indentation.

metthal · 2018-06-03T17:46:30Z

src/fileinfo/file_detector/file_detector.cpp

@@ -26,7 +26,7 @@ namespace fileinfo {
 FileDetector::FileDetector(std::string pathToInputFile, FileInformation &finfo, retdec::cpdetect::DetectParams &searchPar, retdec::fileformat::LoadFlags loadFlags) :
 	fileInfo(finfo), cpParams(searchPar), fileConfig(nullptr), fileParser(nullptr), loaded(false), loadFlags(loadFlags)
 {
-	fileInfo.setPathToFile(pathToInputFile);
+    fileInfo.setPathToFile(pathToInputFile);


Use tabs for indentation.

metthal · 2018-06-03T17:46:36Z

src/fileinfo/file_detector/pe_detector.cpp

@@ -37,8 +37,8 @@ const unsigned long long PE_16_FLAGS_SIZE = 16;
 PeDetector::PeDetector(std::string pathToInputFile, FileInformation &finfo, retdec::cpdetect::DetectParams &searchPar, retdec::fileformat::LoadFlags loadFlags) :
 	FileDetector(pathToInputFile, finfo, searchPar, loadFlags)
 {
-	fileParser = peParser = std::make_shared<PeWrapper>(fileInfo.getPathToFile(), loadFlags);
-	loaded = peParser->isInValidState();
+    fileParser = peParser = std::make_shared<PeWrapper>(fileInfo.getPathToFile(), loadFlags);


Use tabs for indentation.

metthal · 2018-06-03T17:47:48Z

src/fileinfo/file_information/file_information.h

@@ -179,6 +179,9 @@ class FileInformation
 		/// @name Getters of @a exportTable
 		/// @{
 		std::size_t getNumberOfStoredExports() const;
+        std::string getExphashCrc32() const;


Use tabs for indentation.

metthal · 2018-06-03T17:48:27Z

...ntation/getters/iterative_getter/iterative_distribution_getter/export_table_plain_getter.cpp

-	info.push_back(numToStr(fileinfo.getNumberOfStoredExports()));
-
-	return info.size();
+    desc.push_back("CRC32            : ");


Use tabs for indentation.

metthal · 2018-06-03T17:48:37Z

...presentation/getters/iterative_getter/iterative_subtitle_getter/export_table_json_getter.cpp

-	info.push_back(numToStr(fileinfo.getNumberOfStoredExports()));
-
-	return info.size();
+    desc.push_back("crc32");


Use tabs for indentation.

s3rvac · 2018-06-03T18:06:20Z

A few notes from me:

Our coding conventions are described here.
It would be good to reference Fileinfo: generate "import table hash" equivalent for export table (i.e. "export table hash") #121 in the commit and in the PR (this will allow GitHub to automatically link the commit, issue, and PR).
The commit message and the PR would benefit from a description of what they do. compute hashes is too generic.
Please, add regression tests for this PR (into a separate branch, not in master). For inspiration, take a look here.

TravisCI build failed with the following error (details):

retdec/src/fileformat/types/import_table/import_table.cpp: In member function ‘void retdec::fileformat::ImportTable::clear()’:
retdec/src/fileformat/types/import_table/import_table.cpp:829:2: error: ‘impHashBytes’ was not declared in this scope

metthal

I've added my review notes and I also have two general notes to add:

What about other file formats like ELF or Mach-O? I believe this kind of hash would come handy for us even for formats other than PE.
Whenever we detect nonprintable characters in export name we replace its name with generic name exported_function_<address> (at least on PE and Mach-O). This can potentially cause problems and generate same hash for two completely different files with export tables corrupted in the same way. I'll need to analyze this before I give you final decision. I'll get back to you soon.

metthal · 2018-06-05T19:33:54Z

include/retdec/fileformat/types/export_table/export_table.h

@@ -21,14 +21,20 @@ class ExportTable
 {
 	private:
 		using exportsIterator = std::vector<Export>::const_iterator;
-		std::vector<Export> exports; ///< stored exports
+		std::vector<Export> exports;				///< stored exports


We use tabs only to indent code. If you want to add comments at the end of the line, align it with spaces please.

metthal · 2018-06-05T19:40:21Z

...presentation/getters/iterative_getter/iterative_subtitle_getter/export_table_json_getter.cpp

 	info.push_back(numToStr(fileinfo.getNumberOfStoredExports()));
+	info.push_back(fileinfo.getImphashCrc32());


This should be exphash, not imphash. Same for all three of them.

compute hashes

7db978e

metthal added C-fileformat new-feature C-fileinfo labels Jun 3, 2018

metthal self-assigned this Jun 3, 2018

metthal self-requested a review June 3, 2018 17:42

metthal requested changes Jun 3, 2018

View reviewed changes

metthal changed the title ~~compute hashes~~ Implemented computation of export hashes Jun 3, 2018

fileformat and fileinfo export-table hashes support

2b05d27

metthal requested changes Jun 5, 2018

View reviewed changes

pruzko added 2 commits June 9, 2018 17:36

Imphash to Exphash and space padding fixed

86f7b2c

export hashes support for MACH-o and ELF file format

ef82739

metthal mentioned this pull request Jul 3, 2018

Resource icon hash computation #344

Closed

metthal approved these changes Jul 4, 2018

View reviewed changes

metthal merged commit df4bd48 into avast:master Jul 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented computation of export hashes #321

Implemented computation of export hashes #321

JakubPruzinec commented Jun 3, 2018 •

edited

Loading

metthal left a comment

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

metthal Jun 3, 2018

s3rvac commented Jun 3, 2018 •

edited

Loading

metthal left a comment

metthal Jun 5, 2018

metthal Jun 5, 2018

		info.push_back(numToStr(fileinfo.getNumberOfStoredExports()));
		info.push_back(fileinfo.getImphashCrc32());

Implemented computation of export hashes #321

Implemented computation of export hashes #321

Conversation

JakubPruzinec commented Jun 3, 2018 • edited Loading

metthal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s3rvac commented Jun 3, 2018 • edited Loading

metthal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakubPruzinec commented Jun 3, 2018 •

edited

Loading

s3rvac commented Jun 3, 2018 •

edited

Loading