Subject: [Trans] AD Review: draft-ietf-trans-rfc6962-bis-26.txt

From: Eric Rescorla <[email-masked]>
Date: Sun, 3 Sep 2017 16:17:09 -0700

Hi folks,

Please find enclosed the first cut of my AD review of this draft.

Note: the original of this review is on Phabricator at:

  https://mozphab-ietf.devsvcdev.mozaws.net/D13

If you want to see comments in context -- which is a lot easier -- you
can go there. Also, you can create an account and respond inline if
you like.  If you elect to, let me know if you run into problems.

-Ekr

Note: I have not yet reviewed the algorithms in S 2.1. I plan to do
that separately, but figured it would be useful to provide the rest of
my review on the assumption that the changes to that section will be
modest if any.

High-Level:

1. This document makes a variety of claims about the assurances that
clients get that only obtain if some as-yet-to-be-specified
third-party verifiability mechanism is implemented. For instance, in
the intro:

  Certificate Transparency aims to mitigate the problem of misissued
  certificates by providing append-only logs of issued certificates.
  The logs do not need to be trusted because they are publicly
  auditable.

As the extensive discussion following Richard Barnes's and my previous
comments should make clear, this is only a property of CT if you also
have some mechanism for third-party verifiability of STHs, and this
document does not supply that. In the actually deployed -- we can
debate deployable separately the deployability of some of the
proposals for how to get this-- versions of CT, what clients get is
SCTs, which are effectively countersignatures and in fact do require
trusting the logs. This is implicitly acknowledged by proposals that
RPs only accept certificates with >1 SCT. I've noted a number of
places in my review in detail, but in general, you need to scale back
all the claims of third-party verifiability to make clear what you get
with:

(a) the current system
vs.
(b) a system in which the missing third-party verifiability pieces were
filled in.

2. You need to work out how the various TLS extensions work for TLS
1.3.  I suppose you could persuade me that this is a followon piece of
work, but given that TLS 1.3 is in a similar state of maturity, that
seems kind of silly.

INLINE COMMENTS
Line 157
   The logs do not need to be trusted because they are publicly
   auditable.  Anyone may verify the correctness of each log and monitor
   when new certificates are added to it.  The logs do not themselves
This may be the objective, but it doesn't seem to be what CT actually
delivers in practice.

Line 336
   [CrosbyWallach] proposal, except our definition handles non-full
   trees differently).
NIT: what happened here?

Line 625
   chain up to an accepted trust anchor.  The trust anchor (a root or
   intermediate CA certificate) MAY be omitted from the submission.
How does the submitter know what an accepted trust anchor is? Please
provide a reference to the appropriate section here.

Line 735
   of the tree.  * Sign the root of the tree (see Section 4.10).  The
   log may append multiple entries before signing the root of the tree.
I think you mean for these to be bullets with line breaks.

Line 763
   Maximum Chain Length:  The longest chain submission the log is
      willing to accept, if the log chose to limit it.
Nit: chooses

Line 785
   accepted trust anchor, using only the chain of intermediate CA
   certificates provided by the submitter.
Why is this a 2119 MUST? It seems wise, but not necessarily a conformance
requirement

Line 816
   anchor used to verify the chain (even if it was omitted from the
   submission).  The log MUST present this chain for auditing upon
   request (see Section 5.6).  This prevents the CA from avoiding blame
What happens in cases of multiple chains. For instance, say that the
submitter provides superfluous certificates?

Line 837
       opaque LogID<2..127>;
This seems to be the first use of the TLS specification language, but I
don't see a cite. Please provide one,

Line 926
       opaque TBSCertificate<1..2^24-1>;
NIT: there's no actual way a TBSCertificate can be 1 byte.

Line 948
   "tbs_certificate".  The length of the "issuer_key_hash" MUST match
   HASH_SIZE.
Is this true? What happens if we have two CAs that share a key?

Line 970
           Extension sct_extensions<0..2^16-1>;
           opaque signature<0..2^16-1>;
       } SignedCertificateTimestampDataV2;
Why is signature 0 bytes? Certainly it must be at least 1.

Line 1155
   Note that JSON objects and URL parameters may contain fields not
   specified here.  These extra fields should be ignored.
Is this a 2119 SHOULD?

Line 1163
   errors may occur that are caused by skew between the machines.  Where
   such errors are possible, the front-end will return additional
   information (as specified below) making it possible for clients to
"will"? What's the normative force of this?

Line 1233
         (or, if "chain" is an empty array, the "submission") is
         certified by an accepted trust anchor.
IMPORTANT: Is the implication here that the log is not supposed to do path
construction because the submitter provides a complete path?

Line 1432
      index of requested hash < latest STH  Return "inclusion".
This would be clearer with colons or dashes or somesuch.

" latest STH < requested STH: Return latest STH"

Line 1435
      Note that more than one case can be true, in which case the
      returned data is their concatenation.  It is also possible for
      none to be true, in which case the front-end MUST return an empty
It's not actually concatenation, right? It's all the fields.

Line 1522
   permissible.  These entries SHALL be sequential beginning with the
   entry specified by "start".
How does the client know which of the above two cases has occurred?

Line 1552
   TLS servers MUST use at least one of the three mechanisms listed
   below to present one or more SCTs from one or more logs to each TLS
This needs to somehow be clear that it only applies to TLS servers that are
compliant with this specification, as it's not a new requirement on all TLS
servers.

Line 1595
   been struck off for misbehavior, has had a key compromise, or is not
   known to the TLS client).  For example:
Maybe replace "For example:" with "Some ways this can happen are..."

Line 1599
      misissuance from clients.  Including SCTs from different logs
      makes it more difficult to mount this attack.
Assuming that the server is malicious, why would it include multiple SCTs?
It seems like requiring multiple SCTs does in fact provide this defense,
but that's not an argument for servers to provide multiples.

Line 1627
             SerializedTransItem trans_item_list<1..2^16-1>;
         } TransItemList;
Structurally, it's kind of a mess to have this be the place that you make
TransItems self-contained (by having a defined length field). What about
other places I might want to concatenate TransItems. Why don't you instead
make TransItem self-contained, like so:

struct {
          VersionedTransType versioned_type;
          uint16 length;   // NEW
          select (versioned_type) {
              case x509_entry_v2: TimestampedCertificateEntryDataV2;
              case precert_entry_v2: TimestampedCertificateEntryDataV2;
              case x509_sct_v2: SignedCertificateTimestampDataV2;
              case precert_sct_v2: SignedCertificateTimestampDataV2;
              case signed_tree_head_v2: SignedTreeHeadDataV2;
              case consistency_proof_v2: ConsistencyProofDataV2;
              case inclusion_proof_v2: InclusionProofDataV2;
          } data;
      } TransItem;
This is pretty much the universal TLS convention.

Line 1649
6.4.  transparency_info TLS Extension
This extension appears not to have any explicit support for CT entries for
intermediate certs. Am I just supposed to glue together all the TransItems?

Line 1651
   Provided that a TLS client includes the "transparency_info" extension
   type in the ClientHello, the TLS server SHOULD include the
You need to provide an actual definition of what the client includes, and
having the server ignore the contents is bad mojo. TLS convention is for
the client to include an empty extension and the server to validate that it
is in fact empty.

Line 1654
   "transparency_info" extension in the ServerHello with
   "extension_data" set to a "TransItemList".  The TLS server SHOULD
   ignore any "extension_data" sent by the TLS client.  Additionally,
IMPORTANT: The normative language here is kind of confusing. It SHOULD
include the extension but if it's included, it MUST consist of
TransItemList, no? And surely only SHOU
Also, I'm not sure this is the right logic. If the server knows that it has
the SCT information in the certificate or in OCSP, why SHOULD It send this
extension. I would think, rather that servers should aim to send
information at most once, so that it should only send the extension if it
contains information that's not in the cert/OCSP. as it pretty much has to
send those anyway. Otherwise, don't we just end up in a world where if this
info is in OCSP and certs, it's always sent twice, because the client
doesn't know where the info is, and so has to always offer the extension.

Line 1658
   session is resumed, since session resumption uses the original
   session information.
Does this mean the client MUST abort the handshake if the server includes
it?

Line 1668
   o  The TLS client includes the "transparency_info" extension type in
      the ClientHello.
This condition is non-sensical, because if the client *doesn't* include the
extension, the server cannot send the transparency_info extension at all.

Line 1722
8.  Clients
Given the imminent standardization of TLS 1.3, you need to somehow provide
a mapping for client-side CT for that, I think

Line 1739
   view.  The exact mechanisms will be in separate documents, but it is
   expected there will be a variety.
Given the somewhat science fictional status of Gossip, this entire
paragraph should be stricken

Line 1747
   MUST implement all of the three mechanisms by which TLS servers may
   present SCTs (see Section 6).  TLS clients MAY also accept SCTs via
   the "status_request_v2" extension ([RFC6961]).  TLS clients that
IMPORTANT: This also needs to be rewritten so it makes clear it's not a
general levy on TLS clients.

Line 1770
   In addition to normal validation of the server certificate and its
   chain, TLS clients SHOULD validate each received SCT for which they
   have the corresponding log's parameters.  To validate an SCT, a TLS
IMPORTANT: Why is this a SHOULD and not a MUST? If you support CT at all,
why would you not do this?

Line 1791
   TLS clients MUST NOT consider valid any SCT whose timestamp is in the
   future.
What's the reason for this? If your clock is slightly wrong, this is going
to cause new certs to fail even if they otherwise would have succeeded
(because the notBefore and notAfter are more conservative).

Line 1800
   will disclose to the log which TLS server the client has been
   communicating with.
IMPORTANT: This "Note" just mentions in passing a huge privacy issue. You
need to be a lot clearer about this.

Line 1823
   "transparency_info" and "status_request" TLS extensions in the
   ClientHello.
IMPORTANT: This is not consistent with the requirements on the server.
Trying to reconstruct the reasoning here, the client can only decide that
the server is noncompliant if it has given the server a chance to send the
SCTs by every mechanism., otherwise the server might just want to send the
SCT some other way. However, if servers can optionally ignore
transparency_info (it's a SHOULD above), then you can have two compliant
implementations with the server having a CT-compliant cert and yet the
client declares it noncompliant. To fix this, you need to require the
server to respond to "transparency_info"

Line 1831
   "CachedObject" of type "ct_compliant" in the "cached_info" extension.
   The "hash_value" field MUST be 1 byte long with the value 0.
You should explain why this is one byte long (that the PDU is defined as
having a minimum length of 1). Also the server should be required to check
it.

Line 1842
   watches.  It may also want to keep copies of entire logs.  In order
   to do this, it should follow these steps for each log:
Why is this not a 2119 SHOULD?

Also, what does "in order to do this" refer to? Clearly not how to keep
copies.... Presumably, how to poll the log.

Line 1864
   8.  Either:
IMPORTANT: You seem to be missing there part where you actually look at the
entries to verify that they don't contain bogus data (e.g., certificates
for your domain). I get that it's implicit here, but given that you provide
an algorithm, that should be an explicit stage.
This is a pretty odd algorithm. If I understand it correctly, 1-4 are setup
steps and then 5-9 is supposed to be repeated, but I could just do this
once and stop at 4.

Line 1912
   STHs it receives, ensure that each entry can be fetched and that the
   STH is indeed the result of making a tree from all fetched entries.
IMPORTANT: How do you verify MMD?

Line 1944
   If it should become necessary to deprecate an algorithm used by a
   live log, then the log should be frozen as specified in Section 4.13
   and a new log should be started.  Certificates in the frozen log that
RFC 2119 SHOULD? Isn't this a MUST, though?

Line 1958
   "transparency_info" TLS extension.  IANA should update this extension
   type to point at this document.
IMPORTANT: You'll need to fill in the new field specified in
https://tlswg.github.io/draft-ietf-tls-iana-registry-updates/#rfc.section.6

Line 2009
   |                                | ECDSA (NIST P-256) |             |
   |                                | with HMAC-SHA256   |             |
   |                                |                    |             |
Why are you defining both algorithms?

Line 2150
   (with the intention of actually running a CT log that will be
   identified by the allocated Log ID).
This seems like it's not a great thing to be asking an expert to do, as it
seems to require business arrangements. Is it really that valuable to save
a few bytes here?

Line 2163
   that the log has misbehaved, which will be discovered when the SCT is
   audited.  A signed timestamp is not a guarantee that the certificate
   is not misissued, since appropriate monitors might not have checked
IMPORTANT: This is not correct, because the client does not know that the
monitors are verifying the data that it is. See my general comments on
public verifiability above.

Line 2182
   operating correctly.  As a log is allowed to serve an STH that's up
   to MMD old, the maximum period of time during which a misissued
   certificate can be used without being available for audit is twice
Nit: up to the MMD old

Line 2211
   compute the proofs from the log) or communicate with the log via
   proxies.
This also seems quite handwavy in light of the facts on the ground.

Line 2237
      and STHs can be stored by the log and served to other clients that
      submit the same certificate or request the same STH.
This needs to be expanded. Who is this risk against? The log or someone
else? If the log, what's the logs incentive?

Line 2243
   reduce the effectiveness of an attack where a CA and a log collude
   (see Section 6.1).
See my comments in 6.1 about this.

---

From: Ben Laurie <[email-masked]>
Date: Tue, 5 Sep 2017 17:04:56 +0100

On 4 September 2017 at 00:17, Eric Rescorla <[email-masked]> wrote:

> RPs only accept certificates with >1 SCT.
>

The purpose of multiple SCTs is to avoid the death of a single log causing
the death of a large number of certificates. It is not about trust.

> I've noted a number of

Past tense seems correct?

> Why is this a 2119 MUST? It seems wise, but not necessarily a conformance

"To avoid being overloaded by invalid submissions"

> submitter provides superfluous certificates?

Not allowed.

> don't see a cite. Please provide one,

See s1.2.

> Is this true? What happens if we have two CAs that share a key?

Eh?

>    specified here.  These extra fields should be ignored.

Probably should be.

> "will"? What's the normative force of this?

The normative requirements are in the various messages below.

> construction because the submitter provides a complete path?

Yes.

> " latest STH < requested STH: Return latest STH"

Agree.

> It's not actually concatenation, right? It's all the fields.

union would probably be a better word.

> How does the client know which of the above two cases has occurred?

The response includes an STH, which says how big the tree is. Probably
should be the latest one known to that server.

> This needs to somehow be clear that it only applies to TLS servers that
> are compliant with this specification, as it's not a new requirement on all
> TLS servers.
>

Surely its the other way round: i.e. new requirements on all TLS servers
have to be made clear?

> Also, I'm not sure this is the right logic. If the server knows that it
> has the SCT information in the certificate or in OCSP, why SHOULD It send
> this extension. I would think, rather that servers should aim to send
> This condition is non-sensical, because if the client *doesn't* include
> the extension, the server cannot send the transparency_info extension at
> all.
>
>
> Line 1722
> 8.  Clients
> IMPORTANT: You seem to be missing there part where you actually look at
> the entries to verify that they don't contain bogus data (e.g.,
> certificates for your domain). I get that it's implicit here, but given
> that you provide an algorithm, that should be an explicit stage.
> This is a pretty odd algorithm. If I understand it correctly, 1-4 are
> setup steps and then 5-9 is supposed to be repeated, but I could just do
> this once and stop at 4.
>
>
> Line 1912
> _______________________________________________
> Trans mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/trans
>
>

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 5 Sep 2017 10:46:18 -0700

> the death of a large number of certificates. It is not about trust.

That seems like the reason for the server to offer it, but not for the
client to require it.

Line 763
> Past tense seems correct?

I'm willing to dert this to the RFC Editor.

> "To avoid being overloaded by invalid submissions"

There seem like one way to prevent this, but not the only one.

>> submitter provides superfluous certificates?

Hmm...  So your theory is that the submitter does path construction

>> don't see a cite. Please provide one,

OK. Thanks.

>> Is this true? What happens if we have two CAs that share a key?

Say that a CA spins up two subordinates that happen to share the same key.
I agree it's foolish.
 return an empty

> Surely its the other way round: i.e. new requirements on all TLS servers

I'm not sure what you mean. This document does not get to require that all
TLS servers do CT. Was that your intent?

-EKr

>> Assuming that the server is malicious, why would it include multiple
>> SCTs? It seems like requiring multiple SCTs does in fact provide this
>> defense, but that's not an argument for servers to provide multiples.
>>
>>
>> Line 1627
>> This extension appears not to have any explicit support for CT entries
>> for intermediate certs. Am I just supposed to glue together all the
>> TransItems?
>>
>>
>> Line 1651
>> Given the imminent standardization of TLS 1.3, you need to somehow
>> provide a mapping for client-side CT for that, I think
>>
>>
>> Line 1739
>> What's the reason for this? If your clock is slightly wrong, this is
>> going to cause new certs to fail even if they otherwise would have
>> succeeded (because the notBefore and notAfter are more conservative).
>>
>>
>> Line 1800
>> This seems like it's not a great thing to be asking an expert to do, as
>> it seems to require business arrangements. Is it really that valuable to
>> save a few bytes here?
>>
>>
>> Line 2163

---

From: Andrew Ayer <[email-masked]>
Date: Wed, 6 Sep 2017 20:12:30 -0700

On Tue, 5 Sep 2017 10:46:18 -0700
Eric Rescorla <[email-masked]> wrote:

> Say that a CA spins up two subordinates that happen to share the same
> key. I agree it's foolish.

That's not a problem, since the name of the CA is included in the
Issuer field of the TBSCertificate, which is part of the Merkle leaf
input along with the CA's public key hash.

Regards,
Andrew

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 6 Sep 2017 21:34:19 -0700

> input along with the CA's public key hash.

Thanks. That's helpful.

-Ekr

---

From: Ben Laurie <[email-masked]>
Date: Thu, 7 Sep 2017 11:24:51 +0100

On 5 September 2017 at 18:46, Eric Rescorla <[email-masked]> wrote:

>> The purpose of multiple SCTs is to avoid the death of a single log
>> causing the death of a large number of certificates. It is not about trust.
>>
>
>>> Why is this a 2119 MUST? It seems wise, but not necessarily a
>>> conformance requirement
>>>
>>

Yep - and that's borne out by practice.

> TLS servers do CT. Was that your intent?

What I meant is that if every time you mention a thing you have to say "but
only if you are conforming to this RFC" then you a) make the whole thing a
lot more cumbersome, b) are stating the obvious (i.e. that you only have to
conform to the RFC if you have decided to conform to the RFC).

Surely if you are making a new requirement for TLS compliance then you have
to explicitly say so?

>>> Structurally, it's kind of a mess to have this be the place that you
>>> make TransItems self-contained (by having a defined length field). What
>>> about other places I might want to concatenate TransItems. Why don't you
>>> instead make TransItem self-contained, like so:
>>>
>>> struct {
>>> You need to provide an actual definition of what the client includes,
>>> and having the server ignore the contents is bad mojo. TLS convention is
>>> for the client to include an empty extension and the server to validate
>>> that it is in fact empty.
>>>
>>>
>>> Line 1654
>>> Does this mean the client MUST abort the handshake if the server
>>> includes it?
>>>
>>>
>>> Line 1668
>>> IMPORTANT: Why is this a SHOULD and not a MUST? If you support CT at
>>> all, why would you not do this?
>>>
>>> Line 1791
>>> IMPORTANT: This "Note" just mentions in passing a huge privacy issue.
>>> You need to be a lot clearer about this.
>>>
>>> Line 1823
>>> Trying to reconstruct the reasoning here, the client can only decide
>>> that the server is noncompliant if it has given the server a chance to send
>>> the SCTs by every mechanism., otherwise the server might just want to send
>>> the SCT some other way. However, if servers can optionally ignore
>>> IMPORTANT: This is not correct, because the client does not know that
>>> the monitors are verifying the data that it is. See my general comments on

---

From: Eric Rescorla <[email-masked]>
Date: Thu, 7 Sep 2017 07:41:45 -0700

> Yep - and that's borne out by practice.

OK, well, then it would help if the document were clearer on this point.

>> I'm not sure what you mean. This document does not get to require that
>> all TLS servers do CT. Was that your intent?
>>
>
> What I meant is that if every time you mention a thing you have to say
> "but only if you are conforming to this RFC" then you a) make the whole
> thing a lot more cumbersome, b) are stating the obvious (i.e. that you only
> have to conform to the RFC if you have decided to conform to the RFC).
>

I don't think it's merely stating the obvious in that we have *other* RFCs
which actually do attempt to retrospectively impose requirements on all
uses of TLS. See, for instance: https://tools.ietf.org/html/rfc7465. And
this is a different story, in that we are not (I assume) going to go around
saying that TLS stacks that don't do CT are not TLS conformant.

I think the text I would use here would be "CT-using TLS servers MUST..."

> to explicitly say so?

Well, my point is that the way that this document is written in fact does
so, and so you have to look at "Updates" to find out that that's not the
case.

-Ekr

>>>> You should explain why this is one byte long (that the PDU is defined
>>>> as having a minimum length of 1). Also the server should be required to
>>>> check it.
>>>>
>>>>
>>>> Line 1842