Subject: Re: New Version Notification for draft-thomson-http-encryption-00.txt

From: John Mattsson <[email-masked]>
Date: Sat, 25 Jul 2015 08:20:38 +0000

Thanks for your quick answers. And thanks for the acknowledgement ;)

The updated draft seems very good and reads well.

Some new comments:

- s1. “only someone with the appropriate key can read it”
Or modify it.

- s2. “(one fewer for the last record)”
“One” seems wrong. Did you mean “(Or fewer for the last record)”

- s2. In the first figure, the two “data” boxes should probably have the
same size.

- s2. “followed that number” -> followed by that number”?

- s2. It would be good if the draft made it clear that the division
of the content into data chunks of between rs-256 and rs-1 octets is
implementation
specific (if that is the intension).

- 3.1. “a base64 URL-encoded octets” -> “base64 URL-encoded octets”

- s3.3 “is a 12 octet (96 bit) value is produced”
Too many “is"

- 4.2 “The intended recipient recovers their private key and are”
  ->“The intended recipient recovers their private key and is”

See some answers below. I’ll only responded where I has something to add.

On 23/06/15 23:15, "Martin Thomson" <[email-masked]> wrote:

>Thanks for the thorough review John.  I have made a number of changes,
>most of which (I hope) improve the document in the right direction.
>
>You can review my changes here if you like:
>https://github.com/martinthomson/http-encryption/pull/3
>
>Most of your comments were pretty straightforward; I'll only respond
>to those that I think we need to discuss.
>
>On 21 June 2015 at 01:13, John Mattsson <[email-masked]>
>wrote:
>> - Sec 2: “A receiver MUST fail to decrypt if the remainder is 16 octets
>>or less in size”
>> Should likely be “is 1 octets or less”?
>
>16 octets or less is correct.  A valid ciphertext will always include
>at least one padding byte and a 16-byte authentication tag.  I've
>expanded the context here.

Yes, my mistake.

>
>> - Sec 2: According to “The nonce used for each record is a 96-bit value
>>containing the index of the current record”, there is no “fixed” part of
>>the nonce. Setting the length of the fixed portion of the nonce to zero
>>lowers the theoretical security level against various dictionary
>>attacks. I would recommend using the salt to populate a “fixed value” of
>>some length (maybe 4,6, or 8 bytes). A 12-byte counter is not needed
>>anyway.
>
>I'm actually not that concerned about the dictionary attack, but belt
>and braces has only occasionally turned out to be an awful idea.
>
>This is the direction TLS 1.3 has adopted.  And I had planned to copy
>that construction if it ever came about.  The plan there is to
>separately derive a value and XOR that with the record sequence
>number.  That gives you a 12-byte counter, if you want it (my
>implementation only goes to 48-bit lengths and I don't see much need
>for that much even).
>
>(Reminder for me to talk to some folks about breaking this.  Better
>now than later though.)

I think the nonce derivation in the new nonce section is great, I have
always thought this type of xor construction is superior to concatenation.
(This would offer maximum protection against theoretical dictionary
attack, which I am actually not very concerned about either).

I think you can see the construction as a fixed field of length 0 and the
NONCE as a counter field of length 96, In that case all the RFC5116
requirements on nonce creation is fulfilled. The only thing the
construction is not following is the SHOULD in RFC5116 “The Counter part
SHOULD be equal to zero for the first nonce.

>
>> - Sec 2: “A sequence of full-sized records can be” -> “A full sequence
>>of records can be”
>
>The point was that they were full-sized.
>
>> - Sec 2: As far as I can see the construction with no AAD is safe, the
>>nonce is implicitly integrity protected as the decryption fails if the
>>nonce is changed.
>
>That was my analysis as well :)  I just wanted more eyes on before I
>removed the note.
>
>
>Note that this intentionally follows the structure of the AEAD use in
>TLS, which includes the sequence number and the TLS version in the
>AAD.  The things that need to be covered by the integrity check are
>therefore:
> * the sequence number (covered by the nonce, as you say)
> * the fact that this is this specific version of the
>content-encoding, and not some transplant from some other context
>(covered via the content-encryption key, by way of the inclusion of a
>very specific label in HKDF)
>
>> - Sec 4: The Encryption-Key Header must clearly mandate use of https
>>with a high enough security. I suggest something like:
>> “The Encryption-Key Header MUST only be used with https fulfilling at
>>least the requirements in [RFC7540] Section 9.2”
>
>I'm less sure about this, given the DH usage.  Maybe just for explicit
>keys.  Even there, I'm concerned that this is necessary, but not
>sufficient for real security.  I'm a little concerned that a
>stipulation of that form will be taken to imply that HTTPS is enough.

Agree that HTTPS is not enough, but right now the document does not even
mention that the key sent in the Encryption-Key header requires
confidentiality.
Maybe a new section in the Security Considerations is the right way to go.

>> - Sec 4.1 “Other key determination parameters can be ignored if the
>>"key" parameter is present.”, doesn’t this needs to be more strict. If
>>there are not ignored, what happens then? What is associated with keyid?
>>Should it even be allowed to send several conflicting parameters?
>>
>> - Sec 5.3: Why is the content-length in example 1234 and not 1216?
>>Wouldn’t 1216 be the maximum content-length for aesgcm128 with rs=1200?
>
>1216 would be the size of a single record and thus would be invalid.
>1234 is at least valid, but honestly, only by accident.
>
>> - It would be good if the draft separated the general mechanism and the
>>specific aesgcm128 content-coding a little bit more. Is for example
>>Section 3.2 specific for aesgcm128 or is it general? Mandating a
>>16-octet salt seems limiting in the general case.
>
>The lack of generality is entirely intentional, which I'm sure will
>offend instincts that have been drilled into many of us.
>
>As for the specific limit on salt, the goal is to ensure uniqueness.
>16 octets is sufficient for that purpose.