Check if certificateTable overlaps a section and export the information

[HoundThe]

Aims to fix #972

Checks if any real part of section overlaps with certificate table, if it does, then export that the signatures are not outside of the image and thus ignored by Windows (From experimenting with Windows and PE Bear I think that any physical [real] overlap of section and certificate table results in ignore by Windows).

If this solution is fine, I'll create tests for this.

[PeterMatula]

cc @metthal
The main question here is, what do we want to do in case there are signatures like these?

1. Do we want to completely ignore them, like Windows, and don't even parse them?
   • + Any tool that consumes Fileinfo output does not have to bother deciding if it wants to use signatures like these, and if it doesn't, checking some flag or whatever - i.e. there is no risk of using invalid signatures.
   • - It goes against our basic principles - parse as much as possible and let users decide what they want to consume.
2. Do we want to parse them, but tag them somehow? - i.e. the implemented solution.
   • + We parse everything, nothing is thrown away.
   • - Users need to know about these new flags, and if they don't want to use signatures like these (which most of them probably don't want to), they need to skip them - requires a change of code in all consumers, and there is a possibility this is forgotten in the future and invalid signatures are consumed.

@metthal what do you think?

A possible solution that would eat the cake and have it too - instead of adding a flag like implemented here, add a new container for invalid signatures and add them there. Existing consumers take only valid signatures, and if anyone wants to use these invalid ones, they need to read the new container explicitly.
However, I'm not sure we want to further complicate the related container structures.

Smaller notes regarding the code:

• isOutsideImage is a good name for the underlying flag, it can even be exposed as it is, but if we stick to this "is cert valid?" option, I would also add a method like isValid() (or a better name) because the semantic is not immediately clear from the flag name.
• I think this may deserve an entry in the detected anomalies as well - I'm right @ladislav-zezula?

[ladislav-zezula]

I think this may deserve an entry in the detected anomalies as well - I'm right @ladislav-zezula?

Could be. The question is also how many of such samples do we see - if not many, then it's perhaps abundant to define an anomaly.

[metthal]

It kinda reminds me the case we dealt with not that long ago with @ladislav-zezula. There were valid .NET structures in resources of PE file and we parsed them and shown them in fileinfo output. But the decision was to not parse these information. I kinda see some resemblance here. Someone put valid structure of a signature into last section but that is completely unacceptable to expect the signature there.

@ladislav-zezula let us know if you find out how common is this. But if we were to put it somewhere in fileinfo output, I would completely separate it from valid sigantures. I wouldn't rely on the fact that someone inspects some specific isValid attribute but I'd rather put it somewhere separately into some outOfImageSignatures array if really necessary.

[HoundThe]

More like insideImageSignatures? And keep the outOfImage signatures, because they are valid, like classic "digitalSignatures"? But I am not sure if the out/in ImageSignatures description is descriptive enough for most people that might not know the difference between those two.

[metthal]

From the analysis @ladislav-zezula did I got the feeling that it's not very common thing so I personally think that having these inside image or invalid signatures might be a bit pointless. Let's just list what is really a signature. What do y'all think?

[PeterMatula]

Is it just me, or is tools.fileinfo.bugs.certificate-invalid-dates.Test failing? TC is down, so I cannot verify it there now. But based on the discussion in this thread, it looks like invalid signatures are no longer collected - the test in question had "Signature isn't valid" warning and I think its signatures are correctly missing.
Therefore, @HoundThe verify it, and if so remove/modify the test in avast/retdec-regression-tests#100 and maybe even purge those warnings (and others like it) if they no longer make sense - i.e. such signatures are not to be taken into account.

[HoundThe]

Weird, the test is okay for me.

But I see I wrongly named the branches, maybe the problem is that RetDec PR is named pe_signature_inside_image and Tests PR is inside_image_signature?

[PeterMatula]

But I see I wrongly named the branches, maybe the problem is that RetDec PR is named pe_signature_inside_image and Tests PR is inside_image_signature

That would be a problem for TeamCity - it only associates branches with the same names. But I did this manually on my machine, so this was not an issue. Also, I reported this before and you said you could not reproduce it, so I'm wondering what is happening.

[PeterMatula]

lets run TC tests

[PeterMatula]

@HoundThe At the moment, there are merge conflicts.