s_server: Report an error if init-connection fails without an attempt to read #18154

faramir-dev · 2022-04-22T12:41:26Z

…connection.

Checklist

documentation is added or updated
tests are added or updated

bernd-edlinger · 2022-04-22T12:55:05Z

note the typo in the commit message

t8m · 2022-04-22T13:37:15Z

Have you verified that this won't break s_server if for example in dtls mode if client sends some broken client hello or some combination of algorithms the server does not support?

t8m · 2022-04-26T07:32:45Z

There is one more way how you could possibly solve this issue. Set a callback function on the read bio with BIO_set_callback_ex() and if the callback is not called during the init connection call, it means the failure does not have anything with the connection attempt.

…to read. Fixes: openssl#18047.

t8m · 2022-04-29T06:55:59Z

apps/s_server.c

+
+    if (s_debug) {
+        BIO_set_callback_arg(bio, (char* )bio_s_out);
+        bio_dump_callback(bio, cmd, argp, len, argi, argl, ret, processed);


Although the bio_dump_callback() does not modify the ret value we should do here ret = (int)bio_dump....

t8m · 2022-04-29T06:57:37Z

apps/s_server.c

+    if (s_debug) {
+        BIO_set_callback_arg(bio, (char* )bio_s_out);
+        bio_dump_callback(bio, cmd, argp, len, argi, argl, ret, processed);
+        BIO_set_callback_arg(bio, (char* )p_counter);


Should be (char *)

t8m · 2022-04-29T06:58:25Z

apps/s_server.c

+static long count_reads_callback(BIO *bio, int cmd, const char *argp, size_t len,
+                                 int argi, long argl, int ret, size_t *processed)
+{
+    unsigned *p_counter = (unsigned*)BIO_get_callback_arg(bio);


(unsigned *)

Please make this unsigned int * rather than just unsigned *

mattcaswell

Looks good. Some minor nits below

mattcaswell · 2022-04-29T10:21:47Z

apps/s_server.c

+    }
+
+    if (s_debug) {
+        BIO_set_callback_arg(bio, (char* )bio_s_out);


mattcaswell · 2022-04-29T10:25:30Z

apps/s_server.c

+static long count_reads_callback(BIO *bio, int cmd, const char *argp, size_t len,
+                                 int argi, long argl, int ret, size_t *processed)
+{
+    unsigned *p_counter = (unsigned*)BIO_get_callback_arg(bio);


Please make this unsigned int * rather than just unsigned *

mattcaswell · 2022-04-29T10:25:49Z

apps/s_server.c

+                 * It helps us to recognise configuration errors and errors
+                 * caused by a client.
+                 */
+                unsigned read_counter = 0;


unsigned int read_counter = 0;

…ttempt to read.

paulidale

Approved with the style nits fixed and possible with the changed comment (or a not changed comment).

apps/s_server.c

paulidale · 2022-05-03T04:45:26Z

apps/s_server.c

@@ -2328,6 +2328,30 @@ static void print_stats(BIO *bio, SSL_CTX *ssl_ctx)
               SSL_CTX_sess_get_cache_size(ssl_ctx));
 }

+static long count_reads_callback(BIO *bio, int cmd, const char *argp, size_t len,


Since we're being pedantic about the int part: long int.

t8m · 2022-05-04T07:33:13Z

@faramir-dev ping for fixups

…ttempt to read.

t8m · 2022-05-05T07:30:26Z

@paulidale still OK?

openssl-machine · 2022-05-06T10:00:16Z

This pull request is ready to merge

…to read. Fixes: #18047. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18154)

…to read. Fixes: #18047. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18154) (cherry picked from commit a6d52f1)

t8m · 2022-05-06T12:59:33Z

Merged to master and 3.0 branches. Thank you.

It needs adjustments for 1.1.1. @faramir-dev could you please create a backported PR for 1.1.1?

faramir-dev requested a review from mattcaswell April 22, 2022 12:41

faramir-dev self-assigned this Apr 22, 2022

t8m changed the title ~~s_serve: Report an error if non-recoverable error occurs in init_ssl_connection~~ s_server: Report an error if non-recoverable error occurs in init_ssl_connection Apr 25, 2022

faramir-dev force-pushed the daniel/18047 branch 2 times, most recently from 2deda1d to 923d521 Compare April 26, 2022 06:33

faramir-dev changed the title ~~s_server: Report an error if non-recoverable error occurs in init_ssl_connection~~ s_server: Report an error if there are too many init-connection failures. Apr 26, 2022

faramir-dev force-pushed the daniel/18047 branch from dc93512 to 3445859 Compare April 29, 2022 04:13

s_serve: Report an error if init-connection fails without an attempt …

85e987e

…to read. Fixes: openssl#18047.

faramir-dev force-pushed the daniel/18047 branch from 3445859 to 85e987e Compare April 29, 2022 06:12

faramir-dev changed the title ~~s_server: Report an error if there are too many init-connection failures.~~ s_server: s_serve: Report an error if init-connection fails without an attempt to read Apr 29, 2022

faramir-dev changed the title ~~s_server: s_serve: Report an error if init-connection fails without an attempt to read~~ s_server: Report an error if init-connection fails without an attempt to read Apr 29, 2022

t8m reviewed Apr 29, 2022

View reviewed changes

mattcaswell requested changes Apr 29, 2022

View reviewed changes

fixup! s_serve: Report an error if init-connection fails without an a…

d2db590

…ttempt to read.

t8m approved these changes May 2, 2022

View reviewed changes

t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels May 2, 2022

paulidale approved these changes May 3, 2022

View reviewed changes

fixup! s_serve: Report an error if init-connection fails without an a…

bf47c09

…ttempt to read.

t8m approved these changes May 5, 2022

View reviewed changes

mattcaswell approved these changes May 5, 2022

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 May 5, 2022

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 May 6, 2022

t8m removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label May 6, 2022

t8m closed this May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

s_server: Report an error if init-connection fails without an attempt to read #18154

s_server: Report an error if init-connection fails without an attempt to read #18154

faramir-dev commented Apr 22, 2022

bernd-edlinger commented Apr 22, 2022

t8m commented Apr 22, 2022

t8m commented Apr 26, 2022

t8m Apr 29, 2022

t8m Apr 29, 2022

t8m Apr 29, 2022

mattcaswell Apr 29, 2022

mattcaswell left a comment

mattcaswell Apr 29, 2022

mattcaswell Apr 29, 2022

mattcaswell Apr 29, 2022

paulidale left a comment

paulidale May 3, 2022

t8m commented May 4, 2022

t8m commented May 5, 2022

openssl-machine commented May 6, 2022

t8m commented May 6, 2022

s_server: Report an error if init-connection fails without an attempt to read #18154

s_server: Report an error if init-connection fails without an attempt to read #18154

Conversation

faramir-dev commented Apr 22, 2022

Checklist

bernd-edlinger commented Apr 22, 2022

t8m commented Apr 22, 2022

t8m commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8m commented May 4, 2022

t8m commented May 5, 2022

openssl-machine commented May 6, 2022

t8m commented May 6, 2022