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

From: John Mattsson <[email-masked]>
Date: Sun, 21 Jun 2015 08:13:24 +0000

Hi,

I did a thorough read of draft-thomson-http-encryption-00.

I think this is important work and a well-written draft. I think that both the Encrypted Content-Encoding and the Encryption-Key Header will be useful for many future applications.

Some comments (mostly minor):

- The draft sometimes talks about a “key” I suggest that all occurrences are reformulated to use “content encryption key”, “IKM”, “private key”, or “public key”.

- The draft often talks about a “message”:
“If the same key is used for multiple messages, then the nonce parameter MUST be unique for each.”
“The "salt" parameter MUST NOT be reused for two different messages”
”a content encryption key is derived for each message”
What is a “message” in the context of this draft? Feels like “message” in some cases means “HTTP message” and in other cases means the “data” Should be spelled out.

- The draft talks about “aesgcm128” as both a “content coding” and a “content encoding”. Is that correct?

- Sec 1: “In particular, this document does not describe key management practices.”
I would say that the Enryption-Key header is key management. The abstract and introduction should say something about the Enryption-Key header and its use.

- Sec 2: “how the the content” -> “how the content”

- Sec 2: The statement “input of between rs-256 and rs-1 octets” is not true for the last record.

- Sec 2: “The record size determines the length of each portion of plaintext that is enciphered.“
Each portion except the final portion. 

- Sec 2: “Therefore, the length of each enciphered record is equal to the value of the "rs" parameter plus 16 octets”
Each record except the final record.

- Sec 2: The specification on how padding is done needs some refinements:

The statements “records always contain at least one padding octet” and “Each record contains between 0 and 255 octets of padding” contradict each other.

The statements “input of between rs-256 and rs-1 octets” and “Each record contains between 0 and 255 octets of padding” contradict each other unless the padding length byte is outside of the padding.

“The length of the padding is stored in the first octet of the payload.”
Is that the first octet of the plaintext/padding or is an octet appended to the ciphertext?
 
Suggestion: “Each record contains between 1 and 256 bytes of padding. The padding consists of one octet (0-255) encoding the length, followed by 0-255 octets of padding. All padding octets except the first octet MUST be set to zero.”

- 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”? 

- Sec 2: It appears to be conflicting statements on the nonce. Section 2 states that “The nonce used for each record is a 96-bit value containing the index of the current record” while Section 3.1 states “generating a random nonce for each message” and Section 7.1 states that “An implementation SHOULD generate a random nonce parameter for every message”.

- 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.
 
- Sec 2: “A sequence of full-sized records can be” -> “A full sequence of records can be”
 
- 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.
 
- Sec 3.1: “The "salt" parameter MUST NOT be reused for two different messages that have the same content encryption key;”
“content encryption key” -> “IKM”
 
- Sec 4: “materal” -> “material”

- Sec 4.1: The "key" parameter MUST decode to exactly 16 octets in order to be used as input keying material for "aesgcm128" content encoding.
The content encoding aesgcm128 only puts requirements on the content encryption key length, not the length of the IKM. Using e.g. a 256-bit IKM should be fine, HKDF handles the key derivation. 
Suggestion: “The "key" parameter MUST decode to at least 16 octets to be used as input keying material for "aesgcm128" content encoding.”
 
- 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”

- Sec 4: I think the Encryption-Key Header section needs more information on how the UA handle the key and keyid. Clearly the UA needs to save the key-value pair (keyid, key), but for how long? Who can access it, and for what can it be used? I think the Encryption-Key Header may need to be expanded with more parameters.

- 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?

- Sec 5.4, 5.5: As the length of “I am the walrus” is 15, there cannot be any padding in these examples.

- Sec 7.1: “If a key and nonce are reused, this could expose the content encryption key and it makes message modification trivial.”
This does not reveal the content encryption key. Suggestion: “If a key and nonce are reused, this exposes the plaintext and the GCM authentication key, making message modification trivial.”

- Sec 7.1: “relies on the uniqueness of the "nonce" parameter to ensure that the content encryption key is different”
“nonce” -> “salt”

- I think the draft needs some short description on how the content-coding works with partial GET requests. They clearly need to align on record borders, i.e. for aesgcm128 bytes=X-Y, where X = n * (rs + 16) and Y = (n + i) * (rs + 16)

- 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.

Cheers,
John

------------------------------------------------------------------
John Mattsson
MSc Engineering Physics, MSc Business Administration and Economics
Ericsson IETF Security Coordinator 
Senior Researcher, Security

Ericsson AB                             Phone +46 10 71 43 501               
Ericsson Research                       SMS/MMS +46 76 11 53 501
Färögatan 6                             [email-masked]
SE-164 80 Stockholm, Sweden             www.ericsson.com
------------------------------------------------------------------

> On 11 May 2015, at 22:32, Martin Thomson <[email-masked]> wrote:
> 
> Of potential interest.
> 
> This is a new version of draft-nottingham-http-encryption-encoding.
> Mark asked that I look after this.
> 
> 
> ---------- Forwarded message ----------
> From:  <[email-masked]>
> Date: 11 May 2015 at 13:26
> Subject: New Version Notification for draft-thomson-http-encryption-00.txt
> To: Martin Thomson <[email-masked]>
> 
> 
> 
> A new version of I-D, draft-thomson-http-encryption-00.txt
> has been successfully submitted by Martin Thomson and posted to the
> IETF repository.
> 
> Name:           draft-thomson-http-encryption
> Revision:       00
> Title:          Encrypted Content-Encoding for HTTP
> Document date:  2015-05-11
> Group:          Individual Submission
> Pages:          17
> URL:
> https://www.ietf.org/internet-drafts/draft-thomson-http-encryption-00.txt
> Status:         https://datatracker.ietf.org/doc/draft-thomson-http-encryption/
> Htmlized:       https://tools.ietf.org/html/draft-thomson-http-encryption-00
> 
> 
> Abstract:
>   This memo introduces a content-coding for HTTP that allows message
>   payloads to be encrypted.
> 
> 
> 
> 
> Please note that it may take a couple of minutes from the time of submission
> until the htmlized version and diff are available at tools.ietf.org.
> 
> The IETF Secretariat
>

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Jun 2015 14:15:02 -0700

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.

> 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.

> - 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 o…

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.)

> - 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)

> “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.

> - 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? Mand…

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.