Encryption and access privileges

[zesik]

This pull request adds encryption and security features defined in PDF spec v1.3, v1.4, v1.6 and v1.7 ExtensionLevel 3. This PR fixes #16.

It adds an encryption dictionary for access privileges and the following encryption methods on content:

• 40-bit RC4 (PDF 1.3)
• 128-bit RC4 (PDF 1.4 and 1.5)
• 128-bit AES (PDF 1.6 and 1.7)
• 256-bit AES (PDF 1.7 ExtensionLevel3)

[alafr]

Thank you for this nice feature.
It appears however that 40-bit RC4 is obsolete and not secure any more (can be cracked in about 2 days with a regular PC). Unfortunately it's the only encryption available in PDF 1.3.
Is it possible to add support for more secure encryption such as 256-bit AES and dynamically change the pdf version?

[zesik]

I would prefer to use AES as well, and I think it is possible to dynamically set version.

If there's no specific reason to stay in 1.3, I'll try to implement encryption for newer version.

[zesik]

@alafr Hi, I removed RC4 and added AES-128 (PDF 1.7) and AES-256 (PDF 1.7 ExtensionLevel 3). By default AES-128 is used, but users can choose AES-256 by specifying some option. Please have a look!

PS: I don't have access to the PDF 1.7 ExtensionLevel 8 or PDF 2.0 specification now, so I implemented encryptions defined in v1.7 spec.

[liborm85]

But pdfkit generate PDF file content in version 1.3.
Can be used encryption from 1.7, but generate content in 1.3? Is it a valid pdf file?

I think encryption should be used according to version which will be set in pdfkit to generate pdf file - RC4 for 1.3 and AES for 1.7.

[alafr]

It may be useful to keep the RC4 option.
PDF files made with PDFKit are not valid for version 1.3, it already uses opacity supported since version 1.4. I think it's an issue that must be fixed. PDF readers are not very strict with file versions but we should ensure PDFKit makes valid files.

[zesik]

Can be used encryption from 1.7, but generate content in 1.3?

@liborm85 If you mean the header in generated PDF file, this PR ensures that when encryption is enabled, a PDF 1.7 header is generated.

It may be useful to keep the RC4 option.

@alafr I removed this option because it is unsafe. Since we can set newer versions in the header, I didn't find it necessary to keep outdated and unsafe encryption methods. But I can add it back for the sake of completeness and compatibility.

You mentioned let PDFKit choose generated PDF version based on encryption in previous comments. But how about allowing users to specify PDF version and let PDFKit choose the best encryption method? This also enables other PDFKit code to utilize this information and decides how it create contents.

[zesik]

@alafr I added 40-bit RC4 and 128-bit RC4.

Now, supported encryption methods are:

• 40-bit RC4 (PDF 1.3)
• 128-bit RC4 (PDF 1.4 and 1.5)
• 128-bit AES (PDF 1.6 and 1.7)
• 256-bit AES (PDF 1.7 ExtensionLevel3)

[petrgazarov]

Nice! Look forward to this feature.

[HunterMitchell]

Do we know when this might be merged into master?

[zesik]

Looks like the ES6 PR is merged before this branch.
I'm going to fix conflicts and adapt to ES6.

[zesik]

I updated the MR with ES6!

@blikblum In lib/reference.js of your ES6 update, length key (/Length) is written before zipping of streams, so the value is length of original stream. AFAIK, the /Length value matches (zipped) stream length. Could you tell me if I'm understanding this wrong? Thanks.

@devongovett Could you check this MR and let me know what you think? Thanks.

[blikblum]

@blikblum In lib/reference.js of your ES6 update, length key (/Length) is written before zipping of streams, so the value is length of original stream. AFAIK, the /Length value matches (zipped) stream length. Could you tell me if I'm understanding this wrong? Thanks.

This is AFAIK as it was in coffescript so no change here. It maybe a bug anyway

Regarding the PR:

• why the output of tests that do not use security feature changed? IMO it should not change
• the only drawback i see is the size increase because of dependencies - 50kb minified. For node does not matter but for browser, it matters. PDFKit is already a lot big. If we could a way to make it optional, like kind of plugin or middleware, better

[blikblum]

This is AFAIK as it was in coffescript so no change here. It maybe a bug anyway

This is really a bug

* why the output of tests that do not use security feature changed? IMO it should not change

Probably because of the stream Length fix. Although seems correct, better to do this change in a separated PR so we get sure that the encryption feature is not altering other files. I will try to setup a proper unit test so these issues can be easily detected

[zesik]

Probably because of the stream Length fix. Although seems correct, better to do this change in a separated PR so we get sure that the encryption feature is not altering other files. I will try to setup a proper unit test so these issues can be easily detected

The Length fix is necessary for encryption and decryption, otherwise applications cannot recognize encrypted streams. If you like, I can submit a separated PR for it, and it needs to be merged first.

Besides this, this PR also has the following two changes which affected other test cases:

• ID entry in file trailer dictionary. This is necessary for encryption, and is "strongly recommended" according to PDF reference, so I included it for all files.
• Use indirect reference in document information dictionary.

As long as tests check for the entire PDF file, this PR is likely to require modification of all snapshots.

If we could a way to make it optional, like kind of plugin or middleware, better

This might require changes of the build process. And probably there are other components which we can also optimize. Should it be done in this PR?

[blikblum]

As long as tests check for the entire PDF file, this PR is likely to require modification of all snapshots.

I see. Is not the ideal but since will be changing the pdfs anyway, is doable. Until there are proper unit tests we will have to live with that.

This might require changes of the build process. And probably there are other components which we can also optimize. Should it be done in this PR?

No, this is an important feature. IMO this should be merged with a note that is a experimental and API may change in future

[blikblum]

docs/guide.pdf can be removed from the PR

[blikblum]

I just added unit tests for references. It checks the Length property. Can you rebase and check if these tests passes with this PR?

It you could write some unit tests to trailer changes would be great (in document.spec.js)

[zesik]

New guide has documents about encryption. I see other features also modifying this file. Is it okay to use old one?

…

On Mon, Dec 17, 2018 at 10:19 Luiz Américo ***@***.***> wrote: ***@***.**** requested changes on this pull request. docs/guide.pdf can be removed from the PR — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#820 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACpaLMFYoEtCz6w3jz9AAsTsgJdIYxERks5u5vEggaJpZM4UGAUy> .

[blikblum]

New guide has documents about encryption. I see other features also modifying this file. Is it okay to use old one?

It should be updated on each new release.

[zesik]

@blikblum Changed the following:

• docs/guide.pdf is removed.
• Rebased branch and your reference unit test passed!
• Add unit test for trailer

[blikblum]

@devongovett this looks good to me. Any thoughts on this?

It there's no objection will be merging in the next days

The possible future interface changes can be discussed at #885