SEC-2778: Support Encryptors without needing to download JCE extensions #2917

Closed
spring-issuemaster opened this Issue Dec 3, 2014 · 13 comments

Projects

None yet

5 participants

@spring-issuemaster

Dave Syer (Migrated from SEC-2778) said:

Using the crypto module (AesBytesEncryptor in particular) requires the user to download JCE extension policy jars and install them in the JRE, which is a bit of a barrier to entry for some people. It would be good if there was a bouncy castle alternative, or a system property we can set or something (maybe already there)?

@spring-issuemaster

Michael Tecourt said:

Huge +1.

Aligning Spring Security crypto's defaults with the JRE's would really help.

According to Luke Taylor :

The current design of the crypto classes is intended to provide a simple set of canned utilities to enable best-practice for use in encryption, hashing, rather than a comprehensive set of classes which are fully configurable with any algorithm spec [...]
Unfortunately those users who do need canned utilities to apply security best practices are the ones confused when it comes to installing add-ons to the JRE. It's especially confusing in Spring Boot where users particulary expect stuff to just work out of the box.

In practice, the following change would suffice in class org.springframework.security.crypto.encrypt.AesBytesEncryptor :

    public AesBytesEncryptor(String password, CharSequence salt, BytesKeyGenerator ivGenerator) {
        // Hard coded "256" replaced by "128"
        PBEKeySpec keySpec = new PBEKeySpec(password.toCharArray(), Hex.decode(salt), 1024, 128);

The following issues stem from the fact that the default helpers use AES with 256-bit keys while the JVM restricts it to 128-bit keys by default :

@spring-issuemaster

Dave Syer said:

Good points. However, I don't think defaulting to 128-bit keys could be described as "encouraging best practice", which is still definitely one of the main goals of spring security crypto. I don't have a better idea though. Oracle should just end the nonsense and package the JCE extensions with the JVM.

@spring-issuemaster

Michael Tecourt said:

Before Oracle ends the nonsense, "AES-128 provides more than enough security margin for the forseeable future" ... Is it still true 6 years later ?

https://www.schneier.com/blog/archives/2009/07/another_new_aes.html

@tfnico
tfnico commented Mar 23, 2016

@dsyer: As Michael Tecourt noted, 128 bit AES is still practically unbreakable (also according to more recent comments under Schneier's old blog post):

The fact is that I am writing right now in 2015, six years later, and there has been zero progress in showing AES is anywhere even close to be broken.
What progress has been made in 6 years that weakens AES in any practical way? Nothing. Zero. Well, except biclique attack that requires 2^88 bits of data (38 trillion terabytes) and weakens AES 128 to still impractical 2^126, Even that attack ism given amount of data required, is probably slower in practice than brute force.

One thing is that the default Spring Crypto behavior makes it difficult to manage JRE configurations (sometimes even impossible). There are enough people complaining about this problem in this issue tracker and on SO.

Another thing is that it might be straight out illegal in some cases, ref http://crypto.stackexchange.com/questions/20524/why-there-are-limitations-on-using-encryption-with-keys-beyond-certain-length/20525#20525 - I doubt Oracle will change anything about the JVM in the foreseeable future, especially in these times of Apple vs US and so forth.

@dsyer
Member
dsyer commented Mar 23, 2016

You're preaching to the converted here. I'm not sure how we could downgrade the AES default without breaking existing apps, though, so there's a problem with switching to 128 as the default.

@tfnico
tfnico commented Mar 23, 2016

@dsyer I would be happy if there was just an easy way to use 128 bit. Perhaps another factory method in Encryptors.

@rwinch
Member
rwinch commented Mar 23, 2016

@dsyer @tfnico I tend to agree the ship has sailed in terms of ensuring things work with existing applications.

However, I think we can allow better customization of the encryptors. Honestly, I'm not too keen on the approach that was taken in terms of Encryptors.standard, Encryptors.delux, etc. The problem is that cryptography needs to change but we must remain passive. This means that ideally I would be able to update the algorithms that are being used. However we cannot do that as things currently stand.

@tfnico If you would like to submit a PR to make AesBytesEncryptor public and provide some tests, I'd be happy to merge this for 4.1.

@tfnico
tfnico commented Mar 24, 2016

@rwinch +1 on the reasoning there.

As for PRs, my project is actually politically bound to do AES 256 (for no good reason really), so I won't get the work-time to improve anything. I'll moonlight it if I can, but would like to state that this is up for grabs if anyone else wants to have a go at it.

@tfnico tfnico added a commit to tfnico/spring-security that referenced this issue Mar 24, 2016
@tfnico tfnico Introduce EncryptorBuilder to make JCE optional
The default encryptors so far all use a default keyLength of 256, and
doing so is only possible when running in a JRE where the JCE is installed.

At the same time, the Encryptors factory methods are not flexible enough
to account for easy configuration of this. Therefore, introduce the
EncryptorBuilder as a starting point for enveloping the Encryptors with
a more user-friendly interface.

Move CipherAlgorithm to be its own public class as it is exposed for the
EncryptorBuilder.

On the test-side, make the encryption tests take the JCE availability
into account, so 128 is used as keyLength if needed.

Fixes gh-2917
2e6318e
@tfnico
tfnico commented Mar 24, 2016

@rwinch @dsyer OK, so I managed to moonlight a little already. Quite late here, so go easy on me :)

I made a builder that can be used to construct an encryptor. It could be developed further to reflect the old Encryptions completely, or go a different way entirely:

https://github.com/tfnico/spring-security/blob/2e6318e26757bf66f7614373690861517746391b/crypto/src/main/java/org/springframework/security/crypto/encrypt/EncryptorBuilder.java

I then re-shaped the test into using that builder so the tests will run wth 128 keyLenght on a JRE where there is no JCE, or with 256 as usual if the JCE is there.

https://github.com/tfnico/spring-security/blob/2e6318e26757bf66f7614373690861517746391b/crypto/src/test/java/org/springframework/security/crypto/encrypt/EncryptorsTests.java

The commit is here: tfnico@2e6318e

@tfnico
tfnico commented Mar 27, 2016

On a half-related note, I put together a little desktop tool for playing around with the encryptors: https://github.com/tfnico/encryptomania

As you can imagine, having more configurable encryptors would make the tool look a lot more interesting :)

@william-tran
Member

I would love to see a bouncycastle AES256 as default if we're weary of making AES128 default. The hard part is doing the AES256 properly with BouncyCastle Lightweight API, which is quite a bit lower level.

@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 1, 2016
@william-tran william-tran BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
7d38949
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 1, 2016
@william-tran william-tran BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
a428d96
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 1, 2016
@william-tran william-tran BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
1a6ef73
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 1, 2016
@william-tran william-tran BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
86b4adb
@rwinch rwinch added a commit to rwinch/spring-security that referenced this issue Apr 1, 2016
@william-tran @rwinch william-tran + rwinch BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
43045bf
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 1, 2016
@william-tran william-tran BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
1e1eb8e
@rwinch rwinch added a commit to rwinch/spring-security that referenced this issue Apr 1, 2016
@william-tran @rwinch william-tran + rwinch BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
e9c5370
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 1, 2016
@william-tran william-tran BouncyCastle implementation of "AES/CBC/PKCS5Padding"
Fixes #2917
1358da4
@william-tran
Member

Sorry for all the force-push spam. On a different note, because of the performance optimizations in JCE, should we try to see if that's available, and if not, use the BC implementation for Encryptors.standard()?

@rwinch
Member
rwinch commented Apr 1, 2016

@william-tran Thanks for the comment. We cannot make this the default because we need to ensure we are passive. Changing the crypto provider out from under people could possibly break FIPS compliance (i.e. JCE is compliant and BC is not AFAIK) for someone using the defaults and who just forgot to setup JCE on a new box.

@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 4, 2016
@william-tran william-tran Bouncy Castle implementations of AES-256.
Implments "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
2ba1b9d
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 4, 2016
@william-tran william-tran Bouncy Castle implementations of AES-256.
Implments "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
44af995
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 6, 2016
@william-tran william-tran Bouncy Castle implementations of AES-256.
Implments "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
cf829a1
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 13, 2016
@william-tran william-tran Bouncy Castle implementations of AES-256.
Implments "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
b6d8e28
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 13, 2016
@william-tran william-tran Bouncy Castle implementations of AES-256.
Implments "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
9b0a758
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 13, 2016
@william-tran william-tran Bouncy Castle implementations of AES-256.
Implments "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
b7b4507
@rwinch rwinch closed this in #3779 Apr 13, 2016
@rwinch rwinch added a commit that referenced this issue Apr 13, 2016
@william-tran @rwinch william-tran + rwinch Bouncy Castle implementations of AES-256
Adds "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

Fixes gh-2917
63b2cfe
@rwinch rwinch added this to the 4.1.0 RC2 milestone Apr 13, 2016
@rwinch rwinch self-assigned this Apr 13, 2016
@william-tran william-tran added a commit to william-tran/spring-security that referenced this issue Apr 15, 2016
@william-tran william-tran Only use methods present in Bouncy Castle 1.47.
This forces us to avoid using CipherOutputStream, and instead use the
BlockCiphers directly. As an extra measure for correctness, test the
equivalence of the BC implementations against data sizes from 1 to 2048
bytes.

Fixes gh-2917
f3e3a5a
@rwinch rwinch added a commit that referenced this issue Apr 18, 2016
@william-tran @rwinch william-tran + rwinch Bouncy Castle 1.47 Support
This forces us to avoid using CipherOutputStream, and instead use the
BlockCiphers directly. As an extra measure for correctness, test the
equivalence of the BC implementations against data sizes from 1 to 2048
bytes.

Fixes gh-2917
b014372
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment