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

Invalid license digest with multiple RSA service providers #53

Closed
mauriceackel opened this issue Feb 12, 2019 · 8 comments
Closed

Invalid license digest with multiple RSA service providers #53

mauriceackel opened this issue Feb 12, 2019 · 8 comments

Comments

@mauriceackel
Copy link

mauriceackel commented Feb 12, 2019

Today I encountered, that as I loaded a certain library (i.e. icepdf), suddenly my otherwise valid license was no longer recognized as properly signed. As I dug through the code, the issue appears to be originating from the bouncy castle library, which is included and loaded by icepdf.

In particular, the issue encounters, because in the License class on line 140, the following function call
final var cipher = Cipher.getInstance(key.getAlgorithm());
Does no longer return the "SunJCE: Cipher.RSA -> com.sun.crypto.provider.RSACipher" but instead, the "BC: Cipher.RSA -> org.bouncycastle.jcajce.provider.asymmetric.rsa.CipherSpi$NoPadding" Cipher.

The latter decrypts the signature in a substantially different way as the first, native Cipher implementation as it "padds" the decrypted digest to 255 bytes. See the following example:

Decrypted signature with SunJCE: Cipher.RSA (Also the message digest generated by the digester):
[-59, -25, -68, -101, 12, -88, 80, 0, 59, 69, -43, 47, 94, 28, -12, 72, 34, 48, 97, -71, 30, -19, 113, -117, 96, 49, -39, -106, -98, -120, -90, -80, -114, 87, 124, 30, -16, 3, 61, 71, -13, -92, 118, -54, 16, -7, 105, 19, 48, 46, 59, 48, 120, 54, -12, 97, -21, -75, -21, 73, 50, -26, 18, -34]

Decrypted signature with BC: Cipher.RSA:
[1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, -59, -25, -68, -101, 12, -88, 80, 0, 59, 69, -43, 47, 94, 28, -12, 72, 34, 48, 97, -71, 30, -19, 113, -117, 96, 49, -39, -106, -98, -120, -90, -80, -114, 87, 124, 30, -16, 3, 61, 71, -13, -92, 118, -54, 16, -7, 105, 19, 48, 46, 59, 48, 120, 54, -12, 97, -21, -75, -21, 73, 50, -26, 18, -34]

Due to this, the following array comparison (Line 143)
return Arrays.equals(digestValue, sigDigest);
will FAIL and the license will be marked unsigned.

At this point, I'm not quite sure how to fix this issue, but a reasonable approach would be to enforce the selection of Sun's Cipher.RSA, no matter which other crypto libraries are loaded.

Cheers
Maurice

@mauriceackel
Copy link
Author

mauriceackel commented Feb 12, 2019

After further research, according to [this] (https://stackoverflow.com/a/32033937) StackOverflow thread, it is bad practice to instantiate a Cipher by using Cipher.getInstance("RSA") (which is currently done), as this is no fully qualified cypher string.

Instead, it is recommended to use a cipher string defined in the official [Java Docs] (https://docs.oracle.com/javase/10/docs/api/index.html?javax/crypto/Cipher.html).

I tested the fully qualified cipher string "RSA/ECB/PKCS1Padding" and the result, although the security provider is still bouncy castle, does not include the padding shown above.

In the end, iI think this melts down to the fact, that instead of using the algorithm, retrieved by the public key, a static cipher qualifier should be used. However, as I don't know the code thoroughly, this may limit any flexibility for using other asymmetric encryption algorithms.

Except from that, a solution could also be to not compare the digest and the decrypted signature 1:1 but instead to "search" the digest in the decrypted signature. This way, as long as the result of the decryption contains the digest somewhere, the license will be marked as signed. This might also be a quick fix approach to ensure the correct behavior in the short run!

@verhas
Copy link
Owner

verhas commented Feb 13, 2019

Could you help me disclosing which version of License3j is this ticket related to?

@mauriceackel
Copy link
Author

mauriceackel commented Feb 13, 2019

I’m using License 3j Version 3.0.1 in a Java 11 environment.

@verhas
Copy link
Owner

verhas commented Feb 13, 2019

I had some time to read your ticket in detail and I feel like I understand the issue.

First and foremost: I do not agree with the quick fix you propose. It would be a workaround for one situation and does not solve the problem.

The problem itself is that the algorithm specification is not fully qualified and thus leaves room for the actual implementation to execute it at its discretionary decision. That way the BC implementation that was on the classpath in your test implemented the decoding differently than the standard implementation.

License3j DOES NOT USE any algorithm specification in its core. It is only the REPL application that uses RSA as a default value for the algorithm specification string. It seems that this default is not the best choice and I will change it to RSA/ECB/PKCS1Padding in the next release that will already feature the REPL application as a separate JAR.

Thanks for the ticket and the analysis, I have learned a lot from your ticket.

@verhas
Copy link
Owner

verhas commented Feb 13, 2019

I tried it (no BC on the classpath):

[WARNING] No console in the system
License3j REPL application
CDW is C:\Users\Peter_Verhas\Dropbox\github\License3jRepl\.
type 'help' for help
Startup file .license3j was not found.
L3j> $ help gen
generateKeys [algorithm=RSA/ECB/PKCS1Padding] [size=2048] [format=BINARY*|BASE64] public=xxx private=xxx
Generate public and private keys and save them into files.
You can specify the algorithm, key size and the format. The defaults are RSA, 2048 and BINARY.
You should specify the file names using the parameters 'public' and 'private'.
The keys remain in the memory and can be used to sign and verify license.

No license in memory
No keys in memory.
L3j> $ gen algorithm=RSA/ECB/PKCS1Padding public=pub.key private=priv.key
[ERROR] Algorithm RSA/ECB/PKCS1Padding is not handled by current version of this application.
L3j> $ 
L3j> $ 

@verhas
Copy link
Owner

verhas commented Feb 13, 2019

OK.

Now I understand. The key generation is RSA but using it has to define the other parameters.

I will have a look at how to incorporate that into the code.

@mauriceackel
Copy link
Author

Thank you for digging into this! Glad I could support the project. I'm looking forward to the next release. Keep up the good work!

@verhas
Copy link
Owner

verhas commented Feb 13, 2019

I created a fix that you can see on the master branch version 3.1.0-SNAPSHOT.

With this modification when you generate the key only the algorithm is taken into account from the transformation specification string but the whole string is stored in the keys when they are converted to byte array and saved to file. When signing and verifying is executed this string is used.

@verhas verhas closed this as completed Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants