Encryption processing is under-specified #31
Comments
|
We considered adding the transformed byte format to which apply the encryption/decryption to the spec: But it was agreed that:
Regarding complexity of the temporal buffers, having implemented it, it only took a couple of hours to add it to the libsrtp library and it only requires an 32bits temporal register plus 2xnum_of_csrcs 32 bits mem moves. I would not even know how to start implementing option (3) so hard to consider it as an easier to implement solution. |
|
It sounds like the intent of what was agreed in #18 was basically option (1) in the original comment here. The "cryptex reordered packet" sounds like it is the plaintext I refer to. But that notion never made it to the document, so even if the WG's decision is to go with this pattern, the document needs to be updated. But you would still have the problem that you have to do all the moves you describe, which wouldn't be necessary with option (3). To your point about how to implement -- in libsrtp terms, you would just do what |
|
Here's a mockup of how you would do the encryption with option (3) in libsrtp: |
|
Agree that we need to explain more about what we're doing here to ensure everyone is on the same page. Test vectors however should be sufficient to ensure proper interop. Regarding 1) vs 3), there's about the same amount of code either way. I do agree 3) is slightly easier to reason about, since you just skip over bytes in a way that I suppose isn't possible with a block cipher. The fact that there is precedent for this in 6904 is also nice. @JonathanLennox, thoughts on this? |
|
Since the existing test vectors (and thus the PRs for both libsrtp and jitsi-srtp) use option 1, I'm personally inclined to leave it that way, though I agree it could be better described. I confess I don't quite understand how hole-punching can work for AEAD ciphers, since ciphertext from the hole isn't available for the verifier when verifying the authentication tag. |
|
@JonathanLennox The only way to "hole-punch" with AEAD is to assemble a byte string without the bytes you want to skip (either omitted or set to zero or something). Either way, there's some This is why my preference is for (3), because there are well-defined ways to do hole-punching with stream ciphers. |
|
As you say with AEAD you have to assemble the ciphertext as a byte stream without the bytes you want to skip, so it seems to me that it'd be simplest for CTR to do the same thing, rather than have separate mechanisms for CTR and AEAD. |
|
Well, the question is basically whether we always do the AEAD thing or always do the CTR thing (where in the latter case you would use some stream cipher corresponding to the AEAD, as with RFC 6904). These basically correspond to (1) and (3). |
|
The whole point of cryptex is that the header extensions and the payload (and the CSRCs) are encrypted in a single operation, so it's not clear to me what "using a stream cipher corresponding to the AEAD" would mean? |
|
To be concrete here about the proposals (imagining the primary encryption is AEAD and assuming the same ultimate packet layout): Option 1: AEAD-like
(You could make this more elegant by making the ciphertext contiguous, e.g., by setting CC=0 in the header and representing the real CC in the "defined by profile" of the shim header.) Option 3: RFC 6904-like
Also, since both of those are kind of gross, here's a proposal with a different packet format that works more nicely with AEAD: Option 4: AEAD but nicer
So your packet format would look like: |
|
I still feel like option 1 is the best. It's just a buffer copy of 4*(cc+1) bytes (only when cc != 0), and my implementation https://github.com/jitsi/jitsi-srtp/pull/29/files#diff-5e0923c6a3d2fde412050f213229e9c3dc69b8c3b35d895ffecd66364994b2cfR411 is like 15 lines of simple code. |
|
But really at this point I feel like this is a topic big enough that it should get WG consensus. Will you be in person in Vienna? |
|
Assuming you == me, I will not be in Vienna in person, but I'm happy to dial in. Re: buffer copy of csrc_start = pkt_start + fixed_hdr_size;
ext_hdr_start = csrc_start + (4 * cc);
ext_data_start = ext_hdr + fixed_ext_hdr_size;
payload_start = ext_data_start + ext_data_size;
encrypt_start = payload_start;
encrypt_size = payload_size;
+if (cryptex) {
+ encrypt_start = csrc_start + 4;
+ encrypt_size = (4 * cc) + ext_data_size + payload_size;
+ move_ext_hdr_up(csrc_start, ext_hdr_start);
+}
aead_encrypt(aad=pkt[..encrypt_start], encrypt_start, encrypt_size);
+if (cryptex) {
+ move_ext_hdr_down(csrc_start, ext_hdr_start);
+}Does that look right? In any case, it would probably be good to describe that in the document as a suggestion to implementers. |
|
I can try to prepare a PR to add the "internal" packet structure for AED and how to generate it from and to an RTP packet as a note to the implemementators. I would however not able to present the alternatives in the meeting as I think my crypto knowledge is not good enough to do a proper job and lead the discussions. |
|
FWIW, I wouldn't phrase it as the "internal packet structure", but as "the inputs to the encryption", roughly as above: With an explanatory note that you can transform a packet to make the plaintext continuous by swapping the CSRCs and the extension header (which might be clearer with a picture). If it would be helpful, I could present the alternatives in the meeting. |
|
I have created a PR for clarifying the plaintext and associated data in use for the encryption context. Would be good if you can check if this approach works for you or you would like to present a alternative encryption method. |
|
fixed in #45 |
The current description in {{encryption-procedure}} is not sufficient to interoperably implement this, as it allows multiple interpretations. It is also unnecessarily complicated to implement.
On multiple interpretations: With a stream cipher like AES-CTR, it is not clear whether the 4-byte extension header advances the keystream or not. That is, does the cipher need to generate
4*CC + ext.length + payload.lengthbytes of keystream, which it splits over the two segments, or does it generate4*CC + 4 + ext.length + payload.lengthbytes, and not use the four where the plaintext extension header goes (as in RFC 6904). Likewise, for AEAD ciphers like AES-GCM, the cipher authenticates the plaintext as a single contiguous byte string, so we need to specify what that byte string is.On unnecessarily complicated: Most encryption APIs in the world take a single "plaintext" argument, comprising all of the plaintext to be encrypted. This is especially true for AEAD ciphers. The reassembly required by the current scheme forces implementations to do a bunch of copies to and from a temporary buffer on either side of the encryption.
There are basically two ways to address this problem:
Clearly define the plaintext, but leave it discontiguous (don't solve the complexity problem). The interoperability problem would be fixed if you just defined that the plaintext is the concatenation of (1) the CSRCs, (2) the extension header data, and (3) the RTP payload. That would still be a hassle to implement, but at least you would have a chance of interop.
Make the plaintext contiguous. If I understand correctly, the need for the plaintext header extension header is to allow a cryptex-encrypted RTP header to parse in the same way as an unencrypted one. Assuming this constraint, I don't really see an easy way to make the plaintext contiguous. You would basically have to have set
X=0andCC=<encrypted header size>, but then you would have to add the realXandCCvalues in the plaintext, which is again messy.Use a stream cipher to allow hole-punching. You could do this in the style of RFC 6904: Leave the main SRTP protection unchanged, but use a stream cipher to encrypt the CSRCs and header extension. This also makes it simple to punch a hole for the header extension header, in the same way RFC 6904 does.
My preference would be for (3) here. It's a larger change to the document, but I think it ends up being simpler to implement:
The text was updated successfully, but these errors were encountered: