On Tue, Sep 5, 2017 at 9:04 AM, Ben Laurie <[email protected]> wrote:
>
>
> On 4 September 2017 at 00:17, Eric Rescorla <[email protected]> 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.
>
That seems like the reason for the server to offer it, but not for the
client to require it.
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?
>
I'm willing to dert this to the RFC Editor.
>
>
>>
>>
>> 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"
>
There seem like one way to prevent this, but not the only one.
> 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.
>
Hmm... So your theory is that the submitter does path construction
>
>
>>
>>
>> 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.
>
OK. Thanks.
>
>> 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?
>
Say that a CA spins up two subordinates that happen to share the same key.
I agree it's foolish.
return an empty
>> 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?
>
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
>
>> 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
>> [email protected]
>> https://www.ietf.org/mailman/listinfo/trans
>>
>>
>
_______________________________________________
Trans mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/trans
