Fix for TLS handshake issue with GnuTLS #28902 by martinRa2 · Pull Request #28955 · openssl/openssl

martinRa2 · 2025-10-19T16:45:06Z

This is a fix for the Issue

TLS handshake fails between OpenSSL 3.6.0 and GnuTLS #28902

The OCSP response is now looked up before the packet is created. If the correct OCSP response is not found the extension for TLS 1.3 is not sent.

I have to put in a workaround for test_tls13messages and test_sslmessages. For some reason I don't know, the hash for the issuer name I get is 0x01 0x02 ... 0x015. I introduced a workaround that in this case the check if the OCSP response is the correct one is skipped.

Checklist

tests are added or updated

beldmit · 2025-10-19T17:01:02Z

apps/s_server.c


+        /*
+         * in the case the root CA certificate is not included in the chain
+         * we assme that the last remaining response is issued by it


Nit: assume

vdukhovni · 2025-10-20T05:59:01Z

My preference would for multi-stapling support to be withdrawn from OpenSSL until we have a robust well-documented design for that feature. The new s_client option to implement OCSP verification would then be the only retained feature of the original PR.

As it stands, as in the initial review, I am not convinced we have a robust design for the lifecycle and robust construct of multi-stapled server responses.

mattcaswell · 2025-10-20T08:41:24Z

For some reason I don't know, the hash for the issuer name I get is 0x01 0x02 ... 0x015

That will be because the ossltest provider is in use. That supplies various dummy implementations for various hash functions:

openssl/test/p_ossltest.c

Lines 193 to 207 in ca95d13

    
           /** 
        
            * @brief Finalize the digest output. 
        
            * 
        
            * provide pre-set data (increasing count) for the SHA1 DIGEST 
        
            * 
        
            * @param md unsigned char *md. 
        
            * @param ctx void *ctx. 
        
            * @return int. 
        
            */ 
        
           static int ossltest_SHA1_final(unsigned char *md, void *ctx) 
        
           { 
        
               fill_known_data(md, SHA_DIGEST_LENGTH); 
        
               return 1; 
        
           }

Those particular tests are using the TLSProxy which uses dummy ciphers and hashes so that the proxy can amend TLS messages for test purposes without breaking the handshake Finished messges.

We really should not be putting workarounds for test code into the library itself.

martinRa2 · 2025-10-21T13:43:51Z

I created a new OCSP response using the test provider and removed the workaround.

t8m · 2025-10-21T18:40:48Z

The CI is relevant. It would be also good if you could please include a test for the regression.

martinRa2 · 2025-10-21T19:53:02Z

What type of test do you have in mind?

t8m · 2025-10-22T15:12:59Z

What type of test do you have in mind?

Could a testcase be added to one of the tls13 test recipes?

martinRa2 · 2025-10-23T11:35:26Z

I had a look at the failed CI/non-caching job and found that in that particular case the correct issuer name SHA-1 value is calculated when the OCSP responses are checked.
In all other github jobs the dummy value 00 ... 13 are returned by the test provider, in that particular case not.

I'm a little bit lost on that point. Implement a workaround I understood is not a good idea. A other solution would be to remove the check if the correct OCSP response is returned and just take the one which is on this place.

@mattcaswell , @t8m any adivse?

DDvO · 2025-10-24T07:20:58Z

apps/s_server.c

The variable name server_certs is confusing.
Please rename to, e.g., server_chain and add a comment that it may include the root (TA cert) or not.

DDvO · 2025-10-24T07:29:08Z

apps/s_server.c

        /* issuer certificate is next in chain */
        issuer = sk_X509_value(server_certs, i);

+        /*
+         * in the case the root CA certificate is not included in the chain
+         * we assume that the last remaining response is issued by it
+         */
+        if (issuer == NULL && i == (num - 1) && sk_OCSP_RESPONSE_num(sk_resp_unordered) == 1) {
+            resp = sk_OCSP_RESPONSE_value(sk_resp_unordered, 0);
+            (void)sk_OCSP_RESPONSE_push(*sk_resp, resp);
+            sk_OCSP_RESPONSE_delete(sk_resp_unordered, 0);
+            continue;
+        }


Better not even attempt to call issuer = sk_X509_value(server_certs, i) in case i is beyond the end of the chain.

And this whole loop with all its exceptions, including this new one, is a mess.
Please re-think it as a whole and come up with a more straightforward and well-readable overall algorithm.

martinRa2 · 2025-10-27T11:54:26Z

The CI is relevant. It would be also good if you could please include a test for the regression.

I'm still struggling with the failed check. Can anybody tell me why in this particular test not the test value from the test engine is returned?

nhorman · 2025-10-27T12:13:55Z

@martinRa2 Can you run the failing test(s) locally with V=1 and post the output here. It almost certainly has something to do with @mattcaswell s comments here

davidben · 2025-10-27T13:50:24Z

ssl/statem/extensions_srvr.c

+    resp = get_ocsp_response(s, chainidx);
+
+    /* If no OCSP response was found the extension is not sent */
+    if (SSL_CONNECTION_IS_TLS13(s) && resp == NULL)


If, somehow, the application has configured an OCSP response for an intermediate certificate, but not the leaf, I think this PR will still get it wrong? It is also forbidden to send an empty CertificateStatus message. I believe this means you should unconditionally skip the extension when resp == NULL, not only if SSL_CONNECTION_IS_TLS13(s).

(Footnote: is chainidx guaranteed to be zero in contexts where this is not relevant? This would rely on this.)

In the comment below it is mentioned that for TLS <=1.2 an empty extension is send to the client. Therefore, the check here is to see if we have a response for TLS 1.3 or not. If not we don't send the extension for the current certificate.

I checked. If for the leaf certificate is no OCSP response is given, no response is send to the client. I have to check why.

In TLS 1.2, you send an empty extension to the client to signal that you are about to staple a response. If you aren't going to staple a response, you shouldn't send the extension.

(Strictly speaking, servers are allowed to signal the extension and then skip the CertificateStatus message. But that doesn't seem to be what the PR is doing (seems to be sending an empty CertificateStatus message, if I'm reading it right? It's not returning CON_FUNC_DONT_SEND), and is also kinda silly because the server already knows whether it has an OCSP response at this point.)

davidben · 2025-10-27T13:53:30Z

ssl/statem/statem_srvr.c

+
+    resp = get_ocsp_response(s, 0);
+
+    if (resp != NULL && !tls_construct_cert_status_body(s, resp, pkt)) {


Related to the comment in extensions_srvr.c, I don't believe this resp != NULL check is correct. It'll just send an empty CertificateStatus message, which isn't allowed. I believe this should trigger ERR_R_INTERNAL_ERROR or an assert or whatever because, after fixing extensions_srvr.c, it should be impossible to get to this point without an OCSP response.

davidben · 2025-10-27T13:55:13Z

ssl/statem/statem_srvr.c

-        }
-    }
-
    if (resp != NULL)


I believe resp == NULL is now impossible and should probably fail with ERR_R_INTERNAL_ERROR or an assert or whatever is preferred these days.

If resp ever were NULL, I believe you all would output a syntax error.

(This condition is now impossible.)

martinRa2 · 2025-10-28T13:23:24Z

@martinRa2 Can you run the failing test(s) locally with V=1 and post the output here. It almost certainly has something to do with @mattcaswell s comments here

@nhorman here are the output from my tests. It is shown that the test provider is used even in the failed test cases, but somehow the hash for the issuer name is not set to the fixed value 000102030405060708090A0B0C0D0E0F as in the other jobs. Instead the correct SHA-1 value for the issuer name hash is provided, but only for the failed test cases.
Therefore, the check if the correct ocsp response is given failed and no response is send to the client and the test case fails.

If I know which parameter influences that at this point the correct hash value is used I could implement the test that it uses a other response file with that hash values.
Other possibility would be to remove the check if the ocsp response is correct and just send it to the client. I'm not sure if that helps in this case, as the check then might fail on the client side.

test_sslmessages_5_output.txt
test_sslmessages_output.txt

nhorman · 2025-10-28T14:58:11Z

@martinRa2 Thank you for the traces. though I'm not sure I see in the trace what you are observing (certificate issuer hashes, as far as I can see aren't printed in the logs).

At the moment, what I can say (keeping in mind that I'm not super familiar with with ocsp) is:

I'm able to reproduce the problem locally with your PR here, which is good.
If I augment the TLSProxy code to record keylogs, and take a tcpdump during the sslmessages test, I see the following in a failed test:

The things I note about this primarily is that the certificate status response has a zero length, and an unkown response type. I think at minumum, that response should be at least 4 bytes with a value between 0 and 6 (based on my read of RFC 2560.

Now the server may be responding with such a malformed response based on its inability to parse the certificate above, I'm honestly not sure, I just wanted to share what I've observed so far.

martinRa2 · 2025-10-28T16:22:46Z

@martinRa2 Thank you for the traces. though I'm not sure I see in the trace what you are observing (certificate issuer hashes, as far as I can see aren't printed in the logs).

At the moment, what I can say (keeping in mind that I'm not super familiar with with ocsp) is:

I'm able to reproduce the problem locally with your PR here, which is good.

If I augment the TLSProxy code to record keylogs, and take a tcpdump during the sslmessages test, I see the following in a failed test:

The things I note about this primarily is that the certificate status response has a zero length, and an unkown response type. I think at minumum, that response should be at least 4 bytes with a value between 0 and 6 (based on my read of RFC 2560.
Now the server may be responding with such a malformed response based on its inability to parse the certificate above, I'm honestly not sure, I just wanted to share what I've observed so far.

Thanks for analyzing the issue. An empty CertificateStatus message is according RFC 6066 not correct. I have to analyze why this is the case.

For the test case I added some output to check the issuer name hash in the OCSP response and the calculated one from the certificate:

 ===> respIssuerNameHash =  00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13
 ===> certIssuerNameHash =  14 e1 82 e7 51 10 7e fb f7 47 20 5f fa eb d1 2e 7e 26 c1 4d

If I use a response file with the correct issuer name hash the test passes with that configuration.

martinRa2 · 2025-10-28T16:27:00Z

ocsp-response_noprov.der.gz

This is the file with the correct issuer name hash. You should place it in ./test/recipes/ocsp-response.der

nhorman · 2025-10-28T17:18:52Z

just a note, the empty ocsp response is likely related to #28989

martinRa2 · 2025-10-29T15:47:27Z

@nhorman @davidben
I spent the afternoon testing the version with different clients

GNU Wget 1.21.1
curl 7.76.1
Chrome Version 141.0.7390.122 (with OCSP activated)
Firefox 140.4.0esr (64-Bit)
The current version of BoringSSL form googlesource

everything on my CentOS computer (Linux artax 5.14.0-630.el9.x86_64)

I didn't face any problems with TLS 1.2 and 1.3

Please let me know which issues you observed exactly. I will try to reproduce them here.

davidben · 2025-10-29T16:14:26Z

Did you configure the server to serve TLS 1.2, with a stapled OCSP response for the intermediate and not the leaf? That's the scenario where the PR seems to not get things right. Since TLS 1.2 can only staple a response for the leaf, it should not send the status_request extension at all.

martinRa2 · 2025-11-18T07:52:38Z

@t8m the testing problem should now be fixed.

@rsith71 your suggestion looks promising. My priority is currently on fixing the current issue, but I will have a look at it and do some testing.

t8m

There are also some unresolved comments from @bob-beck if you could please address them.

t8m · 2025-11-18T08:49:29Z

ssl/statem/statem_srvr.c

                if (cert_id_md_oid != NULL)
-                    cert_id_md = EVP_get_digestbyobj(cert_id_md_oid);
+                    cert_id_md = EVP_MD_fetch(sctx->libctx,
+                                              OBJ_nid2sn(OBJ_obj2nid(cert_id_md_oid)), sctx->propq);


I think using OBJ_obj2txt() would be better way as it avoids going through nids. Please note that algorithms should be fetchable by the textual representation of the oid.

t8m · 2025-11-18T08:56:13Z

apps/s_server.c

+    SSL_get0_chain_certs(s, &server_chain);
+    ssl_ctx = SSL_get_SSL_CTX(s);
+    SSL_CTX_get0_chain_certs(ssl_ctx, &server_chain);


This doesn't make sense - why do you first call it on SSL when the second call on SSL_CTX will overwrite it?

t8m · 2025-11-18T08:57:43Z

ssl/ssl_local.h


 __owur int send_certificate_request(SSL_CONNECTION *s);

+OCSP_RESPONSE *get_ocsp_response(SSL_CONNECTION *s, int chainidx);


Please rename the function to ossl_get_ocsp_response

apps/s_server.c

When a Cert status body is received and the length of the response is zero error out. This PR is dependent on openssl#28955 It will not pass tests until this PR is merged into master.

rsith71 · 2025-11-24T16:05:41Z

I have this PR to fix the client to fail when it receives a cert status body of zero length: The tests will fail until this PR is merged into master.

#29207

openssl-machine · 2025-11-28T11:00:47Z

This pull request is ready to merge

t8m · 2025-11-28T14:53:03Z

Squashed and merged with a rewritten commit message to the master and 3.6 branches. Thank you.

If the OCSP response was not present for a certificate the server created a non-conforming empty CertificateStatus extension instead of not sending the extension at all. Fixes #28902 Fixes b1b4b15 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28955)

If the OCSP response was not present for a certificate the server created a non-conforming empty CertificateStatus extension instead of not sending the extension at all. Fixes #28902 Fixes b1b4b15 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28955) (cherry picked from commit c5c8b44)

If the OCSP response was not present for a certificate the server created a non-conforming empty CertificateStatus extension instead of not sending the extension at all. Fixes openssl#28902 Fixes b1b4b15 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#28955)

When a Cert status body is received and the length of the response is zero error out. This PR is dependent on openssl#28955 It will not pass tests until this PR is merged into master.

If the OCSP response was not present for a certificate the server created a non-conforming empty CertificateStatus extension instead of not sending the extension at all. Fixes openssl#28902 Fixes b1b4b15 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#28955)

3.6.1 CHANGES.md includes the following: * #28760 "Improve the CPUINFO display for RISC-V" * #28797 "Fix regression when X509_V_FLAG_CRL_CHECK_ALL is set" * #28955 "Fix for TLS handshake issue with GnuTLS #28902" * #29155 "fix(x509.c): fixed -checkend return values" * #29214 "s390x: Check and fail on invalid malformed ECDSA signatures" * #29245 "Clang format 3.6" * #29251 "Fix change of behavior of the single stapled OCSP response API" 3.6.1 NEWS.md includes the following: * #28797 "Fix regression when X509_V_FLAG_CRL_CHECK_ALL is set" * #28955 "Fix for TLS handshake issue with GnuTLS #28902" Co-Authored-by: Tomáš Mráz <[email protected]> Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Nikola Pajkovsky <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Mon Jan 26 20:01:30 2026

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 19, 2025

martinRa2 mentioned this pull request Oct 19, 2025

TLS handshake fails between OpenSSL 3.6.0 and GnuTLS #28902

Closed

beldmit reviewed Oct 19, 2025

View reviewed changes

martinRa2 force-pushed the master branch from 70ec7a5 to aaa5fe6 Compare October 21, 2025 13:42

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Oct 21, 2025

t8m requested a review from mattcaswell October 21, 2025 18:37

t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.6 Applies to openssl-3.6 labels Oct 21, 2025

t8m added the severity: regression The issue/pr is a regression from previous released version label Oct 21, 2025

DDvO reviewed Oct 24, 2025

View reviewed changes

davidben reviewed Oct 27, 2025

View reviewed changes

changed to EVP_MD_fetch

ec2a0f9

t8m requested changes Nov 18, 2025

View reviewed changes

Review from Tomas

7b9d754

t8m reviewed Nov 19, 2025

View reviewed changes

apps/s_server.c Outdated Show resolved Hide resolved

fixed styling issue

960cc92

t8m added the tests: deferred Tests will be added in a subsequent PR (label should be removed when the PR with tests is merged) label Nov 20, 2025

t8m approved these changes Nov 20, 2025

View reviewed changes

rsith71 mentioned this pull request Nov 24, 2025

Updating the Client's parsing of the Cert Status Body #29207

Closed

1 task

loci-dev mentioned this pull request Nov 24, 2025

UPSTREAM PR #29207: Updating the Client's parsing of the Cert Status Body auroralabs-loci/openssl#120

Open

1 task

t8m requested a review from DDvO November 25, 2025 13:30

t8m linked an issue Nov 25, 2025 that may be closed by this pull request

TLS handshake fails between OpenSSL 3.6.0 and GnuTLS #28902

Closed

t8m requested a review from a team November 26, 2025 14:23

mattcaswell approved these changes Nov 27, 2025

View reviewed changes

mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 27, 2025

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 28, 2025

t8m closed this Nov 28, 2025


		resp = get_ocsp_response(s, 0);

		if (resp != NULL && !tls_construct_cert_status_body(s, resp, pkt)) {


		__owur int send_certificate_request(SSL_CONNECTION *s);

		OCSP_RESPONSE get_ocsp_response(SSL_CONNECTION s, int chainidx);

Uh oh!

Comments

Conversation

martinRa2 commented Oct 19, 2025 • edited by vdukhovni Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdukhovni commented Oct 20, 2025

Uh oh!

mattcaswell commented Oct 20, 2025

Uh oh!

martinRa2 commented Oct 21, 2025

Uh oh!

t8m commented Oct 21, 2025

Uh oh!

martinRa2 commented Oct 21, 2025

Uh oh!

t8m commented Oct 22, 2025

Uh oh!

martinRa2 commented Oct 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinRa2 commented Oct 27, 2025

Uh oh!

nhorman commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinRa2 commented Oct 28, 2025

Uh oh!

nhorman commented Oct 28, 2025

Uh oh!

martinRa2 commented Oct 28, 2025

Uh oh!

martinRa2 commented Oct 28, 2025

Uh oh!

nhorman commented Oct 28, 2025

Uh oh!

martinRa2 commented Oct 29, 2025

Uh oh!

davidben commented Oct 29, 2025

Uh oh!

martinRa2 commented Nov 18, 2025

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rsith71 commented Nov 24, 2025

Uh oh!

openssl-machine commented Nov 28, 2025

Uh oh!

t8m commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

martinRa2 commented Oct 19, 2025 •

edited by vdukhovni

Loading

DDvO Oct 24, 2025 •

edited

Loading