Skip to content
This repository has been archived by the owner on Jul 3, 2020. It is now read-only.

Bouncy Castle 1.65 instead of 1.49 #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bouncy Castle 1.65 instead of 1.49 #32

wants to merge 1 commit into from

Conversation

Neustradamus
Copy link

@cobratbq
Copy link

This will most likely cause problems:

The default parameter sizes for DH and DSA are now 2048. If you have been relying on key pair generation without passing in parameters generated keys will now be larger. -- BouncyCastle release notes

It will cause protocol violations for otr4j implementations, as they did not hardcode default lengths for DSA signatures.

@Neustradamus
Copy link
Author

This will most likely cause problems:

The default parameter sizes for DH and DSA are now 2048. If you have been relying on key pair generation without passing in parameters generated keys will now be larger. -- BouncyCastle release notes

It will cause protocol violations for otr4j implementations, as they did not hardcode default lengths for DSA signatures.

Note: It is since Bouncy Castle 1.57.

We can already done an update from 1.49 to 1.56.

@cobratbq
Copy link

For completeness, I believe I answered you on a different channel already - at least partially:

  1. True, as of version 1.57 indeed.
  2. Yes, upgrades up to 1.56 should go alright. (At least, I do not recall running into such nasty issues.)
  3. Necessary changes in code should be in the crypto package, probably primarily OtrCryptoEngine (+ -Impl). I seem to recall that SMP-related code wasn't an issue as it does not use DSA, but I'm not completely sure anymore.

ibauersachs added a commit that referenced this pull request Jun 9, 2020
ibauersachs added a commit that referenced this pull request Jun 9, 2020
@ibauersachs ibauersachs mentioned this pull request Jun 9, 2020
@ibauersachs
Copy link
Owner

@cobratbq Hey :-)
Do we even need BouncyCastle in otr4j? The only relevant usage is in net.java.otr4j.crypto.OtrCryptoEngineImpl and from what I can tell, this is all basic JCE crypto: a bit of AES and DSA.

The problem with key sizes are in implementations of OtrEngineHost. Given that 1024bit keys are insecure anyway, I'd be willing to break those intentionally. See also #34

@cobratbq
Copy link

cobratbq commented Jun 9, 2020

@cobratbq Hey :-)

Hi!

Do we even need BouncyCastle in otr4j? The only relevant usage is in net.java.otr4j.crypto.OtrCryptoEngineImpl and from what I can tell, this is all basic JCE crypto: a bit of AES and DSA.

I remember there being comments about compatibility issues. I have not checked about removing BC entirely, that could also work.

The problem with key sizes are in implementations of OtrEngineHost. Given that 1024bit keys are insecure anyway, I'd be willing to break those intentionally. See also #34

Yes, I misremembered that at first. Indeed DSA keypair generation is left as an exercise for the user, in this case Jitsi. The 1024 bit key length - or more precisely the 20 bytes/160 bits public parameter q requirement - is mandated by spec, though. So we cannot deviate from that.

@ibauersachs
Copy link
Owner

@cobratbq Hey :-)

Hi!

Do we even need BouncyCastle in otr4j? The only relevant usage is in net.java.otr4j.crypto.OtrCryptoEngineImpl and from what I can tell, this is all basic JCE crypto: a bit of AES and DSA.

I remember there being comments about compatibility issues. I have not checked about removing BC entirely, that could also work.

Wasn't that some crap about BouncyCastle vs. SpongyCastle (Android)? If it runs on plain Java 8, I see no reason to keep BouncyCastle.

The problem with key sizes are in implementations of OtrEngineHost. Given that 1024bit keys are insecure anyway, I'd be willing to break those intentionally. See also #34

Yes, I misremembered that at first. Indeed DSA keypair generation is left as an exercise for the user, in this case Jitsi. The 1024 bit key length - or more precisely the 20 bytes/160 bits public parameter q requirement - is mandated by spec, though. So we cannot deviate from that.

Fine, but then we can set that in KeyPair/KeyFactory.init in Jitsi.

@cobratbq
Copy link

cobratbq commented Jun 9, 2020 via email

@ibauersachs
Copy link
Owner

Sure, that will work. The only reason for keeping BC for this situation is better/simpler API.

I already replaced one part of it, what was ~30 lines of BC calls and conversion to regular JCE is now 5 lines, including the try-catch.

I'm not sure if we use that. If not, then no reason at all. Unless JDK complains about 1024 bit DSA keys being weak already. Can't remember from the top of my head.

It probably does for TLS, but not if you call the APIs directly.

Anyway, pointless. I missed #33.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants