Add Certificate Table parsing and Authentihash support for PE

[abcSup]

Your checklist for this pull request

• I've read the guidelines for contributing to this repository
• I made sure to follow the project's coding style
• I've added tests that prove my fix is effective or that my feature works (if possible)
• I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description
Add Authentihash and Authentihash check support for Portable Executable. Currently, the Authentihash information are stored in the pe sdb.

Missing steps

• A signed binary with valid Authentihash to test true case. There seems to be only signature.exe in testbins which has an invalid Authentihash.
• Include Authentihash in it output (?)

Imo, Authentihash information should be displayed at somewhere specific to the file format. For now, it seems to generate general hashes (md5, sha1, sha256). Adding Authentihash into the command will complicate the code (checking if rclass == pe, get authentihash with sdb_querys, output directly) while the existing code is using a RList of RBinHash, which makes me wonder if there is a cleaner way to do this. For Authentihash check, I thought about adding it into RBinInfo, but that makes the structure less general, so I am looking forward to some feedbacks.

Closing issues
#921

[radare]

minor improvements. and ready to merge

[ret2libc]

Missing steps

* [ ]  A signed binary with valid Authentihash to test true case. There seems to be only `signature.exe` in testbins which has an invalid Authentihash.

I do not use Windows. Do you have any of those binary on your system? If so, you can upload it to https://github.com/radareorg/radare2-testbins .

* [ ]  Include Authentihash in `it` output (?)

Imo, Authentihash information should be displayed at somewhere specific to the file format. For now, it seems to generate general hashes (md5, sha1, sha256). Adding Authentihash into the command will complicate the code (checking if rclass == pe, get authentihash with sdb_querys, output directly) while the existing code is using a RList of RBinHash, which makes me wonder if there is a cleaner way to do this. For Authentihash check, I thought about adding it into RBinInfo, but that makes the structure less general, so I am looking forward to some feedbacks.

Maybe you can fix a bit the code around the RBinPlugin->info method. In bin_pe.inc:info() you could compute the authentihash, create a RBinHash structure and put it in info->file_hashes (you should adapt a bit the code in bfile.c:r_bin_file_hash() to consider the case when info->file_hashes is already initialized, or maybe you should always initialize info->file_hashes somewhere else). By doing so the hash will be shown in it and any file format can easily add its own specific hash.

[abcSup]

I do not use Windows. Do you have any of those binary on your system? If so, you can upload it to https://github.com/radareorg/radare2-testbins .

Currently, I am testing it on commercial programs. I will look it up how to sign one without a proper cert.

Maybe you can fix a bit the code around the RBinPlugin->info method. In bin_pe.inc:info() you could compute the authentihash, create a RBinHash structure and put it in info->file_hashes (you should adapt a bit the code in bfile.c:r_bin_file_hash() to consider the case when info->file_hashes is already initialized, or maybe you should always initialize info->file_hashes somewhere else). By doing so the hash will be shown in it and any file format can easily add its own specific hash.

Thanks for the green light, I will proceed to refactor bfile.c:r_bin_file_hash() to make it more general.

[radare]

thanks! i think we are good to remove the WIP after you do this refactor so we will be ready to merge

[XVilka]

Binary was merged, please update the PR and we are good to go.

[abcSup]

Appveyor and one of the Travis tests passed, but others failed because of:

v/v install radare.r2
Skipping module "radare.r2", since it is missing name or url information.
Makefile:19: recipe for target 'v/v' failed
make[1]: Leaving directory '/home/travis/build/radareorg/radare2/test/src'
make[1]: *** [v/v] Error 1
make[1]: Target 'all' not remade because of errors.
make: *** [src/r2r] Error 2
Makefile:14: recipe for target 'src/r2r' failed
make: Target 'all' not remade because of errors.
+ exit 1

I assume that if we rebuild the tests, it should be fine.

[ret2libc]

Looking at it now, I'm not sure adding the securihash to the info method was the best approach :(

I have not run your branch, but I believe r_file_bin_hash will not compute the format-specific hashes, so SecuriHash will be computed only the first time and it will never be updated, right? Also, I think the API is not very good, as it forces users of r_bin_file_hash to save the result in o->info->file_hashes and that's error prone (because you have to remember to manually update that value every time you use r_bin_file_hash) and not very encapsulating.

For the r_bin_file_hash APIs, what about:

// compute hashes of the current rbinfile and return them
RList *r_bin_file_compute_hashes(RBin *bin, int limit);
// set `hashes` as the last computed hashes for the current rbinfile. `hashes` becomes owned by the current rbinfile, so you don't need to free it. If other hashes were previously set for the current rbinfile, they are returned and they should be freed by the caller when not needed anymore.
RList *r_bin_file_set_hashes(RBin *bin, RList *hashes);

So for it you can do something like:

RList *new_hashes = r_bin_file_compute_hashes (bin, limit);
RList *old_hashes = r_bin_file_set_hashes (bin, new_hashes);
.... do the diff, print, etc ....
r_list_free (old_hashes);

And in radare2.c something like:

r_bin_file_set_hashes (bin, r_bin_file_compute_hashes (bin, limit));

With regard to where to place the new securihash, maybe the best thing was indeed to add a RBinPlugin.hashes method and call it at the end of r_bin_file_compute_hashes function.

What do you think?

[ret2libc]

this does not re-compute the securihash, does it?

[abcSup]

You are right, it does not.

[abcSup]

Looking at it now, I'm not sure adding the securihash to the info method was the best approach :(

I have not run your branch, but I believe r_file_bin_hash will not compute the format-specific hashes, so SecuriHash will be computed only the first time and it will never be updated, right? Also, I think the API is not very good, as it forces users of r_bin_file_hash to save the result in o->info->file_hashes and that's error prone (because you have to remember to manually update that value every time you use r_bin_file_hash) and not very encapsulating.

For the r_bin_file_hash APIs, what about:

// compute hashes of the current rbinfile and return them
RList *r_bin_file_compute_hashes(RBin *bin, int limit);
// set `hashes` as the last computed hashes for the current rbinfile. `hashes` becomes owned by the current rbinfile, so you don't need to free it. If other hashes were previously set for the current rbinfile, they are returned and they should be freed by the caller when not needed anymore.
RList *r_bin_file_set_hashes(RBin *bin, RList *hashes);

So for it you can do something like:

RList *new_hashes = r_bin_file_compute_hashes (bin, limit);
RList *old_hashes = r_bin_file_set_hashes (bin, new_hashes);
.... do the diff, print, etc ....
r_list_free (old_hashes);

And in radare2.c something like:

r_bin_file_set_hashes (bin, r_bin_file_compute_hashes (bin, limit));

With regard to where to place the new securihash, maybe the best thing was indeed to add a RBinPlugin.hashes method and call it at the end of r_bin_file_compute_hashes function.

What do you think?

Initially, I was thinking that RBinInfo should always keep the original hashes in info->file_hashes and subsequent calls to r_bin_file_hashes will compute new hashes, so the users can always know that the file has been changed previously instead of discarding the old hashes and replacing it with the new ones. And bfile.c:r_bin_file_hashes will not hash files larger than the limit argument and discard the old hashes in info->file_hashes (Authentihash), so keeping the original hashes will be more appropriate.

I think your suggestions are good by including the binary specific hash in RBinInfo as a method and call it from r_bin_compute_hash and having a new method r_bin_file_set_hashes to set and return the old hashes. These help resolving previous design problems I faced.

[ret2libc]

I think your suggestions are good by including the binary specific hash in RBinInfo as a method and call it from r_bin_compute_hash and having a new method r_bin_file_set_hashes to set and return the old hashes. These help resolving previous design problems I faced.

I guess you meant RBinPlugin, right?

[abcSup]

I guess you meant RBinPlugin, right?

Yeah, sorry for the typo. I will work on it first and see how it goes. Thanks for the help! :)

[ret2libc]

No problem, thank you for working on this! The base stuff that calculates the securihash is actually good already I think, it's just a matter of integrating it with r2 code base ;)

[abcSup]

I have clicked wrong for the revert pull request. I guess I will create another pull request for the refactor in two hours.

[radare]

Yep better improve in a separate pr than reverting. The contrib is good 👍

…

On 20 Feb 2020, at 21:46, Zi Fan ***@***.***> wrote:  I have clicked wrong for the revert pull request. I guess I will create another pull request for the refactor in two hours. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.