crash in set_client_ciphersuite if clntsess->cipher = NULL at line 4780 in sslapitest.c

[avoget]

Hello.

OpenSSL 3.0 has UB in set_client_ciphersuite(SSL s, const unsigned char cipherchars) line #1356 in file openssl/ssl/statem/statem_clnt.c ... if (md == NULL || md != ssl_md(s->ctx, s->session->cipher->algorithm2)) { ... if s->session->cipher is NULL

To reproduce crash:

1. Change openssl/test/sslapitest.c at line # 4780 clntsess->cipher = NULL;
2. Rebuild tests
3. Start sslapitest

Regards.

[avoget]

PR to fix problem https://github.com/openssl/openssl/pull/22418

[t8m]

@mattcaswell can this ever happen?

[t8m]

i.e., is this a real bug fix?

[avoget]

As i understand the comment at line 1341: /*

• Depending on the session caching (internal/external), the cipher
• and/or cipher_id values may not be set. Make sure that cipher_id is
• set and use it for comparison. */ s->session->cipher may be NULL

[t8m]

Yeah, but there the s->hit is set and I believe the cipher will be set then.

[avoget]

May be it is a good idea to add this comment before this condition ? Relation of s->hit and s->session->cipher is not obvious.

[t8m]

@mattcaswell ping?

[mattcaswell]

@mattcaswell can this ever happen?

I don't see how it could ever happen in reality. sslapitest is "cheating" in that it is making modifications to an internal data structure. No real application should be doing this. Having an assert that s->session->cipher is non NULL might not be a bad idea - but it doesn't look like a bug to me.

[avoget]

Hello Matt. Thank you ! I see.

[nhorman]

Given that the consensus here is that this is not a bug, I'm marking this as inactive, to be closed at the end of the 3.4 development cycle. Please comment if there is a further issue