Fix bug with multiple signatures. #940
Conversation
|
One thought - if you are calling |
|
I plan to implement a limit (16 seems reasonable) and may have uncovered a bug in it. I'll hunt it down and update this PR soon. |
|
I've uncovered a weird case here which I haven't had the time to fully understand, and may not be able to get to it this week. If anyone wants to take a look here's what I know. I'm hoping to get to this by the end of this week but if someone knows what is going on with this file I'd appreciate a pointer. :) |
|
@wxsBSD is this PR ready to be merged? |
|
I’ve got some more work to do on this. Can you give me a couple of days, please? I’ll dedicate some time to it and hopefully have a better PR by Friday morning! |
Sure, no hurries. I was just cleaning up the list of pending PRs a little bit. |
…nd don't recurse forever when parsing nested signatures.
wxsBSD
force-pushed
the
multi_signatures
branch
from
59dd2b8 to
f925cc4
Compare
|
Sorry for the force push, but after spending too long fighting with openssl I'm not in the best of moods. ;) This branch is ready to merge. It now parses the files with nested signatures better (tested on d29e93c0fe4f108fa063e1a9692559a4278a0d51ab4feabbb231907dffaeb019) and will stop parsing at 16 nested signatures. I also moved away from using the BIO interface in OpenSSL in favor of d2i_PKCS7 as it's less cumbersome to work with. I spent entirely too long trying to get b605b216588fe454b76c51dbb407cc9e71f7b1932367d5557fcc296bd9f28d34 to parse correctly, and I'm giving up on it. As far as I can tell this is not a very common format and was only used by MSFT for a while, but I've been told they are moving away from it in favor of the multiple WIN_CERT format (see https://twitter.com/vathpela/status/1045389079473459200 for details). I'll give a rundown of the file in case anyone wants to pick up where I left off, but I'm not planning on fixing this problem because it's a pain and not likely to be problematic IMO. The start of the WIN_CERT structure is at 0xe3000. The certificate contained in there has serial |
|
Thank you to @recvfrom for showing me how to walk the X509 attributes! I'm able to now parse the 3rd signature from the file in question. This PR is now complete and brings YARA more in line with how MSFT is parsing authenticode signatures. |
Move the nested signature checking out of the main certificate parsing loop. The nested signatures are on the PKCS7 structure, not the certificate. Also, make the loop better by not processing the same attribute over and over. These were suggested by Andrew Williams.
If the nested signature is ever NULL, break early because it will always be NULL. Also, tighten up the the checks for MAX_PE_CERTS. We aren't likely to ever see a PE that hits the case where it has multiple certs in a single PKCS7 blob, but it can't hurt to check in the loop too.
|
@wxsBSD it looks like this is not compiling in Windows. Maybe the version of OpenSSL we are using in Windows doesn't have the |
|
I’ll take a look tonight. |
|
I don't have a copy of VS 2017 handy to test on Windows, but I did manage to confirm the Is it possible to get better build logs from appveyor? In particular can we get whatever the equivalent of "V=1 make" is on there? Maybe that will point us in a better direction for debugging? |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
It looks like this PR makes harder a future use of BoringSSL as a replacement of OpenSSL. BoringSSL doesn't implements any of the PKCS7 APIs and they don't plan to do it. So, we either forget about BoringSSL or keep this unfixed :( |
|
I’d rather see this fixed than switch to BoringSSL. I think accuracy in parsing is the more important thing here. |
|
It'd be worth investigating how difficult it'd be to re-implement the functionality in this patch without using the PKCS7 functions... I'll aim to take a look sometime in the next few days unless someone beats me to it. |
I don't think it's worth the effort, PKCS7 seems to be a very complicated format and parsing it is error-prone. |
|
@wxsBSD I found out why the build is breaking in Appveyor. It turns out that wincrypt.h has the following macro definition which conflicts with the type defined by OpenSSL: //+-------------------------------------------------------------------------
// Predefined PKCS #7 data structures that can be encoded / decoded.
//--------------------------------------------------------------------------
#define PKCS7_SIGNER_INFO ((LPCSTR) 500)Looking for the most elegant way for fixing this. |
|
It looks like Wine had the same issue, they simply undefined those macros: |
|
Ah, thanks for digging into this and merging it. As I’ve said before, I’m all for removing OpenSSL but I don’t want to sacrifice useful functionality or parsing accuracy. |
* Fix bug with multiple signatures. * Don't use BIO interface, use d2i instead. Fix some other small bugs and don't recurse forever when parsing nested signatures. * Walk the X509 attributes looking for nested signatures. * Move variable declarations out of loop. * Move nested signature checking out of the loop. Move the nested signature checking out of the main certificate parsing loop. The nested signatures are on the PKCS7 structure, not the certificate. Also, make the loop better by not processing the same attribute over and over. These were suggested by Andrew Williams. * If the nested signature is NULL, break early. If the nested signature is ever NULL, break early because it will always be NULL. Also, tighten up the the checks for MAX_PE_CERTS. We aren't likely to ever see a PE that hits the case where it has multiple certs in a single PKCS7 blob, but it can't hurt to check in the loop too.
This fixes a bug with files that have multiple signatures. Turns out there are two ways to have multiple signatures.
The way it works is it makes the code which converts a PKCS7 structure into YARA module data into a function
_parse_pkcs7()and then at the very end it checks if there is a "hidden" nested signature and recursively calls itself if one is found. That part of it is on line 1265, the rest of this diff is mostly just a copy/paste of the rest of the code into the recursive function.I've tested this with the sample mentioned in #515 (d29e93c0fe4f108fa063e1a9692559a4278a0d51ab4feabbb231907dffaeb019) and it does properly parse both of them. I've also verified it by comparing it to what Windows says about the binary and it does indeed match.
Fixes #515.