Subject: Re: [websec] Gen-art LC review of draft-ietf-websec-key-pinning-19

From: Barry Leiba <[email-masked]>
Date: Mon, 4 Aug 2014 11:06:12 -0400

Yoav, is a revised I-D forthcoming?  Are there further comments about
Elwyn's review?

Barry

On Thu, Jul 31, 2014 at 3:27 PM, Yoav Nir <[email-masked]> wrote:
> Hi, Elwyn
>
> [ adding the WebSec mailing list ]
>
> Thanks for the review. I think the editorial comments and the un-expanded initialisms are not controversial. The text in section 4.1 contains our hard-earned consensus about maximum max-age. Perhaps your comment about section 2.1.1 can be accommodated by pointing to section 4.1 from section 2.1.1.
>
> As for the rest, I will leave up to the group, but I might also comment as an individual.
>
> To the WebSec group: please send in comments about this. A GenArt review (just like *Dir reviews that are likely to come in the next few days) should be addressed just like any other comments. Pushing back is fine if we think they are inappropriate, but comments about lack of clarity tend to be correct when they're made by someone outside the working group - someone who reads the document without having participated in the discussion on-list and at meetings. Making changes is still up to the working group, so please go over Elwyn's comments.
>
> Yoav
> As co-chair and document shepherd.
>
> On Jul 31, 2014, at 4:05 PM, Elwyn Davies <[email-masked]> wrote:
>
>> I am the assigned Gen-ART reviewer for this draft. For background on
>> Gen-ART, please see the FAQ at
>>
>> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>>
>> Please resolve these comments along with any other Last Call comments
>> you may receive.
>>
>> Document: draft-ietf-websec-key-pinning-19.txt
>> Reviewer: Elwyn Davies
>> Review Date: 31 July 2014
>> IETF LC End Date: 1 August 2014
>> IESG Telechat date: (if known) -
>>
>> Summary:
>> Almost ready.  There are some minor issues some of which may be as a result of some
>> misunderstanding on my part.  The descriptive text in the early part of s2 is missing
>> some definitions that make it unclear until they appear later on.  This makes the early
>> descriptions more confusing that illuminating in places.  Suggestions in the detailed
>> comments below.
>>
>> One thing that is not fully clear to me and could probably be explained to help others
>> is the start up of the pinning mechanisms for a given host domain.  AFAICS Pin validation
>> would possibly not be carried out on a first connection to a domain when there are no
>> preconfigured Pins.  I am not clear if this adds anything to the risk of a MITM attack or
>> does it in any way negate the value of the whole pinning process?  I was not clear if
>> an effective Pin validation should be carried out during the first connection when the
>> UA receives the host's Pins for the first time and decides that it is now a Pinned Host,
>> in that the document doesn't state that the connection is terminated if the setting up
>> of the Pinned host fails because the certs don't validate.
>>
>> Major issues:
>> None
>>
>> Minor issues:
>> s1: The term "Pin" (as a noun) is not explicitly defined. The definition doesn't appear
>> until s2.4.
>>
>> s2.1.1: I'm not sure if this could be an issue.. should a maximum possible value for max_age
>> be specified to avoid UA's being cluttered up with old Pins - this might possible be a DoS
>> attack vehicle but I am not sure (yet) if it could be. OK.. so s2.3.3 talks about limits.
>> A pointer to this discussion would be useful here
>>
>> s2.2.1: Does this response behaviour apply to all possible request types? Once a server has sent a
>> Pin header should it send it again on all subsequent requests on the same TLS connection or is
>> that a choice?  Given the "SHOULD" in s2.2.1, what are the circumstances in which the server should
>> refrain from sending the Pins? [I first thought about 'Redirects' but realized that that was probably
>> a really good time to send Pins!]
>>
>> s2.3.1/s2.4: S2.4 states that hash algorithms might be deprecated in future.  Presumably a
>> deprecated algorithm should be treated as an unrecognized directive or some such to avoid
>> downgrade attacks.  Probably worth being explicit about this.  Also this is potentially
>> incompatible with the statement that 'UAs MUST recognize "sha256"' (Para 3 of s2.4).
>> This results in a potential downgrade attack when and if SHA256 is deemed to be no longer
>> cryptographically effective. I think this statement can be removed as presently a UA
>> has no other option if it is to implement the specification.
>>
>> s2.6:
>>>    Note that the UA MUST perform Pin Validation when setting up the TLS
>>>    session, before beginning an HTTP conversation over the TLS channel.
>> I suspect I am confused: If a UA is making its first connection to a host for which it doesn't
>> have a preconfigured Pin, then it won't get the Pin(s) from the host until it has set up the
>> TLS connection and received the response to the request at the HTTP protocol level.  In that case
>> Pin validation will pass by default (subject to local policy perhaps) since the cache doesn't have
>> entries for the host.  Presumably the UA should then perform Pin validation if it has passed by
>> default during TLS setup (assuming that this is possible given the layering) or does the UA have to
>> terminate the session and restart it so that Pin validation can be performed?  The second case may
>> give scope for a DoS attack.  Or is it the case that Pin validation is not needed on the first
>> connection... I don't see why this shouldn't be done but I may not understand the problem.  I think
>> some clarification about the startup of the process is needed.
>> Nits/editorial comments:
>>
>> s1, last para: s/toegether/together/, s/but is possible/but it is possible/
>> s2.1: It would be good to expand the term OWS.
>>
>> s2.1, Figure 1 caption: The acronym HPKP needs expanding.
>>
>> s2.1, 2nd para after numbered bullets:
>> It is not the definition of hash algorithms that is relevant, but allowing them to be
>> used in pin-directives thus:
>> OLD:
>> additional algorithms may be defined in the future
>> NEW:
>> additional algorithms may be allowed for use in this context in future
>>
>> Also the implication of the "sha256" name should be explained precisely -
>> i.e, that the SHA256 hash algorithm will be used, and a suitable reference
>> for SHA256 should be given. (Again this doesn't happen until s2.4).
>>
>> And finally the "Fingerprint" of what SPKI? Defining Pin formally as noted above would help!
>>
>> s2.1, last para: s/hahs/hash/
>>
>> s4.2/Figure 8: The first line of text is too wide.
>>
>> s5, para 1: Is it really "HSTS or HPKP"?  I thought it would be "HSTS combined with HPKP".
>>
>> s6: Needs to be more precise about *which* message headers repository is to be updated! Presumably
>> the permanent one at http://www.iana.org/assignments/message-headers/message-headers.xml#perm-headers.
>>
>> Also there may be some of the questions in s8.3.1 of RFC 7231 that need specific answers.
>>
>> s5, 2nd top level bullet: Expand SNI acronym.
>>
>

---

From: Yoav Nir <[email-masked]>
Date: Mon, 4 Aug 2014 18:49:55 +0300

[ with no hats on ]

inline

>> descriptions more confusing that illuminating in places.  Suggestions in the detailed

[YN] I believe the missing definitions you’re talking about are Pin, Pin Validation. I think anyone who reads this specification is familiar enough with HTTP to know what request, response, UA and directive mean. If so, I suggest that these can be defined in 1-2 sentences in section 1, even if they’re better explained later on.

>> of the Pinned host fails because the certs don't validate.

Key pinning is a TOFU (trust on first use) mechanism. As such, the first time a UA connects to a domain there is no validation. A MitM attacking such a first connection will not be discovered. Worse, such a MitM can inject its own PKP header into the HTTP stream, and pin the UA to its own keys. Note that in order to pin false information, the attacker would have to be able to produce an error-free connection. Without compromising a trusted CA, this should not be possible, and the best that attackers can accomplish is to use an invalid TLS certificate, leading to an interstitial warning page.

This is apparent from section 2.3.1, but perhaps deserves a short paragraph in the introduction.

>> a really good time to send Pins!]

This has been discussed, and because of all kinds of weird deployments, such as with multiplexed servers behind a load-balancer the only viable way was to send the PKP in every response. Some (me!) were concerned about the overhead of these large-ish headers, but we heard from people who actually run servers that a header this small is inconsequential. The draft for HTTP/2 makes this less of an issue, as repeated headers are efficiently encoded by referring back to previous header sets.

---

From: Tobias Gondrom <[email-masked]>
Date: Tue, 05 Aug 2014 21:55:13 +0100

<no hats>
comments inline

> [YN] I believe the missing definitions you’re talking about are Pin, Pin Validation. I think anyone who reads this specification is familiar enough with HTTP to know what request, response, UA and…

I agree with Yoav.

> This has been discussed, and because of all kinds of weird deployments, such as with multiplexed servers behind a load-balancer the only viable way was to send the PKP in every response. Some (me!)…

I agree with Yoav's answer.

> _______________________________________________
> websec mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/websec

---

From: Chris Palmer <[email-masked]>
Date: Tue, 5 Aug 2014 14:37:16 -0700

These fixes are now available in a new version. You can see the changes at:

https://code.google.com/p/key-pinning-draft/source/list

---

From: Elwyn Davies <[email-masked]>
Date: Mon, 11 Aug 2014 16:08:24 +0100

Hi, Yoav and authors.

Thanks for the responses and -20 updates.  Apologies for the delay in 
replying (expedition to rural parts for my birthday!)

These have fixed several of the issues/nits but some remain as noted below.

> This is apparent from section 2.3.1, but perhaps deserves a short paragraph in the introduction.
Fine - done.

>>> s1: The term "Pin" (as a noun) is not explicitly defined. The definition doesn't appear
Done.
>>> A pointer to this discussion would be useful here

A pointer to the discussion in s2.3.3 would still help.

> This has been discussed, and because of all kinds of weird deployments, such as with multiplexed servers
> behind a load-balancer the only viable way was to send the PKP in every response. Some (me!) wereconcerned

about the overhead of these large-ish headers, but we heard from people 
who actually run servers

that a header this small is inconsequential. The draft for HTTP/2 makes 
this less of an issue, as

repeated headers are efficiently encoded by referring back to previous 
header sets.

Ok.  Accepting that the host is expected normally to send the headers on 
all responses, when SHOULD it not send the headers? (from the previous 
comments a server that knows it is not multiplexed might be a candidate 
- or if it knows it has already responded on this connection?)

>>> has no other option if it is to implement the specification.

Not addressed as yet.  At least the statement in s2.4 needs to be removed.

>>> some clarification about the startup of the process is needed.

Ok. Done.

>>> s5, 2nd top level bullet: Expand SNI acronym.

All nits above sorted.

>>> the permanent one at http://www.iana.org/assignments/message-headers/message-headers.xml#perm-headers.
OK.

>>> Also there may be some of the questions in s8.3.1 of RFC 7231 that need specific answers.

There are still some of the questions that ought to be explicitly answered.

One extra nit:
s2, para after bullets: s/reistry/registry/

Regards,
Elwyn

---

From: Chris Palmer <[email-masked]>
Date: Mon, 25 Aug 2014 13:23:57 -0700

> A pointer to the discussion in s2.3.3 would still help.

Added.

> Ok.  Accepting that the host is expected normally to send the headers on all
> responses, when SHOULD it not send the headers? (from the previous comments
> a server that knows it is not multiplexed might be a candidate - or if it
> knows it has already responded on this connection?)

I think it's easiest and simplest for servers to simply always send the headers.

> Not addressed as yet.  At least the statement in s2.4 needs to be removed.

Removed.

> There are still some of the questions that ought to be explicitly answered.

To me, the only questions that might not have obvious answers are:

   o  Whether the header field is useful or allowable in trailers (see
      Section 4.1 of [RFC7230]).

Surely not? For the same reason we don't want it in meta http-equiv.

   o  Whether the header field ought to be preserved across redirects.

Surely not?

Any others?

> s2, para after bullets: s/reistry/registry/

Fixed.

Changes visible at
https://code.google.com/p/key-pinning-draft/source/list. I won't push
a version 21 until I've gone through the rest of the incoming email.

Thank you!

---

From: Elwyn Davies <[email-masked]>
Date: Sat, 30 Aug 2014 19:42:55 +0100

Hi, Chris.

A couple of remaining points and a couple of nits/queries in the fixes.

I have deleted the items that we are agreed on below and added comments 
in line.

New nits:
s2.6, next to last para: s/previous/previously/

s4, lasta para:
> and indeed MUST pin to issuers not in the chain

Is this a requirements MUST?  The protocol will be quite happy and 
implementers can't (at least I don't think) they can check if the host 
leaves it out.  This is operational advice (admittedly highly relevant) 
but still advice I think.

> On Mon, Aug 11, 2014 at 8:08 AM, Elwyn Davies <[email-masked]> wrote:
<<snip>>

> I think it's easiest and simplest for servers to simply always send the headers.

Having thought about this a bit, I don't think this text fully reflects 
what is going on in the server.  If I understand correctly, we have:
- Server has a policy that it wants to be pinned if it can be 
(presumably this ought to be configurable in the server).
- When a client sends a request over a new secure connection and the 
policy says 'be pinned', the server sends back pinning header(s) in the 
response.
- Thereafter, it MAY for simplicity send pinning headers in all 
subsequent responses on the connection. Alternatively it can do 
something intelligent and not bother sending the headers if it is the 
same connection - but of course this probably means a layer violation.

Thought: Does it make any difference if the client doesn't accept the 
pin? Should success or failure be logged?  Should the host always keep 
sending the headers if the client didn't accept the pin on the first
exchange (is this a possible symptom of a MITM?)?

Suggestion:
OLD:
2.2.1.  HTTP-over-Secure-Transport Request Type

    When replying to an HTTP request that was conveyed over a secure
    transport, a Pinned Host SHOULD include in its response exactly one
    PKP header field, exactly one PKP-RO header field, or one of each.

NEW:
2.2.1.  HTTP-over-Secure-Transport Request Type

    If a host has a policy of becoming a Pinned Host whenever possible,
    then on replying to an initial HTTP request that was conveyed over a
    secure transport, a host intending to become Pinned Host includes in
    its response exactly one PKP header field, exactly one PKP-RO header
    field, or one of each.  The host MAY include the same header(s)
    with any subsequent responses using the same connection.

<<snip>>

>     o  Whether the header field ought to be preserved across redirects.

The point was that I guess you ought to explicitly say "not" in each case.

<<snip>>

> a version 21 until I've gone through the rest of the incoming email.
Cheers,
Elwyn