[jitsi-dev] [libjitsi] Rework encryption / openssl offloading (#97)


#1

just need to test F8 mode a bit more
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Fix OpenSSLHMAC with recent libcrypto
  * Don't export HMAC_CTX_cleanup() in jnopenssl lib
  * Remove OpenSSLDigest
  * Rework SRTPCipherF8 to hide the BlockCipher into it
  * Rework SRTPCipherCTR to hide CipherBlock and getCipherStream
  * Rewite SRTPCipherCTR process
  * Improve SRTPCipherF8.processBlock a bit
  * Make SRTPCipherCTR an abstract class and
  * Fix HMAC.c include
  * Add SRTPCipherCTROpenSSL, use it by default if available
  * Introduce OpenSSLWrapperLoader and use it
  * Drop OpenSSLBlockCipher, we now have SRTPCipherCTROpenSSL

-- File Changes --

    D src/native/openssl/BlockCipher.c (303)
    D src/native/openssl/BlockCipher.h (101)
    D src/native/openssl/Digest.c (151)
    D src/native/openssl/Digest.h (77)
    M src/native/openssl/HMAC.c (14)
    M src/native/openssl/HMAC.h (8)
    A src/native/openssl/OpenSSLWrapperLoader.c (20)
    A src/native/openssl/OpenSSLWrapperLoader.h (21)
    A src/native/openssl/SRTPCipherCTROpenSSL.c (92)
    A src/native/openssl/SRTPCipherCTROpenSSL.h (45)
    M src/org/jitsi/impl/neomedia/transform/srtp/AES.java (20)
    M src/org/jitsi/impl/neomedia/transform/srtp/BaseSRTPCryptoContext.java (31)
    M src/org/jitsi/impl/neomedia/transform/srtp/CryptoBenchmark.java (7)
    M src/org/jitsi/impl/neomedia/transform/srtp/HMACSHA1.java (54)
    D src/org/jitsi/impl/neomedia/transform/srtp/NIOBlockCipher.java (51)
    D src/org/jitsi/impl/neomedia/transform/srtp/OpenSSLBlockCipher.java (390)
    D src/org/jitsi/impl/neomedia/transform/srtp/OpenSSLDigest.java (323)
    M src/org/jitsi/impl/neomedia/transform/srtp/OpenSSLHMAC.java (146)
    A src/org/jitsi/impl/neomedia/transform/srtp/OpenSSLWrapperLoader.java (60)
    D src/org/jitsi/impl/neomedia/transform/srtp/SHA1.java (87)
    M src/org/jitsi/impl/neomedia/transform/srtp/SRTCPCryptoContext.java (31)
    M src/org/jitsi/impl/neomedia/transform/srtp/SRTPCipherCTR.java (108)
    A src/org/jitsi/impl/neomedia/transform/srtp/SRTPCipherCTRJava.java (112)
    A src/org/jitsi/impl/neomedia/transform/srtp/SRTPCipherCTROpenSSL.java (99)
    M src/org/jitsi/impl/neomedia/transform/srtp/SRTPCipherF8.java (105)
    M src/org/jitsi/impl/neomedia/transform/srtp/SRTPCryptoContext.java (31)

-- Patch Links --

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

···

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


#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/97#issuecomment-191945307


#3

just added unit tests for F8 and CTR, you can review

···

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


#4

I need some more time to review this; in the meantime, could you please fix the formatting (e.g. curly braces on new lines)? Thanks.

···

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


#5

I think i've fixed all formating issues
I've added arguments checks (throwing IllegalArgumentException)

···

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


#6

          */
- f8Cipher.processBlock(iv, 0, f8ctx.ivAccent, 0);
+ cipher.init(true, new KeyParameter(encKey));

Good catch!

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/233f6e2c7822751a774a7d2c2c59a16761b9f0cb..5f98e4d4e3c1fa8b1b93ca23e236c6dacb6ffe41#r56245197


#7

      *
- * @param out byte array holding the output cipher stream
- * @param length length of the cipher stream to produce, in bytes
- * @param iv initialization vector used to generate this cipher stream
+ * @param data
+ * @param off
+ * @param len
+ * @param iv initial value of the counter (this value is modified)
      */

Please document the parameters and include the reference to the RFC. Say something about the cipher stream being computed and used directly.

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/c8c0954dd7b2a377848d26c8596657906135ae3d..512864c2ba04c1f2574eaefdf04024b3fa40bcc8#r56252212


#8

@@ -0,0 +1,92 @@
+#include "SRTPCipherCTROpenSSL.h"

The copyright header must be added here too.

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/69eb551924ebd79519d486500336f3604b13a30e..ae3887e88f15169ca4e905db074a9f5fb0d7b848#r56252630


#9

      *
- * @param out byte array holding the output cipher stream
- * @param length length of the cipher stream to produce, in bytes
- * @param iv initialization vector used to generate this cipher stream
+ * @param data
+ * @param off
+ * @param len
+ * @param iv initial value of the counter (this value is modified)
      */

Github says the diff is outdated, true, but: SRTPCipherCTRJava's class doc would be more appropriate on the (new) base class. Then say something about what SRTPCipherCTRJava does different/specialized from its base.

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/c8c0954dd7b2a377848d26c8596657906135ae3d..512864c2ba04c1f2574eaefdf04024b3fa40bcc8#r56255610


#10

There's a few cosmetics left with Javadoc and copyrights, but otherwise this looks okay to me. There might be an incompatibility with OpenSSL < 1.0.1, but Ubuntu has 1.0.1something since at least 12.04 so that I wouldn't worry about it.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-197060502


#11

      *
- * @param out byte array holding the output cipher stream
- * @param length length of the cipher stream to produce, in bytes
- * @param iv initialization vector used to generate this cipher stream
+ * @param data
+ * @param off
+ * @param len
+ * @param iv initial value of the counter (this value is modified)
      */

that's what i was doing already (```@inheritDoc```)
I've updated the class JavaDoc

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/c8c0954dd7b2a377848d26c8596657906135ae3d..512864c2ba04c1f2574eaefdf04024b3fa40bcc8#r56312684


#12

          */
- f8Cipher.processBlock(iv, 0, f8ctx.ivAccent, 0);
+ cipher.init(true, new KeyParameter(encKey));

using 1 cipher instead of 2 ?

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/233f6e2c7822751a774a7d2c2c59a16761b9f0cb..5f98e4d4e3c1fa8b1b93ca23e236c6dacb6ffe41#r56312827


#13

@@ -0,0 +1,92 @@
+#include "SRTPCipherCTROpenSSL.h"

done

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/69eb551924ebd79519d486500336f3604b13a30e..ae3887e88f15169ca4e905db074a9f5fb0d7b848#r56312860


#14

we can support OpenSSL 1.0.0 if needed (EVP_aes_128_ctr is not there but the CTR impl is there if i remenber right)
Don't forget to rebuild libjnopenssl if you merge

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-197257877


#15

          */
- f8Cipher.processBlock(iv, 0, f8ctx.ivAccent, 0);
+ cipher.init(true, new KeyParameter(encKey));

Ah, now I see it. Was too late yesterday :slight_smile:

···


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97/files/233f6e2c7822751a774a7d2c2c59a16761b9f0cb..5f98e4d4e3c1fa8b1b93ca23e236c6dacb6ffe41#r56315466


#16

I don't think OpenSSL 1.0.0 is needed. @lyubomir: can you merge and then build the JNI lib? I don't have access to an old Debian.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-197263200


#17

@ibauersachs, thank you very much! I'll try to build the JNI lib.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-197357947


#18

any updates?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-200415752


#19

@champtar, I have this in my TODO list. I'm afraid I'm very busy at work with a delivery due at the end of the month though.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-200421672


#20

just to be clear: we are just waiting for someone to build the JNI? @damencho could you please do this?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/97#issuecomment-200439822