Encrypt the byte order mark along with the text it marks (#460) - #494
Open
jafin wants to merge 2 commits into
Open
Encrypt the byte order mark along with the text it marks (#460)#494jafin wants to merge 2 commits into
jafin wants to merge 2 commits into
Conversation
A UTF-16BE text string begins with a byte order mark, and that mark is part of the value of the string rather than part of its syntax, so an encrypted document has to encrypt it with the text. PdfSharpCore spelled it out in front of the ciphertext instead, writing <FEFF...> where the FEFF was plain and only what followed was encrypted. A reader decrypts the whole string, so it took the mark for two bytes of text, and with the keystream two bytes out of step from there on the rest of the string came out as noise. That is what makes the title and the author of a protected document unreadable in Acrobat and in Firefox, reported in issue ststeiger#460. Only Title, Author, Subject, Keywords and Creator are affected. PdfDocumentInformation writes those with PdfStringEncoding.Unicode, while Producer is set through the two argument SetString and goes out as a plain literal, which is why the round trip test that asserts on Producer never saw this. The reader made the same mistake in reverse. It strips the mark in the lexer, before anything has been decrypted, so a string encrypted the way the specification asks for it arrives as bytes that no longer say what they are, and a document from any other producer reads back as noise. Nothing showed, because a reader that shares the writer's mistake reads the writer's output back perfectly. The two halves are one fault and have to move together: with only the writer corrected, the existing CreateAndReadPasswordProtectedPdf fails on its outline title. The writer now puts the mark into the bytes before they are encrypted, which leaves an unencrypted document byte for byte what it was, since the same mark is written either way. The reader looks for it after decrypting, where it can now be seen, and corrects the encoding of the string to match what it found. PdfStringObject gains the Unicode handling its TODO asked for. Documents written before this still open: their mark is outside the ciphertext, the lexer strips it as it always did, and what is decrypted then carries no mark to find. The tests check what the writer produces against an independent implementation of the standard security handler rather than by reading it back, since reading it back is what hid this. That implementation checks its own key derivation against the /U entry of the document it is given, so a test that cannot agree with the document fails rather than comparing noise to noise.
Moving the byte order mark into the bytes moved it into the count the hexadecimal writer breaks lines by, which breaks every 48 bytes. A string of more than 24 characters therefore came out with its line breaks two bytes earlier than before. The breaks are white space inside a hexadecimal string and mean nothing to a reader, but the claim that an unencrypted document is left byte for byte what it was only held for strings too short to break at all, which is exactly what the test used. Count from the text rather than from the start of the bytes, so the mark travels with the text without carrying the count along with it. The test now covers a title long enough to break, and the expected string is built by the loop as it stood before, so it states what is being preserved rather than restating the result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #460, reproduced with the exact
SecureDocumentsettings from that issue at both 40-bit and 128-bit.What happens
A UTF-16BE text string begins with a byte order mark, and that mark belongs to the value of the string rather than to its syntax, so an encrypted document has to encrypt it along with the text. PdfSharpCore spells it out in front of the ciphertext:
A conforming reader decrypts the whole string. It takes
FE FFfor two bytes of text, and from there the keystream is two bytes out of step, so everything after it comes out as noise. That is why the title and author of a protected document are unreadable in Acrobat and in Firefox.Only
Title,Author,Subject,KeywordsandCreatorare affected:PdfDocumentInformationwrites those withPdfStringEncoding.Unicode, whileProduceris set through the two-argumentSetStringand goes out as a plain literal. That is also whyPdfSecurity.CreateAndReadPasswordProtectedPdf, which asserts onProducer, never saw this.Setting
DocumentSecurityLevel.None, suggested in the thread, avoids it only by removing the encryption.The reader has the same fault in reverse
Lexer.ScanHexadecimalStringstrips the mark before anything is decrypted, so a string encrypted the way the specification asks for it arrives as bytes that no longer say what they are. A document from any other producer reads back as"\u00FE\u00FF\u0000T\u0000h\u0000e...". Nothing showed, because a reader that shares the writer's mistake reads the writer's output back perfectly.The two halves are one fault and have to move together: with only the writer corrected, the existing
CreateAndReadPasswordProtectedPdffails on its outline title. That was verified rather than assumed.The fix
PdfEncoders.FormatStringLiteralputs the mark into the bytes before they are encrypted. An unencrypted document is left byte for byte what it was, since<+ hex(FEFF‖ bytes) is the same text as<FEFF+ hex(bytes).PdfString.EncryptionValueandPdfStringObject.EncryptionValuelook for the mark after decrypting, where it can now be seen, and correct the encoding of the string to match.PdfStringObjectalso gains the Unicode handling itsTODOasked for.Documents written before this still open: their mark is outside the ciphertext, the lexer strips it as it always did, and what is decrypted then carries no mark to find. There is a test for that.
Only RC4 is affected in practice, since that is all the writer produces; the AES path is read-only and goes through the same corrected code.
How this was checked
Not by reading the output back — that is what hid the fault.
PdfSharpCore.Test/Security/StandardSecurity.csis an independent implementation of the standard security handler (ISO 32000-1 clause 7.6, algorithms 1, 2, 4 and 5). It checks its own key derivation against the /U entry of the document it is given, so a test that cannot agree with the document fails rather than comparing noise to noise. With the key confirmed that way,/Producerdecrypted perfectly while the Unicode strings did not, which is what isolated this to Unicode strings rather than to the key.EncryptedTextStringTestscovers, at both security levels:Without the change 4 of the 9 fail. With only the writer corrected, the read-back tests and the pre-existing security test fail.
Test run on net8.0: 35 passed, 5 failed, 1 skipped. The five failures are the
XTextFormatterTestrendering comparisons, which fail identically on an unmodified master here because they shell out to Ghostscript (gswin64c.exe, exit 127) and it is not installed on this machine. Baseline on master is 26 passed with the same 5 failures, so this adds 9 passing tests and changes nothing else.Note for reviewers
This changes the bytes of every encrypted document the library writes. Old documents still open, but anything downstream pinning the expected output of an encrypted PDF will see a diff.