Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parsing of the PE Authenticode format #902

Merged
merged 59 commits into from
May 20, 2021

Conversation

HoundThe
Copy link
Member

@HoundThe HoundThe commented Nov 29, 2020

I'm submitting progress on Authenticode parsing, it's still a work in progress.

There are few shortcuts I've taken during development that I need to finish.

  • Parse out the rest of the information from PKCS9 counter-signatures
  • Parse undocumented MsCounterSignature
  • Check if the signer can have multiple counter signers, same with a counter signer
  • Free the Openssl allocations correctly (use smart pointers)
  • Implement plain text output
  • Add verification of the signatures and export any abnormalities to the user output

Also, the commit history is a mess, I can fix it if that's a problem.

Further plans after this could be adding more things that are being parsed, validation/verification of the signature contents

Also, solves #380.

@HoundThe HoundThe changed the title [WIP] Add parsing of the PE Authenticode format Add parsing of the PE Authenticode format Apr 6, 2021
Copy link
Collaborator

@PeterMatula PeterMatula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core looks ok and I'm ready to merge it after a few minor changes.

  • It looks like Telfhash, rich header hash, and even some UPX changes got mixed up with this. That is unfortunate and it convolutes the PR. Please fix the merge conflicts, and be careful about this in the future.
  • Also, there is a massive alignment change in src/fileinfo/file_information/file_information.h which also pollutes the PR. You don't have to change it back. it is ok, but again it is not good to have this in a PR.
  • Add new lines at the ends of your sources. Many are missing them. Git doesn't like it.
  • Nitpick: In general we use C++/Doxygen style comments in RetDec. You use C style comment. Don't change it now, it is ok. I said you can use whatever style you want if you are consistent about it, and I should have noticed this earlier. Just for the future reference, it is best to use C++/Doxygen comments in C++, especially if other modules are using it.

CHANGELOG.md Outdated Show resolved Hide resolved
deps/CMakeLists.txt Outdated Show resolved Hide resolved
src/fileformat/file_format/pe/pe_format.cpp Outdated Show resolved Hide resolved
src/fileinfo/file_information/file_information.cpp Outdated Show resolved Hide resolved
src/fileinfo/file_information/file_information.h Outdated Show resolved Hide resolved
@HoundThe
Copy link
Member Author

Yes, I polluted the PR when I merged with master, I removed the polluting commits and forced pushed it into (hopefully) correct state.

@HoundThe
Copy link
Member Author

I have reverted the indent as I changed it on an accident with autoformatter, it also solves the merge conflict.

@HoundThe HoundThe requested a review from PeterMatula May 11, 2021 15:18
@PeterMatula
Copy link
Collaborator

PeterMatula commented May 13, 2021

Now the tests are falling, probably only because of "certificateTable" -> "digitalSignatures" rename. I hope once avast/retdec-regression-tests#86 is modified and notes there are fixed it will pass and we can merge it.

@PeterMatula
Copy link
Collaborator

Lets run TC tests.

@PeterMatula
Copy link
Collaborator

TC tests are still failing, but it is because it is not testing this PR with the associated tests PR. For future reference - RetDec PR branch is tested with the RetDec regression tests PR branch with a matching (i.e. the same) name - if it exists, otherwise with master. So in this case HoundThe:sig_parser is not matching HoundThe:authenticode.

@PeterMatula
Copy link
Collaborator

PeterMatula commented May 17, 2021

@HoundThe can you try to rename the branch for regression tests PR to HoundThe:sig_parser and create a new PR? It should work if there won't be another problem with the fact that these are branches from another repo (your clone). We use PRs even for merging our branches from this repo and I'm not sure we ever tested it with 3rd party PR from cloned repos - at least, we will test it now.

@HoundThe
Copy link
Member Author

HoundThe commented May 17, 2021

I've renamed the testing PR branch to sig_parser

@HoundThe
Copy link
Member Author

Some Windows tests were failing because of UB from an uninitialized algorithm value, when parsing of a countersignature fails, should be fixed now.

@PeterMatula PeterMatula merged commit d800097 into avast:master May 20, 2021
PeterMatula added a commit that referenced this pull request May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants