Re: [AVTCORE] draft-ietf-avtcore-srtp-aes-gcm-10 review

[John Mattsson <[e-mail]>]

Hi,

With the NIST servers back online, I could read the NIST material regarding GCM, which reminded me of the fact that in GCM, a small number of repeated IVs for different messages do not only void the confidentiality of these messages, it also leaks the GCM authentication key H. With H, an adversary can calculate the AEAD authentication tag for any chosen Ciphertext and Associated Data.

This is not a problem when GCM is used in e.g. TLS or IPsec, but with SRTP where group keys are common, SSRC collisions may occur, and the IV is calculated from information given from the RTP application, repeated IVs may actually happen.

I think the consequences should be made clear to application developers using AES-GCM (this is currently missing), and the needed deviations from RFC3711 should be more explicitly stated.

(I do not like just pointing out problems, I reviewed this draft because I really like GCM and CCM, and think they will (and should) be used a lot. In fact, I think the introduction could advertise the benefits even more, both GCM and CCM combine outstanding performance with provable security. What more can you hope for.)

/John

Four more comments, all related to repeated IVs:

- [Section 6]
"With any counter mode, if the same (IV, encryption_key) pair is used twice, precisely the same keystream is formed. As explained in section 9.1 of RFC 3711, this is a cryptographic disaster."

While this is true, Section 9.1 of [RFC3711] only discusses two-time pad and the consequences of repeated IV with GCM are even worse. I would suggest writing:

"With any counter mode, if the same (IV, encryption_key) pair is used twice, precisely the same keystream is formed.  As explained in section 9.1 of RFC 3711, this is a cryptographic disaster. For GCM the consequences are even worse as also the integrity is compromised, and not only temporary, but for all future use of encryption_key."

And maybe better to move the text to Section 9.4 or the Security Considerations section.


- [Section  9.4]
"In order to prevent IV reuse, we must ensure that the (ROC,SEQ,SSRC) triple is never used twice with the same master key. There are two phases to this issue.

Counter Management: A rekey MUST be performed to establish a new master key before the (ROC,SEQ) pair cycles back to its original value."

This assumes that the RTP application is totally trusted. With the transforms in [RFC3711] this is not important as a malicious RTP application can only compromise the confidentiality of information it already had knowledge about. For GCM, this is no longer the case as a malicious RTP application can make the SRTP implementation leak the authentication key just by using the same SEQ twice, something RFC3711 do not require the SRTP implementation to check:

Section 4.1.1 of [RFC3711]: "It is RECOMMENDED that, if a dedicated security module is present, the RTP sequence numbers and SSRC either be generated or checked by that module (i.e., sequence-number and SSRC processing in an SRTP system needs to be protected as well as the key)."

For GCM, I think this has to be changed to MUST. I therefore suggest adding something along the lines of:

"For GCM, the SEQ and SSRC values used MUST either be generated or checked by the SRTP implementation, or by a module (e.g. the RTP application) that can be considered equally trusted as the SRTP implementation. While [RFC3711] allows detecting SSRC collisions after they happen, SRTP using GCM with shared master keys MUST prevent SSRC collision from happening even once."


- [Section  9.4, Section 10.4]
"before the (ROC,SEQ) pair cycles back to its original value."
"Ideally, a rekey performed should be performed and a new master key put in place well before the SRTCP index overflows."

Due to previous re-keying, the need for a new master key may or may not coincide with overflow of the indices. 


- [Section 9.4]
In the bullet "SSRC Management" I think something like the following is needed:
"The identity of the entity giving out SSRC values MUST be verified, and the SSRC signaling MUST be integrity protected."


-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of John Mattsson
Sent: den 14 oktober 2013 15:10
To: [e-mail]; [e-mail]
Subject: [AVTCORE] draft-ietf-avtcore-srtp-aes-gcm-10 review

Hi,

I reviewed draft-ietf-avtcore-srtp-aes-gcm-10. Sorry for commenting after the publication request.

I support the publication, but I have some comments that should probably be addressed before proceeding to the IESG review:

/John


Comments and suggestions on draft-ietf-avtcore-srtp-aes-gcm-10
-------------------------------------------------------------------------

- [s6] The values for the maximum keystream/plaintext length do not seem to agree with the P_MAX values in [RFC5116]. Section 6 states:
"up to (2^28)-16 octets for AES-CCM and up to (2^36)-32 octets for AES-GCM."

[RFC5116] states:
"P_MAX is 2^36 - 31 octets" for GCM and "P_MAX is 2^24 - 1 octets" for CCM. 

While lowering the value for GCM with 1 octet would be ok, increasing the values with a factor of 16 for CCM would require some motivation. Might this this actually be a typo in [RFC5116]? 

Why were the values from [RFC5116] not followed directly? I assume it was done to match an integer number of blocks. I suggest adding a short motivation.

- [s11] The values for C_MAX in Section 11 do not agree with the values for keystream/plaintext length in Section 6. Section 11 states:

"C_MAX      maximum ciphertext       GCM: MUST be <= 2^36-16 octets.
            length per invocation    CCM: MUST be <= 2^28-16 octets."

As the minimum AEAD tag length is 8 octets this would mean that the maximum keystream length is 2^28-24. Assuming the values in Section 6 is correct, the correct value should probably be:
"CCM: MUST be <= 2^28 octets."

- [s14.1] The text describing the handling of the block counters do not seem correct. Section 14.1 states:
" At the start of each packet, the block counter MUST be reset (to 0 for CCM, to 1 for GCM). The block counter is incremented after each block key has been produced, but it MUST NOT be allowed to exceed 2^32 for GCM and 2^24 for CCM." 

1. While the number of blocks could exceed 2^32 for GCM and 2^24 for CCM, the block counter can never exceed 2^32-1 for GCM and 2^24-1 for CCM.

2. Even with a block counter value of 2^24-1, CCM would have exceeded maximum keystream length of (2^28)-16 octets specified in Section 6.

Assuming that the keystream limits in Section 6 are correct, I think the correct values should be:
"The block counter is incremented after each block key has been produced, but it MUST NOT be allowed to exceed (2^32) - 2 for GCM and (2^24) - 2 for CCM.

- The values for the maximum Ciphertext length for GCM_8 do not seem to agree with the constraints in Appendix C of 800-38D: "For any implementation that supports 32-bit or 64-bit tags, one of the rows in Table 1 or Table 2, respectively, shall be enforced." This seems to be missing in RFC5285 as well. 

- The values for the maximum number on invocations (2^48 and 2^31) for GCM_8 do not seem to agree with the constraints in Appendix C of 800-38D. This seems to be missing in RFC5285 as well.

I have not had time to read the actual papers behind Appendix B and C in 800-38D, but if Appendix C for some reason do not apply to draft-ietf-avtcore-srtp-aes-gcm-10 (and RFC5285), this should be mentioned and motivated. Otherwise, would there be any additional constraints on Ciphertext length and maximum number on invocations for GCM_12?

- The document decreases the session salt length from the RFC3711 default of 112 to 96. Whether it is to follow the recommendations in RFC5116 or allowing longer keystreams, it should be motivated. And even if the negative security implications against pre-computation and TMTO attacks are theoretical (2^96 keys in use instead of 2^112) the negative security implications compared to RFC3711 should be discussed (shortly).

- The document decreases the session salt length from the RFC3711 default of 112 to 96, but it does not mention weather the default length of the master salt is also changed to 96 or if it is still 112.

- There is a lot of "optional", "must" and "not recommended" etc. written with lower case. Both in text and figures. Should all be CAPITAL I think. 

- [s3] As this section gives an overview of the security architecture in the draft rather than general SRTP I would suggest:
"SRTP/SRTCP security" -> "SRTP/SRTCP AEAD security"

- [p4] "A Key Derivation Function is applied to the shared master key value to form separate encryption keys, authentication keys and salting keys for SRTP and for SRTCP (a total of six keys).  This process is described in sections 4.3.1 and 4.3.3 of [RFC3711]."

Key derivation for SRTCP is described in section 4.3.2. Maybe best to write "This process is described in section 4.3 of [RFC3711]."

- [s4] The defined terms are barely used at all:
"Crypto Context" is not used at all in the text.
"Instantiation" is used exactly 1 time after the definition.
"Invocation" is used only 3 times. 

Should be fixed in one way or another.

- [s5.2] 5.2.1 specifies the encrypt input for Associated data and Plaintext as "Bit string", while 5.2.2 defines them defines them as "Octet string". I assume both should be "Octet string". 

- [s5.2.1] "cannot", should this maybe be "MUST NOT" or "SHOULD NOT" instead?

- [s6] As the output of GCM_keystream and CCM_keystream is key_stream and not Ciphertext, the input should probably be len(Plaintext) instead of Plaintext.

- [s8.2] Should probably use ""NOT RECOMMENDED".

- [s8.2] Maybe give VBR codecs as an example.

- [s9.2] Marker (M) and Payload Type (PT) is missing from the list of Associated Data fields.

- [Figure 2] While PT means "Packet Type" in RTCP it means "Payload Type" in RTP.

- [Figure 2,4,5] I suggest writing "SRTP authentication tag" and "SRTCP authentication tag" instead of just "authentication tag".

- [s9.2] In the paragraph below Figure 2, shoudn't "cipher" be "ciphertext".

- [s9] Figure 2,4,5 does not show AEAD input as SRT(C)P MKI and SRT(C)P authentication tag are appended after encryption (and removed before decryption). As the packet processing from RFC3711 is not changed (nor should it be) the packet will never look like Figure 2,4,5 during the processing. 

I would suggest changing the pictures to show:
"Format of an AEAD SRTP packet"
"Format of an AEAD SRTCP packet when encryption flag = 1"
"Format of an AEAD SRTCP packet when encryption flag = 0"

which would then include the new "AEAD authentication tag".

- [s9.3] "Additional Authenticated Data (AAD)" Is it not best to just use Associated Data (AD) consistently in the whole document?

- [s9.2, s10.2, s10.3] The SRT(C)P MKI and the SRT(C)P authentication tag both have variable lengths (i.e. not 32 bit). 

- [Figure 4,5] sender info, report block 1, report block 2, ..., SDES items, ..., SRTCP MKI all have variable lengths (i.e. use ":" not "|"). 

- [Figure 4,5] The figures replaces the text "PT=SDES=202" with the general "Packet Type" but still keeps "SDES items".

- [s13] In the tables, shouldn't "Default key lifetime" be "Maximum key lifetime"

- [s13] In the tables, "Cipher (for SRTP and SRTCP)" includes tag length but not key length. Should include both or none.

- [s14.1] As all the bullets contain "MUST", they are maybe more requirements than recommendations

- [s14.1] "Each time a rekey occurs, the initial values of the SRTCP index and the values of all the SEQ counters MUST be saved."
Save initial SEQ value? Do you mean something like:
"the current values of the SRTCP index and the SRTP index MUST be not be changed"

- [s14.1] "Packet Counter" this is called "SRTP index" or "packet index" in RFC3711. I suggest using one of these terms (and as SEQ starts on a random value, it is not really a counter either).

- [s14.1] I would suggest "Processing MUST cease if" -> "Processing MUST cease before" to avoid ambiguity of what to do with the last packet.

- [s14.2] " The goal of an authentication tag is to minimize the probability" As this is not minimize in the mathematical way, I suggest to use some synonym like "reduce", or reformulate.

- [s14.2] Strictly, prob_success <= expected number of successes (equal  for N_tries = 0,1).

- [s14.2] I suggest to mention that:
   if   expected number of successes << 1 
   then prob_success approx. expected number of successes Otherwise it is hard to understand that the table contains approximate and quite exact values and not only maximum bounds for the probabilities.

- [s14.2] "2^-N_tries" is correct for tags in general, but isn't the correct value for GCM "ceiling(len(Ciphertext + AD) / 16) * 2^-N_tries"? (c.f. Appendix B of 800-38D).

- [s15.1] "In order to allow SDP to signal". As RFC4567 could also be used, I suggest changing "SDP" to "Security Descriptions" or "SDP Security Descriptions" 

- [s15.1] I think the correct registry is "Session Description Protocol (SDP) Security Descriptions".

- [s15.3] As the "AEAD authentication tag length" is general and may be used for future AEAD algorithms, the Possible values should be "depends on cipher used"

- [s15.3] If there is a default key length, there should also be a default "AEAD authentication tag length" (8 octets).

- [s16] I suggest changing "AEAD" to "AEAD_AES" or "AES-GCM and AES-CCM" as Section 12 is not that general.


Editorial Comments on draft-ietf-avtcore-srtp-aes-gcm-10
-------------------------------------------------------------------------

- Some of the section headings miss to capitalize all the first letters.

- [p1] "each AEAD will" -> "each AEAD algorithm will"

- [p4] " number(SEQ)" -> " number (SEQ)"

- [p5] "Cipher Context" -> "Crypto Context"

- [p6] "Octets long" -> "octets long"

- [p7] "9.1 and 10.1)with" -> "9.1 and 10.1) with"

- [s9.1] "4-octer" -> "4-octet"

- [9.1, 10.1] "12 octet" -> "12-octet"

- Figure 1 and 3 uses different approaches to enumerate the bytes. 

- [s9.2] "see figure 2" -> "see Figure 2"

- [Figure 4] "SRTCP MKI (optional) index" -> "SRTCP MKI (OPTIONAL)"

- [s13] Caption is above Table 1 and 8 but below all other tables.

- [s13.2] "CBC MAC" -> "CBC-MAC"

- [s15.3] Delete "lists maintained under "MIKEY Security Protocol Parameters"."

- [s16] "length )" -> "length)"
	
- [s16] "the key length of the Key Derivation Function"
     -> "the input key length of the Key Derivation Function"

- [Table 16] remove space before 16:
   "AEAD_AES_128_GCM_12  |  AES-GCM   |  16 octets  | 12 octets   |"
-> "AEAD_AES_128_GCM_12  |  AES-GCM   | 16 octets   | 12 octets   |"

- [s17] If you would like to thank my Ericsson colleagues, their names are "Westerlund" and "Ohlsson"

(BTW the pdf version of the document has strange page breaks...)


------------------------------------------------------------------------------------
JOHN MATTSSON
MSc Engineering Physics, MSc Business Administration and Economics Senior Researcher, Security 

Ericsson AB
Security Research
Färögatan 6
SE-164 80 Stockholm, Sweden
Phone [phone]
SMS/MMS [phone]
[e-mail]
www.ericsson.com




This Communication is Confidential. We only send and receive email on the basis of the terms set out at www.ericsson.com/email_disclaimer 


_______________________________________________
Audio/Video Transport Core Maintenance
[e-mail]
https://www.ietf.org/mailman/listinfo/avt