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

Track use of extensions to avoid duplicate extensions in the same message #114

Open
wants to merge 3 commits into
base: tls13-prototype
Choose a base branch
from

Conversation

gbryant-arm
Copy link
Collaborator

Introduce helper functions to track extensions and detect duplicate extensions (i.e. of the same type) in the same message (RFC requirement). Send an alert if that's the case. Fixes #97.

Implemented for TLS 1.3 only to avoid conflicts with upstream Mbed TLS.
It is assumed that NewSessionTicket and Certificate don't include any extensions.
Extension tracking has not been implemented for writing extensions (yet?)

…id writing

or parsing more than one extension of the same type in a given extension block
(RFC requirement)
…est,

ServerHello, EncryptedExtensions and CertificateRequest
}
else if( ssl->handshake->extensions_present & SUPPORTED_GROUPS_EXTENSION && ssl->handshake->extensions_present & SIGNATURE_ALGORITHM_EXTENSION )
else if( mbedtls_ssl_extensions_present( ssl, SUPPORTED_GROUPS_EXTENSION | SIGNATURE_ALGORITHM_EXTENSION, 0 ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocker: This is functionally incorrect since the present implementaiton of mbedtls_ssl_extensions_present() only checks whether a non-zero part of the flag is present, not whether the entire flag is present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed mbedtls_ssl_extensions_present() to support a conjunction of extensions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It may be easier to independently call mbedtls_ssl_extensions_present() for each extension rather than calling it once to check several extensions though...

@@ -2467,6 +2467,9 @@ static int ssl_certificate_request_parse( mbedtls_ssl_context* ssl,
/*
* Parse extensions
*/

mbedtls_ssl_reset_extensions_present( ssl );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this change made?

@hanno-becker
Copy link
Collaborator

Thanks a lot @gbryant-arm for your work on this.

I'm a bit uncomfortable with how we're overloading the use of a single variable here:

  1. ssl->handshake_extensions globally tracks which extensions have been present in the various handshake messages, CH, SH, EE, CR, Ticket. From what I understand, it is important that ssl->handshake_extensions is global because of the possibility of extensions in later messages depends on their presence in earlier messages.
  2. The issue this PR sets out to solve is a local one: Check that within each message, there's no duplication of extensions.

The danger of this overload is best seen in the clearing of the extensions_present flag at the beginning of some HS messages: That's fine from viewpoint 2., but it's not clear whether it breaks 1.

Going into more detail, I took a look at where and how extensions_present is currently used.

  • Server-side:
    • Track which extensions have been included in the ClientHello
    • Use that to decide which extensions to include in the ServerHello
      It is necessary to keep the information on what the client has offered until we write the ServerHello, so we cannot make this state-local.
    • Notable omission: Do not track which extensions the server writes in ServerHello
  • Client-side:
    • Track which extensions the client writes in ClientHello
    • This is only used for convenience in the ClientHello writing routine itself: For example, we add the KeyShare extension if and only SupportedGroups and SignatureAlgorithms. This, I think, should be removed, and we should have fresh checks for each extensions. This would then allow to entirely remove the use of extensions_present in the ClientHello writing.
    • Track which extensions the server has sent in its ServerHello.
    • The extensions_present flag is cleared between ClientHello writing and ServerHello parsing.
    • In particular, we do not check whether we allow certain extensions in the ServerHello on the basis of whether we have included them in the ClientHello ourselves. This logic is currently entirely missing for some extensions: For example, IIUC it would be illegal for the server to include the ALPN extension in the ServerHello if the client hasn't offered it, but the client-code does currently not check this. If it did, it would require either a flag indicating whether it has sent the extension, or re-play the logic determining whether ALPN should be allowed.

To be continued... still not sure what the best approach is here.

@gbryant-arm gbryant-arm added the needs work Some requests need addressing label Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs work Some requests need addressing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants