Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Client Certificate Verification when Using Extended Master Secret #143

Conversation

DunyaKokoschka
Copy link
Contributor

@DunyaKokoschka DunyaKokoschka commented Nov 17, 2022

Currently it is not possible for UTLS clients to connect to OpenSSL servers (and probably any RFC compliant server) when performing mutual TLS authentication and using the extended master secret extension (and using TLSv1.2)

The session hash used in extended master secret does not include the CertificateVerify handshake message. see: https://www.rfc-editor.org/rfc/rfc7627#section-3

   When a full TLS handshake takes place, we define

         session_hash = Hash(handshake_messages)

   where "handshake_messages" refers to all handshake messages sent or
   received, starting at the ClientHello up to and including the
   ClientKeyExchange message, including the type and length fields of
   the handshake messages.  This is the concatenation of all the
   exchanged Handshake structures, as defined in Section 7.4 of
   [RFC5246].

Note: It should be possible to calculate the master secret on both the client and the server after the client has sent the ClientKeyExchange message. If the CertificateVerify message was part of the hash then this would not be possible on the server. This is also how openssl implements extended master secret. For example you can see from the control flow in openssl that the client key exchange message triggers generating the master key:

tls_process_cke_ecdhe -> ssl_derive -> ssl_gensecret -> ssl_generate_master_secret -> tls1_generate_master_secret

@@ -600,6 +600,9 @@ func (hs *clientHandshakeState) doFullHandshake() error {
}
}

/* sessionHash does not include CertificateVerify */
sessionHash := hs.finishedHash.Sum()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this file might not be strictly following this but, could you enclose your modified sections in any added file (file name NOT starting with u_) like below:

// [UTLS SECTION START]
... modified section
// [UTLS SECTION END]

This will greatly help with readability and maintainability. Note we are routinely syncing with upstream and any modification leading to merge conflicts, without properly labeled as modified by us, might be overwritten/discarded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I can do it if you don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the the section comments. the whole of the extended-master-secret is a UTLS addition so I put the section comments around the whole if statement that checks for extended-master-secret.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton!

@gaukas gaukas merged commit ef21c92 into refraction-networking:master Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants