[jitsi-dev] [libjitsi] Generate RSA keypair / certificate only once in DtlsControlImpl (#98)


#1

security implications need to be reviewed carrefully!!
I don't see why we generate a new RSA keypair and certificate for each dtls conn when on a web server we change them every year or more. Is it ok to share the cert for all connections ?

Top 10 functions is now (this includes some unmerged patch):
(t 6,0,s 6,0) java.lang.Object.wait
(t 5,0,s 5,0) java.net.PlainDatagramSocketImpl.send
(t 3,1,s 3,0) java.net.PlainDatagramSocketImpl.receive0
(t 1,5,s 1,5) org.jitsi.impl.neomedia.transform.srtp.OpenSSLHMAC.HMAC_Update
(t 2,5,s 1,1) java.util.concurrent.locks.AbstractQueuedSynchronizer.release
(t 1,7,s 1,1) org.ice4j.socket.MultiplexingXXXSocketSupport.acceptBySocketsOrThis
(t 1,0,s 1,0) java.lang.Object.notifyAll
(t 1,0,s 1,0) java.util.concurrent.locks.ReentrantLock$Sync.tryRelease
(t 1,0,s 1,0) sun.misc.Unsafe.park
(t 1,0,s 1,0) org.jitsi.impl.neomedia.transform.srtp.SRTPCipherCTROpenSSL.AES128CTR_CTX_process
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/libjitsi/pull/98

-- Commit Summary --

  * Generate RSA keypair / certificate only once in DtlsControlImpl

-- File Changes --

    M src/org/jitsi/impl/neomedia/transform/dtls/DtlsControlImpl.java (44)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/98.patch
https://github.com/jitsi/libjitsi/pull/98.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98


#2

Can one of the admins verify this patch?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-192221708


#3

Downvote.

The purpose of using Jitsi is to have a user-respecting, secure communication tool. The purpose of the server is to serve the user, who is seeking secure communication.

We need strong keys that are rotated frequently. Libjitsi currently does this, and following the RFCs mentioned in the text of the file changed.

There are practical considerations but those are secondary to the premise of weakening security.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-192499662


#4

Downvote.

The purpose of using Jitsi is to have a user-respecting, secure communication tool. The purpose of the server is to serve the user, who is seeking secure communication.

Can you please elaborate on how using a single certificate per videobridge run would compromise the security? This is an honest question, I'm trying to understand it.

My very vague understanding is that if DTLS is used in one of the modes which provide perfect forward secrecy, a compromised certificate cannot be used to retroactively obtain any of the sessions keys. And if that's the case, generating a new certificate for each session doesn't seem to increase security in any way. I hesitate to state this, even with the caveat above, so please don't quote me, and please correct me if I am wrong.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-192519689


#5

if DTLS is used in one of the modes which provide perfect forward secrecy, a compromised certificate cannot be used to retroactively obtain any of the sessions keys

The thought was that frequent key rotation would be better than infrequent, and any moves to go to less frequent to save a little server power would be moving backwards. But if forward secrecy is guaranteed to be used, maybe it is not a big deal.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-192549727


#6

I just rechecked the code and the cert is generated for 6days only, so we can't merge this 'fix' (will rework on it on Monday).
I agree we should rotate key often, but it would be daily for me, not per connection.

For the strong key part, we are only using 1024 bits key, and we are not using SecureRandom().
Fortuna might be a good CSPRNG, but i'm not sure our implementation as been reviewed/audited, and I really prefer to only use code written by cryptographers and often audited like Bouncy Castle.
RSAKeyGenerationParameters needs a SecureRandom as second argument and Bouncy Castle doesn't provide any SecureRandom implementation, which for me means Bouncy Castle author want java.security.SecureRandom.

If we regenerate keypair daily, blocking SecureRandom() will not be a problem, and recent java (jdk8) use /dev/urandom, so no more blocking.
A really good reading (many links included):
https://tersesystems.com/2015/12/17/the-right-way-to-use-securerandom/

Also DTLS supports PFS cipher suite, but i'm not sure we are enforcing only these suites (haven't checked)

To conclude i will rework on this patch on monday
Regards

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-192625279


#7

Upvote @champtar previous comment.

Please also let us know your thoughts on https://github.com/jitsi/libjitsi/pull/67

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-192670586


#8

Hi,
Just updated this PR, I'm still not sure it's ready to merge, ie the fortuna change might need some discussion, but i'm on vacation starting now for one week :slight_smile:

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193356320


#9

@champtar is the change to /dev/urandom independent of the certificate generation? If so, could you please split it in its own PR?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193358807


#10

This looks good to me! I do think that the 24h rotating should address any practical security concerns.
Thanks very much @champtar !
Let's get this in.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193379278


#11

Also, I don't think we're enforcing cipher suites with PFS. If we accept this, we should at least make sure we enforce that.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193389665


#12

My previous message was destined to @champtar, I'm sorry for not mentioning you in the first place.

In summary, if we only support PFS suites, and we use an audited/reviewed SecureRandom implementation and we regenerate the RSA key daily, then even with an RSA key of 1024bits, we should be fine.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193394796


#13

For now Fortuna is initialy seeded with Random instead of SecureRandom ... and I haven't reviewed the constant seeding (hope it work on VM)

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193396244


#14

If Fortuna is removed here, then why extend SecureRandom at all?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193407783


#15

generateSeed calls /dev/random, nextBytes calls /dev/urandom, I'm extending SecueRandom to force the use of /dev/urandom to keep old non blocking behaviour. With more testing/review we can remove this hack or add a configuration.
Once kernel pool is initialized, using /dev/urandom is as safe as /dev/random, but we have no way too know that the pool has been initialized (see also kernel getrandom())

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193420127


#16

BTW i've checked SecureRandom behaviour with java 6 7 8 on a CentOS 6 VM

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193425035


#17

It's been a while since I looked at SecureRandom's source, but I have something in mind that it calls getSeed anyway if it hasn't been seeded before. Anyway, there is a (system) property to force the usage uf urandom (by setting e.g. /dev/../dev/urandom to trick Java). Can't we set that in jvb.sh?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193429878


#18

@ibauersachs https://bugs.openjdk.java.net/browse/JDK-6202721

@gpolitis If we're worried about the length of time it takes to generate a key and therefore reducing key generation to once a day instead of every connection, why not make it stronger than 1024 bits?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-193512650


#19

I'm working on merging https://github.com/champtar/libjitsi/commit/e13d0471a5c11ec187cf32c02289a46d82811bdd now because I need it to speed up the connection time between Meet and Videobridge.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-195796517


#20

Hi @lyubomir,
Still on vacation till wednesday, just merge the RSA cache commit (or another version, i really don't mind the ownership) and i will open another PR with the fortuna removal commit, and another PR to enforce PFS if you don't do it

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/98#issuecomment-195808773