Certificate selection for TLS13

[ocheron]

Adds support for extension "signature_algorithms_cert" and uses hash/signature pairs when deciding about the server certificate (for preference only, not enforcement).

I include an unrelated fix of Fallback SCSV because clientVersion will not be TLS13.

[kazu-yamamoto]

I have mereged the last four commits since they are trivial.
I continue to check the first two commits with #298.

[kazu-yamamoto]

OK. The first two commits are friendly to #298.
I would like to support this change but I don't know whether or not @vdukhovni supports this since he suggested to ignore signature_algorithms_cert.

[vdukhovni]

This is simply wrong. The server's leaf certificate MUST be compatible with signature_algorithms, to the extent that its leaf certificate public key must be usable with one of the HashAndSignatureAlgorithm code points supported on both sides. That check is not obviated by the client's signature_algorithms_cert extension.

As for signature_algorithms_cert it must not be allowed to leave the list of server credentials empty. It should either be completely ignored, or applied to the output of the initial signature algorithms filter to find a preferred list of credentials, but if that list turns out empty, then go with the original list instead. TLS is doing layer violation when it intrudes on the validity of X.509 certificates. We don't know whether the client's list of supported certificate signature algorithms is accurate, and it is ultimately up to the client to validate the signature or not. We must never have the server deny the client the choice of accepting the certificate chain anyway.

In summary the initial filtering for signature_algorithms looks only at the leaf public key, while the optional signature_algorithms_cert compatibility check looks at the full chain, but must never be allowed to fail to produce a non-empty result. The two filter functions are different!

In the server->client direction (which I don't believe is being handled here), things are a bit different, since the client certificate may be optional. It is perhaps better to not send a client certificate at all, than to send one that's not acceptable to the server, but that's not always the case. Some server insist on client certificates, but don't validate their certificate chain (might only care about the leaf public key). Other servers might admit client certificates, but the handshake might fail if the client certificate fails to verify. There's no single right choice. It's all trade-offs. My preference would be to ignore signature_algorithms_cert (as in my client auth PR) and see if anyone complains. This is something that might need to be configurable. TLS is a rather complex protocol. :-(

[kazu-yamamoto]

I don't understand the leaf vs full issue. Both code for signature_algorithms and signature_algorithms_cert are exactly the same. So, they behave the same.

[vdukhovni]

@kazu-yamamoto I don't understand the leaf vs full issue. Both code for signature_algorithms and signature_algorithms_cert are exactly the same. So, they behave the same.

While the syntax of the two extensions is the same, the semantics could hardly be more different. We should chat, if you're free, give me a call on Hangouts.

[ocheron]

@vdukhovni Can you explain what is wrong?

I understand your confusion if you think this is related to #194 but it is not.

The PR is the non-empty producing result that you described for "signature_algorithms_cert".

With limitation that it analyzes only the signature of a non self-signed leaf, not the full certificate chain. For the reason given in a previous PR related to TLS12. The server does not know what the client considers as trust anchor.

[vdukhovni]

I don't think I'm confused. This PR calls the same validation function with either signature_algorithms_cert when provided or else signature_algorithms. But we should not treat these as interchangeable. signature_algorithms needs to be applied strictly to the leaf key only, while signature_algorithms_cert:

• Is a best effort filter, if nothing we have matches it, then we should ignore this extension.
• DOES NOT obviate the need to still check signature_algorithms.
  So the code does not look right as-is.

[ocheron]

Yes the extensions are not interchangeable, since one is tried and the other is used only when the first one is not found.

RFC 8446 section 4.2.3:

If no "signature_algorithms_cert" extension is
present, then the "signature_algorithms" extension also applies to
signatures appearing in certificates.

The additional check you ask with "signature_algorithms" is already there with credentialsFindForSigning13.

[vdukhovni]

@ocheron The additional check you ask with "signature_algorithms" is already there with credentialsFindForSigning13.

Perhaps you're right, but in that case, my complaint is that the code is substantially under-commented. You may have noticed detailed comment blocks explaining non-trivial rationale for various aspects of the code in my client authentication patch. Here the filterCredentialsWithHashSignatures which is doing something non-trivial that's also inter-related with credentialsFindForSigning13 is has no comments at all. As a result the code is unmaintainable.

You need to write the intent for the various functions, rationale for trade-offs made and and any important relationships to the behaviour of other functions. A lot more "literate programming" is required for a maintainable security toolkit.

[vdukhovni]

If the code is now feature-complete, please consider reading through it top to bottom and adding a paragraph or two of comments to each non-trivial function, where non-trivial does not mean that the code is necessarily complex, but rather than the purpose of the function is not necessarily clear, though of course tricky code could also be commented, but more typically best avoided.

[ocheron]

Added documentation to the offending function.

[kazu-yamamoto]

In summary the initial filtering for signature_algorithms looks only at the leaf public key, while the optional signature_algorithms_cert compatibility check looks at the full chain, but must never be allowed to fail to produce a non-empty result. The two filter functions are different!

OK. I think that I understand this now.

[kazu-yamamoto]

It seems to me that @vdukhovni's interpretation if both signature_algorithms and signature_algorithms_cert exist. However, RFC 8466 sec 4.2.3 says:

If no "signature_algorithms_cert" extension is
present, then the "signature_algorithms" extension also applies to
signatures appearing in certificates.

So, if only signature_algorithms exists, it is used as signature_algorithms_cert. Thus, 07c0467 looks good to me.

[kazu-yamamoto]

55981f5 also looks good to me.

[kazu-yamamoto]

@vdukhovni Do you agree with my opinion?

[vdukhovni]

@kazu-yamamoto @vdukhovni Do you agree with my opinion?

My concern was about signature_algorithms_cert being used a hard requirement on the local certificate chain. That was addressed by @ocheron clarifying that this filter is ignored when nothing passes and adding commentary explaining the semantics. Yes, it is OK to apply signature_algorithms when signature_algorithms_cert is not provided.

[kazu-yamamoto]

OK. Merged.
Thank you for discussing!