[jitsi-dev] Re: [jitsi~svn:9015] New ZRTP library that supports trusted MitM/PBX feature and implement SRT


#1

Hey

Some (picky) comments inline, and:
Could you please format the code according as the rest of Jitsi (brackets on its own line)? (Not that I like that style very much, but to keep it consistent...)

Thanks again for the effort!

Regards,
Ingo

+package net.java.sip.communicator.impl.neomedia.transform.srtp;
+
+import net.java.sip.communicator.impl.neomedia.*;
+
+import org.bouncycastle.crypto.digests.SHA1Digest;
+import org.bouncycastle.crypto.macs.*;
+import org.bouncycastle.crypto.params.KeyParameter;
+import org.bouncycastle.crypto.params.ParametersForSkein;
+import org.bouncycastle.crypto.BlockCipher;
+import org.bouncycastle.crypto.Mac;
+
+import org.bouncycastle.crypto.engines.AESFastEngine;
+import org.bouncycastle.crypto.engines.TwofishEngine;

Per convention, these should be package imports.

+ public SRTCPCryptoContext(long ssrcIn,
+ byte[] masterK, byte[] masterS, SRTPPolicy policyIn)
+ {

That's exactly the code that I preferred not to duplicate. Anyway...

+ public void transformPacket(RawPacket pkt) {
+ if (policy.getAuthType() != SRTPPolicy.NULL_AUTHENTICATION) {
+ authenticatePacket(pkt, index);
+ pkt.append(rbStore, 4);
+ pkt.append(tagStore, policy.getAuthTagLength());
+ }

Authentication is mandatory for SRTCP, no need to check the policy.

+ public boolean reverseTransformPacket(RawPacket pkt) {
+ if (policy.getAuthType() != SRTPPolicy.NULL_AUTHENTICATION) {

Dito.

+
+ // get original authentication data and store in tempStore
+ pkt.readRegionToBuff(pkt.getLength() - tagLength, tagLength,
+ tempStore);
+
+ // Shrink packet to remove the authentication tag and index
+ // because this is part of authenicated data
+ pkt.shrink(tagLength + 4);
+
+ // compute, then save authentication in tagStore
+ authenticatePacket(pkt, indexEflag);
+
+ for (int i = 0; i < tagLength; i++) {
+ if (tempStore[i] != tagStore[i])
+ return false;
+ }

Simplification of the hash comparison - there's nothing to cut off in a byte.

+ public void processPacketAESCM(RawPacket pkt, int index) {

Rename to processPacketCM - it's not only AES.

+ public void processPacketAESF8(RawPacket pkt, int index) {

Rename to processPacketF8 - it's not only AES.

+ /**
+ * Checks if a packet is a replayed on based on its sequence number.
+ *
+ * This method supports a 64 packet history relative the the given
+ * sequence number.
+ *
+ * Sequence Number is guaranteed to be real (not faked) through
+ * authentication.
+ *
+ * @param index index number of the SRTCP packet
+ * @return true if this sequence number indicates the packet is not a
+ * replayed one, false if not
+ */
+ boolean checkReplay(int index) {

The name does not clearly indicate what return value indicates what:
isReplayedPacket would be more appropriate (but would inverse the boolean values).

+ private void computeIv(byte label) {
+
+ for (int i = 0; i < 14; i++) {
+ ivStore[i] = masterSalt[i];
+ }
+ ivStore[7] ^= label;
+ ivStore[14] = ivStore[15] = 0;
+ }

Where did the key derivation rate go? Apart that the SRTCP index is only 32bit, I think that should be the same as for SRTP. We probably never notice as a KDR has never been used, but anyway.

Index: trunk/src/net/java/sip/communicator/impl/neomedia/transform/srtp/SRTCPTransformer.java

     public RawPacket transform(RawPacket pkt)
     {
+ long ssrc = pkt.GetRTCPSSRC();
+
+ SRTCPCryptoContext context = this.contexts
+ .get(new Long(ssrc));

New Long() !?

+
+ if (context == null) {
+ context = this.engine.getDefaultContextControl().deriveContext(ssrc);
+ if (context != null) {

This check is not necessary - we're in trouble anyway if we can't derive the context.

+ context.deriveSrtcpKeys();
+ contexts.put(new Long(ssrc), context);

New Long()

+ }
+ }
+ if (context != null) {

Dito - if we can't derive a context we should not send out unencrypted packets. (Applies to the SRTPTransformEngine as well)

+ context.transformPacket(pkt);
+ }
         return pkt;
     }

     public RawPacket reverseTransform(RawPacket pkt)
     {
+ long ssrc = pkt.GetRTCPSSRC();
+ SRTCPCryptoContext context = this.contexts.get(new Long(ssrc));

New Long()

+ if (context == null) {
+ context = this.engine.getDefaultContextControl().deriveContext(ssrc);
+ if (context != null) {

Dito.

+ context.deriveSrtcpKeys();
+ this.contexts.put(new Long(ssrc), context);

New Long()

+ }
+ }
+
+ if (context != null) {

Dito.

+ boolean validPacket = context.reverseTransformPacket(pkt);
+ if (!validPacket) {
+ return null;
+ }
+ }
         return pkt;
     }
}

Index: trunk/src/net/java/sip/communicator/impl/neomedia/transform/zrtp/ZRTCPTransformer.java

--- trunk/src/net/java/sip/communicator/impl/neomedia/transform/zrtp/ZRTCPTransformer.java
  (revision 0)
+++ trunk/src/net/java/sip/communicator/impl/neomedia/transform/zrtp/ZRTCPTransformer.java
  (revision 9015)
@@ -0,0 +1,86 @@
+/*
+ * SIP Communicator, the OpenSource Java VoIP and Instant Messaging client.
+ *
+ * Distributable under LGPL license.
+ * See terms of license at gnu.org.
+ */
+package net.java.sip.communicator.impl.neomedia.transform.zrtp;
+
+import net.java.sip.communicator.impl.neomedia.*;
+import net.java.sip.communicator.impl.neomedia.transform.*;
+
+/**
+ * This is currently not supported so packets are not changed.

Not anymore...


#2

Some replies:

- The SRTP and SRTCP code is part of another project (ZRTP4J) and was just
  copy/pasted from that project and only very small modifications done.
  If I modify the source too much (formatting that is) it becomes much harder
  to verfiy/compare future enhancements.

- Your comments regarding "new Long(ssrc)" and why this is necessary: please
  refer to the Java documentation about the hashtable.

- regarding NULL Authentication: libsrtp contains test cases even for null
  authentication - I'm just preparing to have the same test cases to have
  full test/interop coverage. As per 3711 SRTCP must at least use authentication
  but this is IMHO a matter of the client how to setup the policy.

- If key derivtaion rate is never used (at least for ZRTP is defined has 2^48,
  effectively zero in terms of SRTP/SRTCP) then it's not necessary to deal with it.
  As a matter of fact I'll remove the KDR stuff in SRTP (at least in ZRTP4J project)
  in near future. You may have noticed that I have no code in the crypto contexts
  to re-trigger new key derivations based on the 48-bit index.

- I don't understand your comment
   "> Simplification of the hash comparison - there's nothing to cut off in a byte."
  below.

Best regards,
Werner

···

Am 16.10.2011 11:53, schrieb Bauersachs Ingo:

Hey

Some (picky) comments inline, and:
Could you please format the code according as the rest of Jitsi (brackets on its own line)? (Not that I like that style very much, but to keep it consistent...)

Thanks again for the effort!

Regards,
Ingo

+package net.java.sip.communicator.impl.neomedia.transform.srtp;
+
+import net.java.sip.communicator.impl.neomedia.*;
+
+import org.bouncycastle.crypto.digests.SHA1Digest;
+import org.bouncycastle.crypto.macs.*;
+import org.bouncycastle.crypto.params.KeyParameter;
+import org.bouncycastle.crypto.params.ParametersForSkein;
+import org.bouncycastle.crypto.BlockCipher;
+import org.bouncycastle.crypto.Mac;
+
+import org.bouncycastle.crypto.engines.AESFastEngine;
+import org.bouncycastle.crypto.engines.TwofishEngine;

Per convention, these should be package imports.

+ public SRTCPCryptoContext(long ssrcIn,
+ byte[] masterK, byte[] masterS, SRTPPolicy policyIn)
+ {

That's exactly the code that I preferred not to duplicate. Anyway...

+ public void transformPacket(RawPacket pkt) {
+ if (policy.getAuthType() != SRTPPolicy.NULL_AUTHENTICATION) {
+ authenticatePacket(pkt, index);
+ pkt.append(rbStore, 4);
+ pkt.append(tagStore, policy.getAuthTagLength());
+ }

Authentication is mandatory for SRTCP, no need to check the policy.

+ public boolean reverseTransformPacket(RawPacket pkt) {
+ if (policy.getAuthType() != SRTPPolicy.NULL_AUTHENTICATION) {

Dito.

+
+ // get original authentication data and store in tempStore
+ pkt.readRegionToBuff(pkt.getLength() - tagLength, tagLength,
+ tempStore);
+
+ // Shrink packet to remove the authentication tag and index
+ // because this is part of authenicated data
+ pkt.shrink(tagLength + 4);
+
+ // compute, then save authentication in tagStore
+ authenticatePacket(pkt, indexEflag);
+
+ for (int i = 0; i < tagLength; i++) {
+ if (tempStore[i] != tagStore[i])
+ return false;
+ }

Simplification of the hash comparison - there's nothing to cut off in a byte.

+ public void processPacketAESCM(RawPacket pkt, int index) {

Rename to processPacketCM - it's not only AES.

+ public void processPacketAESF8(RawPacket pkt, int index) {

Rename to processPacketF8 - it's not only AES.

+ /**
+ * Checks if a packet is a replayed on based on its sequence number.
+ *
+ * This method supports a 64 packet history relative the the given
+ * sequence number.
+ *
+ * Sequence Number is guaranteed to be real (not faked) through
+ * authentication.
+ *
+ * @param index index number of the SRTCP packet
+ * @return true if this sequence number indicates the packet is not a
+ * replayed one, false if not
+ */
+ boolean checkReplay(int index) {

The name does not clearly indicate what return value indicates what:
isReplayedPacket would be more appropriate (but would inverse the boolean values).

+ private void computeIv(byte label) {
+
+ for (int i = 0; i < 14; i++) {
+ ivStore[i] = masterSalt[i];
+ }
+ ivStore[7] ^= label;
+ ivStore[14] = ivStore[15] = 0;
+ }

Where did the key derivation rate go? Apart that the SRTCP index is only 32bit, I think that should be the same as for SRTP. We probably never notice as a KDR has never been used, but anyway.

Index: trunk/src/net/java/sip/communicator/impl/neomedia/transform/srtp/SRTCPTransformer.java

     public RawPacket transform(RawPacket pkt)
     {
+ long ssrc = pkt.GetRTCPSSRC();
+
+ SRTCPCryptoContext context = this.contexts
+ .get(new Long(ssrc));

New Long() !?

+
+ if (context == null) {
+ context = this.engine.getDefaultContextControl().deriveContext(ssrc);
+ if (context != null) {

This check is not necessary - we're in trouble anyway if we can't derive the context.

+ context.deriveSrtcpKeys();
+ contexts.put(new Long(ssrc), context);

New Long()

+ }
+ }
+ if (context != null) {

Dito - if we can't derive a context we should not send out unencrypted packets. (Applies to the SRTPTransformEngine as well)

+ context.transformPacket(pkt);
+ }
         return pkt;
     }

     public RawPacket reverseTransform(RawPacket pkt)
     {
+ long ssrc = pkt.GetRTCPSSRC();
+ SRTCPCryptoContext context = this.contexts.get(new Long(ssrc));

New Long()

+ if (context == null) {
+ context = this.engine.getDefaultContextControl().deriveContext(ssrc);
+ if (context != null) {

Dito.

+ context.deriveSrtcpKeys();
+ this.contexts.put(new Long(ssrc), context);

New Long()

+ }
+ }
+
+ if (context != null) {

Dito.

+ boolean validPacket = context.reverseTransformPacket(pkt);
+ if (!validPacket) {
+ return null;
+ }
+ }
         return pkt;
     }
}

Index: trunk/src/net/java/sip/communicator/impl/neomedia/transform/zrtp/ZRTCPTransformer.java

--- trunk/src/net/java/sip/communicator/impl/neomedia/transform/zrtp/ZRTCPTransformer.java
  (revision 0)
+++ trunk/src/net/java/sip/communicator/impl/neomedia/transform/zrtp/ZRTCPTransformer.java
  (revision 9015)
@@ -0,0 +1,86 @@
+/*
+ * SIP Communicator, the OpenSource Java VoIP and Instant Messaging client.
+ *
+ * Distributable under LGPL license.
+ * See terms of license at gnu.org.
+ */
+package net.java.sip.communicator.impl.neomedia.transform.zrtp;
+
+import net.java.sip.communicator.impl.neomedia.*;
+import net.java.sip.communicator.impl.neomedia.transform.*;
+
+/**
+ * This is currently not supported so packets are not changed.

Not anymore...


#3

Hey

- The SRTP and SRTCP code is part of another project (ZRTP4J) and was just
  copy/pasted from that project and only very small modifications done.
  If I modify the source too much (formatting that is) it becomes much harder
  to verfiy/compare future enhancements.

As Emil noted, can't we use the ZRTP code directly?
But: for SDES, I need different contexts for forward and reverse transform. To avoid more duplicated code in the sdes package, I made some small modifications in the SRT(C)PTransformer classes to accept two SRTPTransformers. Could we move that to ZRTP4J too?
(Not yet tested (my equipment is at the university) nor committed, see attachments).

Most comments apply to the code anyway, whether it lives in Jitsi or in ZRTP4J.

- Your comments regarding "new Long(ssrc)" and why this is necessary: please
  refer to the Java documentation about the hashtable.

That was necessary until Java 1.5 finally introduced autoboxing. And since we're using generics, Java 1.4 is out of question. The SRTPTransformer does it without new for a long time. Better yet, use HashMap instead of the Hashtable to avoid unnecessary locking.

- regarding NULL Authentication: libsrtp contains test cases even for null
  authentication - I'm just preparing to have the same test cases to have
  full test/interop coverage. As per 3711 SRTCP must at least use authentication
  but this is IMHO a matter of the client how to setup the policy.

Hmm, oke, but it just makes no sense. If the client is required to setup the policy with authentication, why allow it otherwise?

- If key derivtaion rate is never used (at least for ZRTP is defined has 2^48,
  effectively zero in terms of SRTP/SRTCP) then it's not necessary to deal with it.
  As a matter of fact I'll remove the KDR stuff in SRTP (at least in ZRTP4J project)
  in near future. You may have noticed that I have no code in the crypto contexts
  to re-trigger new key derivations based on the 48-bit index.

I'd prefer to have a correct implementation instead of removing it. The other key-exchanges (SDES, MIKEY) include parameters to freely define that rate.

- I don't understand your comment
   "> Simplification of the hash comparison - there's nothing to cut off in a byte."
  below.

byte x = (byte)0x9a;
byte y = x & 0xff

-> y==x, there's nothing to cut off with that mask if the underlying type is already a byte.

Regards,
Ingo

0004-Enable-SRTCP-for-SDES.patch (9.52 KB)

0001-Fix-SRTCP-policy-assignment.patch (1.12 KB)

0002-Add-SRTP-support-with-different-keys-for-forward-and.patch (11.5 KB)

0003-Ignore-RTP-packets-that-don-t-have-the-version-field.patch (1.21 KB)


#4

Hey

- The SRTP and SRTCP code is part of another project (ZRTP4J) and was just
  copy/pasted from that project and only very small modifications done.
  If I modify the source too much (formatting that is) it becomes much harder
  to verfiy/compare future enhancements.

As Emil noted, can't we use the ZRTP code directly?

No - Once upon a time Jitsi (SC) decided to use it's own RawPacket implementation
and SRTP as well as SRTCP have to deal with it, thus not direct code usage possible

But: for SDES, I need different contexts for forward and reverse transform. To avoid more duplicated code in the sdes package, I made some small modifications in the SRT(C)PTransformer classes to accept two SRTPTransformers. Could we move that to ZRTP4J too?
(Not yet tested (my equipment is at the university) nor committed, see attachments).

The SDES transform introduces some new dependencies which are not covered by the current ZRTP4J project.
The goal to introduce SRTCP was to keep the modifications as small as possible to avoid
migrations effort in other projects.

Most comments apply to the code anyway, whether it lives in Jitsi or in ZRTP4J.

- Your comments regarding "new Long(ssrc)" and why this is necessary: please
  refer to the Java documentation about the hashtable.

That was necessary until Java 1.5 finally introduced autoboxing. And since we're using generics, Java 1.4 is out of question. The SRTPTransformer does it without new for a long time. Better yet, use HashMap instead of the Hashtable to avoid unnecessary locking.

At this point I'm currently looking into some better/faster/less memory solutions,
in particular for the SRTP stuff. However, I'm doing some more testing before putting
the code into operation. Because it's internal only the clients are not affected.

- regarding NULL Authentication: libsrtp contains test cases even for null
  authentication - I'm just preparing to have the same test cases to have
  full test/interop coverage. As per 3711 SRTCP must at least use authentication
  but this is IMHO a matter of the client how to setup the policy.

Hmm, oke, but it just makes no sense. If the client is required to setup the policy with authentication, why allow it otherwise?

- If key derivtaion rate is never used (at least for ZRTP is defined has 2^48,
  effectively zero in terms of SRTP/SRTCP) then it's not necessary to deal with it.
  As a matter of fact I'll remove the KDR stuff in SRTP (at least in ZRTP4J project)
  in near future. You may have noticed that I have no code in the crypto contexts
  to re-trigger new key derivations based on the 48-bit index.

I'd prefer to have a correct implementation instead of removing it. The other key-exchanges (SDES, MIKEY) include parameters to freely define that rate.

:slight_smile:

Additional key derivations based on a KDR are defined as "MAY" in 3711

Even libsrtp doesn't deal with the key derivation rate, it does not even implement
an interface or covers it in its policies. libsrtp treats the key derivation rate as zero
in its KDF code thus disabling it.

Regards,
Werner

···

Am 16.10.2011 15:58, schrieb Bauersachs Ingo:

- I don't understand your comment
   "> Simplification of the hash comparison - there's nothing to cut off in a byte."
  below.

byte x = (byte)0x9a;
byte y = x & 0xff

-> y==x, there's nothing to cut off with that mask if the underlying type is already a byte.

Regards,
Ingo