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

On 4 September 2017 at 00:17, Eric Rescorla <ekr@rtfm.com> wrote:

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

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

Past tense seems correct?

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

"To avoid being overloaded by invalid submissions"

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

Not allowed.

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

See s1.2.

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

Eh?

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

Probably should be.

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

The normative requirements are in the various messages below.

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

Yes.

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

Agree.

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

union would probably be a better word.

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

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

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

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

> 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.
>
>
> _______________________________________________
> Trans mailing list
> Trans@ietf.org
> https://www.ietf.org/mailman/listinfo/trans
>
>