Skip to content
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

Replace default insecure SHA1 hash algorithm #90

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
5 participants
@jornfranke
Copy link

jornfranke commented Jan 8, 2018

Replace default insecure SHA1 hash algorithm with SHA256.

SHA1 has been broken and should not be used anymore for signatures and should not be the default, cf. also https://security.googleblog.com/2016/11/sha-1-certificates-in-chrome.html

Replace default insecure SHA1 hash algorithm
Replace default insecure SHA1 hash algorithm with SHA256
@asfgit

This comment has been minimized.

Copy link

asfgit commented Jan 8, 2018

Can one of the admins verify this patch?

@pjfanning
Copy link
Contributor

pjfanning left a comment

Can you fix the javadoc on line 227?

  /**
     * @return the main digest algorithm, defaults to sha-1
     */
@pjfanning

This comment has been minimized.

Copy link
Contributor

pjfanning commented Jan 9, 2018

@kiwiwings Does this change in default Digest Algorithm look ok to you?

@Alain-Bearez
Copy link
Contributor

Alain-Bearez left a comment

Did you test this change with existing legacy files?

@kiwiwings

This comment has been minimized.

Copy link
Contributor

kiwiwings commented Jan 9, 2018

I'm not sure what to recommend ...

  • SHA256/512 seems to be usable without JCE jurisdiction policy files, but I guess for RSA you'll need anyway the policy files and bouncycastle ... i.e. I haven't tried it myself now ...
  • setting this by default might have unexpected results on people using the old default.
  • it's easy to set it explicit over the SignatureConfig
  • as usual, we should stick with the defaults Office uses (yes, this can be modified via the ADM templates ...)

So I give it a "0" - I won't commit it, if anyone else does ... I don't mind

@jornfranke

This comment has been minimized.

Copy link
Author

jornfranke commented Jan 10, 2018

@pjfanning

This comment has been minimized.

Copy link
Contributor

pjfanning commented Jan 10, 2018

+1 to updating the docs to show how users can avoid SHA-1
I'd still favour adding the code change if it isn't too disruptive (especially since the next release is a major release).
The unit tests all pass with this change. I don't know if we have enough test coverage in this area for this to be regarded as a useful metric. I'm guessing but I would expect that any files that we read will have the hash algorithm specified in the file itself - so this mean the default shouldn't affect reads. In cases where users are writing files, then with the changed default, they might find that some tools can't read the new file. In this case, it is still possible for a user to modify their code to set the hash algorithm for the write to be SHA-1.

@pjfanning

This comment has been minimized.

Copy link
Contributor

pjfanning commented Jan 22, 2018

If there are no objections, I will merge this change.
I think 4.0.0 is a good place to make changes like this.

@asfgit asfgit closed this in a6faf34 Jan 26, 2018

@kiwiwings

This comment has been minimized.

Copy link
Contributor

kiwiwings commented Feb 18, 2018

To repeat Alain question ... Have you tried that on legacy files?
I've done it and spend some time on a fix for a SO question ... SHA256 leads to a file which Powerpoint 2016 rejects because of an invalid signature. Setting it back to SHA1 makes it working again.
I'm now searching for the cause, to make it eventually work with SHA256 - maybe it's my key/cert, maybe something other is not configured right, maybe something on my windows machine has to be enabled - but if you can't prove otherwise, I'll revert that for POI 4.0.0

@jornfranke

This comment has been minimized.

Copy link
Author

jornfranke commented Feb 18, 2018

@kiwiwings

This comment has been minimized.

Copy link
Contributor

kiwiwings commented Feb 18, 2018

Ok it seems to be partly a false alarm - I've used the Office ADM templates to set the hash algorithm there and compared the generated sig1.xml. So now I have a test case (a) with sha256 which is based on a pkcs12/pfx file and one (b) that reads the same certificate from the windows cert store.
(a) is accepted by Powerpoint 2016 and only differs in the SignatureValue entry with (b), i.e. all other parts of sig1.xml are the same.

An intermediate fix can be found here

I'll keep searching for the difference ...

/* Update - 19.02.2018 */
In a small test without all that POI stuff ... :
if the keys (PFX / MS-Capi) are used to encrypt the same digest, I'll get a different response when using the Cipher.doFinal() method, but when using the Signature API they are the same. So I check now, if the code can be changed along the lines of this SO example, i.e. the hash magic in POI seems to be the DER-encoded digestAlgorithm

/* Update - 26.02.2018 */
The SunMSCAPI kept me busy the last week - I found a SO post which describes the problems which I've faced. So finally I have a solution for SunMSCAPI by using the Signature API - the existing approach by calculating the hash separately and then using the RSA cipher simply doesn't work with the SunMSCAPI, i.e. the encrypted bytes can't be decrypted by the public but only with the private key, which is not correct.
I will finish now the implementation, but at least the blocker is now solved ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.