Hi Willy,
Thank you for the reply.
On Tue, Aug 24, 2021 at 8:12 PM Willy Tarreau wrote:
> Hi Marcin,
>
> I'm finally back to your patch set! Overall that looks fine, but I have
> some comments, mostly cosmetic.
>
> > From b3a254b41411f22307a622250a6e95ac39fefee8 Mon Sep 17 00:00:00 2001
> > From: Marcin Deranek
> > Date: Mon, 12 Jul 2021 14:16:55 +0200
> > Subject: [PATCH 1/5] MEDIUM: ssl: Capture more info from Client Hello
> (...)
>
> > diff --git a/include/haproxy/ssl_sock-t.h b/include/haproxy/ssl_sock-t.h
> > index 5acedcfc5..4b993894a 100644
> > --- a/include/haproxy/ssl_sock-t.h
> > +++ b/include/haproxy/ssl_sock-t.h
> > @@ -202,8 +202,16 @@ struct ssl_sock_msg_callback {
> > /* This memory pool is used for capturing clienthello parameters. */
> > struct ssl_capture {
> > unsigned long long int xxh64;
> > - unsigned char ciphersuite_len;
> > - char ciphersuite[VAR_ARRAY];
> > + unsigned short int protocol_version;
> > + unsigned short int ciphersuite_len;
> > + unsigned short int extensions_len;
> > + unsigned short int ec_len;
>
> No need to mention "int" after "short" or "long", that's implicit. I
> guess you did it because of the "unsigned long long int" above but that
> is not needed. Since 2.4 we have shorter type names like "ushort", "uint",
> "uchar" etc that we're progressively trying to standardize the new code
> to, but they haven't slipped into older code yet. This could make sense
> here since the whole structure is being replaced. But that's not an
> obligation :-)
>
Converted all to new types, so we are future proof :-)
> > diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> > index ba61243ac..2965a1446 100644
> > --- a/src/ssl_sock.c
> > +++ b/src/ssl_sock.c
> > @@ -1652,7 +1652,16 @@ static void ssl_sock_parse_clienthello(struct
> connection *conn, int write_p, int
> > struct ssl_capture *capture;
> > unsigned char *msg;
> > unsigned char *end;
> > - size_t rec_len;
> > + unsigned char *extensions_end;
> > + unsigned char *ec_start = NULL;
> > + unsigned char *ec_formats_start = NULL;
> > + unsigned char *list_end;
> > + unsigned short int protocol_version;
> > + unsigned short int extension_id;
> > + unsigned short int ec_len = 0;
>
> Same here for the type names.
>
Also converted.
> > @@ -1747,10 +1763,106 @@ static void ssl_sock_parse_clienthello(struct
> connection *conn, int write_p, int
> > capture->xxh64 = XXH64(msg, rec_len, 0);
> >
> > /* Capture the ciphersuite. */
> > - capture->ciphersuite_len = (global_ssl.capture_cipherlist <
> rec_len) ?
> > - global_ssl.capture_cipherlist : rec_len;
> > - memcpy(capture->ciphersuite, msg, capture->ciphersuite_len);
> > + capture->ciphersuite_len = MIN(global_ssl.capture_cipherlist,
> rec_len);
> > + capture->ciphersuite_offset = 0;
> > + memcpy(capture->data, msg, capture->ciphersuite_len);
> > + msg += rec_len;
> > + offset += capture->ciphersuite_len;
> > +
> > + /* Initialize other data */
> > + capture->protocol_version = protocol_version;
> > + capture->extensions_len = 0;
> > + capture->extensions_offset = 0;
> > + capture->ec_len = 0;
> > + capture->ec_offset = 0;
> > + capture->ec_formats_len = 0;
> > + capture->ec_formats_offset = 0;
>
> Given the number of fields to preset to zero, you should instead replace
> the previous pool_alloc() call with a pool_zalloc(), it would be more
> future-proof and safer against the risk of accidentally missing one
> such field when more are added.
>
Understood. replaced pool_alloc() with pool_zalloc() are removed
initialization of individual fields as they are not needed anymore.
> > + /* Expect 2 bytes extension id + 2 bytes extension size */
> > + msg += 2 + 2;
> > + if (msg + rec_len > extensions_end || msg + rec_len < msg)
> > + goto store_capture;
> > + if (extension_id == 0x000a) {
> > + /* Elliptic Curves */
>
> Could you put a link here to the registry where you found this definition ?
> I guess it comes from an RFC or anything, but it does help when some
> extensions are needed or when dealing with interoperability issues, to
> figure if any other values need to be considered.
>
Added links to the list of known TLS extensions and to individual RFCs
covering used extensions.
> > + if (ec_start) {
> > + rec_len = MIN(global_ssl.capture_cipherlist - offset,
> ec_len);
>
> I'm not fond of using MIN() here as you're dealing with two different
> types (one signed int due to the subtract, and an unsigned short). It
> actually does the right thing but it's fragile. What about this:
>
> rec_len = ec_len;
> if (offset + rec_len > global_ssl.capture_cipherlist)
> rec_len = global_ssl.capture_cipherlist - offset;
>
Converted both cases.
> > +ssl_fc_cipherlist_
