Re: [websec] AD review of draft-ietf-websec-key-pinning-17

[Yoav Nir <[e-mail]>]

Thanks, Barry.

Authors: you seem to have your work cut out for you.   And to the rest of the working group: there is a good reason why Barry has sent this to the list. Dealing with the AD review is a task for the working group, not only the authors and chairs. So everyone, please feel free to jump in.

The below responses are my own and have no hats.  I’m skipping the language ones, because me not American, me no talk English good. 

On Jun 26, 2014, at 2:00 PM, Barry Leiba <[e-mail]> wrote:

> Is "we believe that" in the middle of the second paragraph something
> that should stand as it is in the final RFC?

                We believe that, with care, host operators can greatly
   reduce the risk of main-in-the-middle (MITM) attacks and other false-
   authentication problems for their users without incurring undue risk.

I think this should stay. 

> 
>   Additional directives extending the semantic functionality of the
>   header fields can be defined in other specifications, with a registry
>   (having an IANA policy definition of IETF Review [RFC5226]) defined
>   for them at such time.  Such future directives will be ignored by UAs
>   implementing only this specification, as well as by generally non-
>   conforming UAs.
> 
> I don't think it's clear enough that this document doesn't define that
> registry, and that this is giving instruction for future registry
> creation.  Please talk with me about why you feel the need *now* to
> say that a future registry will have to use IETF Review as its policy.
> Why would Specification Required or Expert Review not be OK?  Why
> shouldn't the decision be left for the time the registry is created?

I agree with that. How about:

NEW:
   Additional directives extending the semantic functionality of the
   header fields can be defined in other specifications. The first 
   such specification will need to define a registry for such 
   directives.

>   In the pin-directive, the token is the name of a cryptographic hash
>   algorithm, and MUST be "sha256".  (In the future, additional hash
>   algorithms MAY be registered and used.)  The quoted-string is a
>   sequence of base 64 digits: the base 64-encoded SPKI Fingerprint
>   ([RFC4648]).  See Section 2.4.
> 
> I don't like the MUST there (nor the MAY).  As to "registered and
> used", is this intended to be in the same registry as above?  So the
> directive "pin-sha256" would be registered, along with a new
> "pin-shaXYZ"?
> 
> Assuming that's right, I'm really starting to think that the registry
> should be created now.  The reason to defer creation is if we think
> that no new directives will ever really come along, so we won't need
> the registry.  But if the hash algorithm names will be in the
> registry, we can be pretty sure we'll need new ones.

This I will push back on. The day SHA-256 becomes not good enough for this purpose is when there is a second pre-image attack against SHA-256. We don’t have that yet even for MD5. I don’t think we’re going to *need* a pin-SHA512 or a pin-SHA3-256.  Somebody might *want* a pin-GOST-R34.11-94, but in that case I’m happy to leave the work of setting up a registry to them.

> Anyway, a fix for this paragraph (but this will change if we create
> the registry now) could be like this:
> 
> NEW
>   In the pin-directive, the token is the name of a cryptographic hash
>   algorithm.  The only algorithm allowed at this time is "sha256";
>   additional algorithms may be defined in the future. The quoted-string
>   is a sequence of base 64 digits: the base 64-encoded SPKI Fingerprint
>   [RFC4648].  See Section 2.4.
> END

I’m fine with this change.

>   The UA MUST ignore pin-directives with tokens naming hash algorithms
>   it does not recognize.  If the set of remaining effective pin-
>   directives is empty, and if the connection passed Pin Validation with
>   the UA's existing noted pins for the Host (i.e. the Host is a Known
>   Pinned Host), the UA MUST cease to consider the Host as a Known
>   Pinned Host.  (I.e. the UA should fail open.)  The UA SHOULD indicate
>   to users that the Host is no longer a Known Pinned Host.
> 
> The first sentence follows from (5) in the above list, so I suggest
> you say so.  This also appears to be where you define "Known Pinned
> Host", but you've buried the lede here.  I suggest this:
> 
> NEW
>   When a connection passes Pin Validation using the UA's noted pins
>   for the Host at the time, the Host becomes a Known Pinned Host.
> 
>   According to rule 5, above, the UA MUST ignore pin-directives with
>   tokens naming hash algorithms it does not recognize.  If the set of
>   remaining effective pin-directives is empty, and if the Host is a
>   Known Pinned Host, the UA MUST cease to consider the Host as a Known
>   Pinned Host (the UA should fail open).  The UA SHOULD indicate to
>   users that the Host is no longer a Known Pinned Host.
> END
> 
> On the last sentence, though, I'm really unsure: are we really giving
> UI advice in a protocol document?  Is that a good idea?  Do w have any
> sense that my mother will have the first idea what you're talking
> about when you indicate this to her?

We do often write sentences like “This event should be logged.” I don’t think either your mother or mine regularly run Console.app or eventvwr.exe to check logs. I guess this is the equivalent statement for user agents. It makes sense that if there is some indication that a website is pinned, that indication SHOULD be gone when the website is unpinned. It’s not for us to say how a particular vendor will indicate pinning in their UI, but I think it’s reasonable to say that the pinning indication should be gone.

> -- Section 4.3 --
> 
>   Because having a backup key pair is so important to recovery, UAs
>   MUST require that hosts set a Backup Pin. (See Section 2.5.)
> 
> Unless I misunderstand, this requirement means that if my server
> doesn't send a backup pin, my server doesn't benefit from key pinning
> (it's as though I never included the header in the first place).  If
> that's not true, then Section 2.5 needs to be fixed to make it clear
> that failure to include a backup pin constitutes a validation failure.
> 
> If that is true, then you're trading off recovery for security, and I
> expect the Security ADs to question that.  To head that off, it might
> be good to acknowledge that here, and perhaps to say a few more words
> about it.

How about:
The down side of keeping a not-yet-deployed key pair
is that if an attacker gains control of the private
key she will be able to perform a MitM attack without being
discovered. Care must be taken to avoid leaking the key, such
as keeping it offline.


Yoav