Regression on PDF type detection

[MaximeLaffaire]

Hi !

I have a question about this PR, you have removed the PDF header detection in the first 1024 bytes, and I was wondering why ?

For reminder I added that feature because of the Adobe spec for PDF describing it (page 1102, section 3.4.1).

I am curious about the security issues mentioned in the PR, do you have more details about that and know if we could find a way to re-enable the header detection while still responding to this security issue ?

For the context : we have an app where users can upload PDF documents and we are using this library in our validation process.
We find out that A LOT of PDFs (coming from various sources, like scanners or even Google invoices) has some data before the %PDF header.

Thank you :)

[neilharvey]

Hey, the issue was that scanning the header for the %PDF signature in a non-fixed location allowed for an executable to be disguised as a PDF file and the library would incorrectly identify it as PDF.

I looked at whether I could change the Pdf algorithm to defeat the vulnerability - I had hopes that including a check for %EOF at the end of the file would resolve it but that can also be substituted in an executable so it didn't solve the issue. Potentially I could have brought in a library to parse the entire structure of the file but that feels like overkill and not what I was trying to achieve with this library.

I considered whether the Pdf class should simply blacklist the exe header for safety, but whilst that might solve the vulnerability there is still a reasonable chance that a different format could have a %PDF string somewhere in it's header so the fault might just shift elsewhere.

I did have some success with changing the FileFormatInspector class so that it could detect when a format was reading an arbitrary amount of bytes to perform it's check and marking the result as 'unsafe' - but it made the class a fair bit more complex and I wasn't really happy with making the change for a single misbehaving format.

So eventually I landed back at just reverting the behaviour. The conclusion I came to was that scanning a header for a short byte string is never going to be safe. Apologies for the regression.

What I would probably do if I needed that behaviour back would be to reinstate your PDF formats and then plug these into the library during start-up.

Something like this perhaps:

var recognised = new FileFormat[] {
  // Implementations as per PR#66
  new ExtendedAdobePdf();
  new ExtendedPdf();
};
var inspector = new FileFormatInspector(recognised);
services.AddSingleton<IFileFormatInspector>(inspector);

[MaximeLaffaire]

Yes ok I understand, we will extend the inspector on our side then :)

Thanks for the clear explanation !

[Sicos1977]

An executable always needs to start with the magic-bytes MZ (first 2 bytes) so you could just check if these are present and then you know you found an executable that tries to hide as a PDF