Conversation
|
|
||
| BAIL_OUT("failed to fork Sieve listener: $!") unless defined $server_pid; | ||
|
|
||
| if ($server_pid == 0) { |
There was a problem hiding this comment.
So we seem to create a server, listen out for a command (which we assume will be STARTTLS), respond with "ok" - and then abort without doing TLS. This will cause s_client to fail the handshake (but hopefully not crash).
This is all fine, but it may not be immediately obvious to someone coming at this test cold what the point of the test is. Perhaps some comment somewhere explaining the objective of the test.
There was a problem hiding this comment.
Indeed. The test stops after the plaintext Sieve STARTTLS exchange because the regression is in the pre-TLS response parser: the later TLS handshake failure is expected. I’ll add a comment explaining that, and I’ll also add an explicit sanitizer output check so an ASan report isn't hidden by the expected nonzero s_client exit.
There was a problem hiding this comment.
I implemented the changes and force pushed the commit. I also rebased the commit on current master.
7dbc2b5 to
471e2e2
Compare
openssl-machine
added
the
approval: review pending
471e2e2 to
fe9a607
Compare
mattcaswell
added
branch: master
|
Look like there is a conflict backporting this to 3.0, so we'll need a different PR for that branch. |
|
I will open a new PR dedicated to 3.0 branch. For the CI macOS readonly-source failure, it is not related to this PR. The only failing test is 70-test_quic_radix.t / quic_radix_test, while this PR only touches apps/s_client.c and test/recipes/20-test_app_s_client.t. |
|
I opened #31483 for the 3.0 backport. |
openssl-machine
added
approval: done
|
This pull request is ready to merge |
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from #31468)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from #31468) (cherry picked from commit 6ecdedb)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from #31468) (cherry picked from commit 6ecdedb)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from #31468) (cherry picked from commit 6ecdedb)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from #31468) (cherry picked from commit 6ecdedb)
|
Merged to the master, 4.0, 3.6, 3.5 and 3.4 branches. Thank you for your contribution. |
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from openssl#31468)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> MergeDate: Wed Jun 17 16:22:20 2026 (Merged from openssl#31468)
5 participants
Fixes #31443
openssl s_client -starttls sievecopied the first two bytes of the Sieve STARTTLS response intosbufwithstrncpy(sbuf, mbuf, 2)and then passedsbuftomake_uppercase. For an exact two-byte response such as ok, the copied buffer was not NUL terminated, somake_uppercasecould walk past the intended response code, reading and potentially writing until a NUL byte was found.Replace this with a direct case-insensitive comparison of the first two response bytes using
OPENSSL_strncasecmp(mbuf, "OK", 2). The code already verifies that at least two bytes were read before this check.A regression test is added for
s_client -starttls sievewith a local Sieve stub returning an exact lowercaseokresponse.Checklist