Fix pipelining in 3.1/3.0 #20208

mattcaswell · 2023-02-03T17:11:56Z

This PR backports the fixes to a number of pipelining bugs that were resolved in master recently to 3.1/3.0. It also adds a new test for the pipelining code which would have caught these issues.

It is mostly a backport of PR #19456, plus an additional fix which was included in a much larger commit in master (c618679) related to moving the pipelining code into the new record layer that exists there.

Fixes #20197

TLS pipelining provides the ability for libssl to read or write multiple records in parallel. It requires special ciphers to do this, and there are currently no built-in ciphers that provide this capability. However, the dasync engine does have such a cipher, so we add a test for this capability using that engine.

…lled The pipeline input/output buf arrays must remain accessible to the EVP_CIPHER_CTX until EVP_Cipher is subsequently called. This fixes an asan error discovered by the newly added pipeline test.

During read pipelining we must ensure that the buffer is sufficiently large to read enough data to fill our pipelines. We also remove some code that moved data to the start of the packet if we can. This was unnecessary because of later code which would end up moving it anyway. The earlier move was also incorrect in the case that |clearold| was 0. This would cause the read pipelining code to fail with sufficiently large records. Fixes openssl#20197

Document the effect on the internal read buffer when using pipelining.

We shouldn't be putting more data into a pipeline than the value of split_send_fragment. This is a backport of a fix which was included in a much larger commit in master (c618679) related to moving the pipelining code into the new record layer that exists there.

mattcaswell · 2023-02-21T14:47:14Z

Ping for second review

ssl/record/ssl3_buffer.c

openssl-machine · 2023-02-23T23:00:29Z

This pull request is ready to merge

TLS pipelining provides the ability for libssl to read or write multiple records in parallel. It requires special ciphers to do this, and there are currently no built-in ciphers that provide this capability. However, the dasync engine does have such a cipher, so we add a test for this capability using that engine. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20208)

…lled The pipeline input/output buf arrays must remain accessible to the EVP_CIPHER_CTX until EVP_Cipher is subsequently called. This fixes an asan error discovered by the newly added pipeline test. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20208)

During read pipelining we must ensure that the buffer is sufficiently large to read enough data to fill our pipelines. We also remove some code that moved data to the start of the packet if we can. This was unnecessary because of later code which would end up moving it anyway. The earlier move was also incorrect in the case that |clearold| was 0. This would cause the read pipelining code to fail with sufficiently large records. Fixes #20197 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20208)

We shouldn't be putting more data into a pipeline than the value of split_send_fragment. This is a backport of a fix which was included in a much larger commit in master (c618679) related to moving the pipelining code into the new record layer that exists there. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 2c4b1c7)

TLS pipelining provides the ability for libssl to read or write multiple records in parallel. It requires special ciphers to do this, and there are currently no built-in ciphers that provide this capability. However, the dasync engine does have such a cipher, so we add a test for this capability using that engine. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 24c7d36)

…lled The pipeline input/output buf arrays must remain accessible to the EVP_CIPHER_CTX until EVP_Cipher is subsequently called. This fixes an asan error discovered by the newly added pipeline test. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit df9c7ce)

During read pipelining we must ensure that the buffer is sufficiently large to read enough data to fill our pipelines. We also remove some code that moved data to the start of the packet if we can. This was unnecessary because of later code which would end up moving it anyway. The earlier move was also incorrect in the case that |clearold| was 0. This would cause the read pipelining code to fail with sufficiently large records. Fixes openssl#20197 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 1d06598)

We shouldn't be putting more data into a pipeline than the value of split_send_fragment. This is a backport of a fix which was included in a much larger commit in master (c618679) related to moving the pipelining code into the new record layer that exists there. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 2c4b1c7)

TLS pipelining provides the ability for libssl to read or write multiple records in parallel. It requires special ciphers to do this, and there are currently no built-in ciphers that provide this capability. However, the dasync engine does have such a cipher, so we add a test for this capability using that engine. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 24c7d36)

…lled The pipeline input/output buf arrays must remain accessible to the EVP_CIPHER_CTX until EVP_Cipher is subsequently called. This fixes an asan error discovered by the newly added pipeline test. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit df9c7ce)

During read pipelining we must ensure that the buffer is sufficiently large to read enough data to fill our pipelines. We also remove some code that moved data to the start of the packet if we can. This was unnecessary because of later code which would end up moving it anyway. The earlier move was also incorrect in the case that |clearold| was 0. This would cause the read pipelining code to fail with sufficiently large records. Fixes openssl#20197 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 1d06598)

We shouldn't be putting more data into a pipeline than the value of split_send_fragment. This is a backport of a fix which was included in a much larger commit in master (c618679) related to moving the pipelining code into the new record layer that exists there. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#20208) (cherry picked from commit 2c4b1c7)

bernd-edlinger · 2023-07-20T06:08:37Z

test/sslapitest.c

+        goto end;
+
+    if (!TEST_true(SSL_set_cipher_list(clientssl, "AES128-SHA")))
+        goto end;


hmm, @mattcaswell, when any of these goto end are executed and idx == 5
then the statement below

if (idx == 5) OPENSSL_free(msg);

will crash, because msg is still initialized from above:

unsigned char *msg = (unsigned char *) "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz123";

It would be better to report this as separate issue, or a PR with a fix. Comments in closed PRs often get lost/ignored.

OK, done: #21512