-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for AES256-CBC encryption #338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's merge this now, if it works.
i think this could be further improved by reading various attributes and not hard-coding so many things, such as these:
manifest:encryption-data
manifest:checksum-type
manifest:start-key-generation
manifest:start-key-generation-name
manifest:key-derivation
manifest:iteration-count
manifest:key-derivation-name
manifest:algorithm
manifest:algorithm-name
md = MessageDigest.getInstance("SHA-256"); | ||
passBytes = md.digest(passBytes); | ||
PBEParametersGenerator generator = new PKCS5S2ParametersGenerator(new SHA1Digest()); | ||
generator.init(passBytes, salt, 100000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that value was bumped in 2016, it was 1024 before then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could also read these values from the manifest, but I didn't want to change too much of your code.
generator.init(passBytes, salt, 100000); | ||
KeyParameter keyParam = (KeyParameter) generator.generateDerivedParameters(256); | ||
key = new SecretKeySpec(keyParam.getKey(), "AES"); | ||
cipher = Cipher.getInstance("AES/CBC/NoPadding"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe AES uses W3C padding? (whereas Blowfish didn't need padding)
hmm ... i find this page: https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html
this line probably?
ISO10126Padding This padding for block ciphers is described in 5.2 Block Encryption Algorithms in the W3C “XML Encryption Syntax and Processing” document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only worked for me without padding.
I got my info from this StackOverflow posting: https://stackoverflow.com/questions/62001716/encrypt-a-file-inside-an-ods-archive/62005258?r=Saves_UserSavesList#62005258
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm but that also says to use ISO10126Padding?
// AES-256-CBC | ||
md = MessageDigest.getInstance("SHA-256"); | ||
passBytes = md.digest(passBytes); | ||
PBEParametersGenerator generator = new PKCS5S2ParametersGenerator(new SHA1Digest()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a different name for PBKDF2, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it didn't work for me with ‘SecretKeyFactory.getInstance(’PBKDF2WithHmacSHA256‘);’.
That's why I switched to BouncyCastle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes it would need to be with HmacSHA1 for ODF
This pull request fixes the problem with password-protected LibreOffice files described in Issue #138.
It is now possible to open files with the old Blowfish encryption as well as with the AES256-CBC encryption.
Unfortunately, I had to add BouncyCastle because I couldn't solve the problem with the existing libraries.
resolves #138