SSL_CTX_set_ssl_version() preserves TLS 1.3 ciphersuites #7226
Comments
|
It looks like the TLS 1.3 specific cipher list is attempted to be used first if possible when // ssl/ssl_ciph.c
STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(...) {
...
/*
* Allocate new "cipherstack" for the result, return with error
* if we cannot get one.
*/
if ((cipherstack = sk_SSL_CIPHER_new_null()) == NULL) {
OPENSSL_free(co_list);
return NULL;
}
/* Add TLSv1.3 ciphers first - we always prefer those if possible */
for (i = 0; i < sk_SSL_CIPHER_num(tls13_ciphersuites); i++) {
if (!sk_SSL_CIPHER_push(cipherstack,
sk_SSL_CIPHER_value(tls13_ciphersuites, i))) {
sk_SSL_CIPHER_free(cipherstack);
return NULL;
}
}
/*
* The cipher selection for the list is done. The ciphers are added
* to the resulting precedence to the STACK_OF(SSL_CIPHER).
*/
for (curr = head; curr != NULL; curr = curr->next) {
if (curr->active) {
if (!sk_SSL_CIPHER_push(cipherstack, curr->cipher)) {
OPENSSL_free(co_list);
sk_SSL_CIPHER_free(cipherstack);
return NULL;
}
#ifdef CIPHER_DEBUG
fprintf(stderr, "<%s>\n", curr->cipher->name);
#endif
}
}
OPENSSL_free(co_list); /* Not needed any longer */
...
} |
|
It's not "instead"; the other ciphers are added after the TLS 1.3 ciphers. But that's not really the question I was trying to ask about, which is more about whether setting the |
|
@kaduk yes, I see what you are saying now. Thank you for the clarification. It does look like the TLS 1.3 ciphers are added no matter the method, but I will defer to the tagged maintainers that you mentioned on this behavior. |
|
I don't think I'm following what you are trying to achieve here specifically. |
|
I claim that the following set of operations has "unusual and surprising" behavior due to our TLS 1.3-specific implementation: in that instead of getting the default ciphers for the |
|
I would expect the ciphersuite list to remain the same before and after the SSL_CTX_set_ssl_version call as conceptually the cipher suite settings are against the SSL_CTX not against the SSL_METHOD. |
|
And checking the actual behaviour - it is precisely that - changing the method does not alter the output from SSL_get_ciphers after each method change; and you do see a difference depending on the version-specific method for SSL_get1_supported_ciphers exactly as I would have expected. |
|
Would this be an appropriate case for a unit test, @t-j-h? |
|
I'm trying to figure out the point @kaduk is trying to make that I'm not following before I get a sense of whether or not there is some unexpected behaviour or anything that would suggest another test case would be useful. |
|
The historical behavior of But the behavior for TLS 1.3 cipher suites is different from this historical pattern; they are not reset by Whether or not this behavior of |
Historically SSL_CTX_set_ssl_version() has reset the cipher list to the default. Splitting TLS 1.3 ciphers to be tracked separately caused a behavior change, in that TLS 1.3 cipher configuration was preserved across calls to SSL_CTX_set_ssl_version(). To restore commensurate behavior with the historical behavior, set the ciphersuites to the default as well as setting the cipher list to the default. Closes: openssl#7226
|
The concept of changing the method associated with an SSL_CTX after it has been created is just horrible. I'm also not sure why anyone would want to do that. Really once an SSL_CTX has been created an used to create SSL objects we should not be modifying it. So this limits the usefulness of this function to the period between when the SSL_CTX is first created, and when it is first used to create an SSL. This only really makes sense if using a fixed version method. All of those are now deprecated - so we really ought to deprecate this function too. That all being said, and on the assumption that we have to live with it, the fix proposed by @kaduk looks ok. |
Historically SSL_CTX_set_ssl_version() has reset the cipher list to the default. Splitting TLS 1.3 ciphers to be tracked separately caused a behavior change, in that TLS 1.3 cipher configuration was preserved across calls to SSL_CTX_set_ssl_version(). To restore commensurate behavior with the historical behavior, set the ciphersuites to the default as well as setting the cipher list to the default. Closes: #7226 Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7270) (cherry picked from commit 2340ed2)
|
A better way to look at it is to see the differences in handling between ciphersuites and the cipher list. They are different - they don't do the same thing. After SSL_CTX_set_cipher_list the list is small and the defaults are gone. Now I would argue that resetting the cipher list or the cipher suites is actually the wrong thing to be doing in this function - but that is a separate topic - but having a function which has an entirely undocumented side effect (resetting things) should be fixed. Right now you are noting that a non-documented behaviour isn't consistent with a new concept and I think that is because it wasn't documented. |
I would argue that "large" vs. "small" is not quite the best way to look at things, either, as there are different reference points to compare to. There are some 300-odd pre-TLS 1.3 ciphersuites currently defined, and only 5 TLS 1.3 ciphersuites defined (we only have three of them in our default list). Internally, our code is being forced (for some reasonably good reasons) to squash the TLS 1.2 and TLS 1.3 ciphersuites into a single list, but they are conceptually different and non-interchangable. So it's not that the ciphersuites are "prepended to" the list; they are just the first part of the list, when we squash different things into a single container. So I would think of this as "after SSL_CTX_set_cipher_list, we have 3/3 TLS 1.3 and (small/large) TLS 1.2", and "after SSL_CTX_set_ciphersuites, we have 1/3 TLS 1.3 and (large/large) TLS 1.2". The 1/3 and small/large seem commensurate to me, and more meaningful than the squashed-together (3+small)/(large) and (1+large)/large.
I would also argue that it's the wrong thing to be doing, but not very hard, as I'd be arguing that the function shouldn't exist in the first place :) I'm happy to prepare some documentation (which will note that this function should be considered deprecated, at least in the first cut). |
SSL_CTX_set_ssl_version()is intended to adapt anSSL_CTXto a new (presumably, fixed-version)SSL_METHOD, filtering down the cipher list to ciphers that are supported by the new method. However, there is no TLS 1.3-specific method, so for any fixed-version method, no TLS 1.3 ciphers should apply. The current implementation of this function just blindly preserves the TLS 1.3 ciphers in the resulting (combined) cipher list, which does not seem like the correct behavior.This is an issue and not a pull request because I'm not sure if we need to handle the case of
SSL_CTX_set_ssl_version(., TLS_method()).The text was updated successfully, but these errors were encountered: