Hi Willy,
(first, before I forget, the patch should be backported in 1.8)
Test case was very fun: I have a HAProxy listening on 127.1:443 with my
cert. When I "curl" it, I could get the list of ciphers sent by the client
in the log using the relevant fetch and when I used Chrome or Firefox, I
could not.
After reading the code (of both HAProxy and OpenSSL) and tcpduming the
traffic, I saw that curl did not send the SSL session ID while
Chrome/firefox did (I might had one generated long time ago).
When I do run curl/chrome/firefox with the patch enabled, I can get the
cipher list in the log line.
I just RTFM curl, and I can see it now has a ssl session id cache when you
ask for multiple requests on the same line:
Add the following line in a frontend:
http-request set-header X-TLS %[ssl_fc_cipherlist_xxh,debug]
Run HAProxy in debug mode.
Run the following curl command:
curl https://127.0.0.1/ https://127.0.0.1/
and check the output:
00000006:f_public.accept(0005)=000b from [127.0.0.1:40747] ALPN=<none>
00000006:f_public.clireq[000b:ffffffff]: GET / HTTP/1.1
00000006:f_public.clihdr[000b:ffffffff]: Host: 127.0.0.1
00000006:f_public.clihdr[000b:ffffffff]: User-Agent: curl/7.47.0
00000006:f_public.clihdr[000b:ffffffff]: Accept: */*
[debug converter] type: sint <4615001522986040631>
00000006:default.srvrep[000b:adfd]: HTTP/1.0 503 Service Unavailable
00000006:default.srvhdr[000b:adfd]: Cache-Control: no-cache
00000006:default.srvhdr[000b:adfd]: Content-Type: text/html
00000006:default.srvcls[000b:adfd]
00000006:default.clicls[000b:adfd]
00000006:default.closed[000b:adfd]
00000007:f_public.accept(0005)=000b from [127.0.0.1:40793] ALPN=<none>
00000007:f_public.clireq[000b:ffffffff]: GET / HTTP/1.1
00000007:f_public.clihdr[000b:ffffffff]: Host: 127.0.0.1
00000007:f_public.clihdr[000b:ffffffff]: User-Agent: curl/7.47.0
00000007:f_public.clihdr[000b:ffffffff]: Accept: */*
00000007:default.srvrep[000b:adfd]: HTTP/1.0 503 Service Unavailable
00000007:default.srvhdr[000b:adfd]: Cache-Control: no-cache
00000007:default.srvhdr[000b:adfd]: Content-Type: text/html
00000007:default.srvcls[000b:adfd]
00000007:default.clicls[000b:adfd]
00000007:default.closed[000b:adfd]
You can see the debug converter which displays the list of ciphers for the
first request, when there are no session id, and does not display it for
the second request.
Now, the output when the patch is applied:
00000000:f_public.accept(0005)=000b from [127.0.0.1:45115] ALPN=<none>
00000000:f_public.clireq[000b:ffffffff]: GET / HTTP/1.1
00000000:f_public.clihdr[000b:ffffffff]: Host: 127.0.0.1
00000000:f_public.clihdr[000b:ffffffff]: User-Agent: curl/7.47.0
00000000:f_public.clihdr[000b:ffffffff]: Accept: */*
[debug converter] type: sint <4615001522986040631>
00000000:default.clicls[000b:adfd]
00000000:default.closed[000b:adfd]
00000001:f_public.accept(0005)=000b from [127.0.0.1:45145] ALPN=<none>
00000001:f_public.clireq[000b:ffffffff]: GET / HTTP/1.1
00000001:f_public.clihdr[000b:ffffffff]: Host: 127.0.0.1
00000001:f_public.clihdr[000b:ffffffff]: User-Agent: curl/7.47.0
00000001:f_public.clihdr[000b:ffffffff]: Accept: */*
[debug converter] type: sint <4615001522986040631>
00000001:default.srvcls[000b:adfd]
00000001:default.clicls[000b:adfd]
00000001:default.closed[000b:adfd]
You can see the cipher list for both connections.
I am unfortunately not familiar with reg-test, but I can have a look at it
and contribute one if you want.
Baptiste
Post by Willy TarreauHi Baptiste,
Post by BaptisteHi there,
There is a small bug with the function involved in the
ssl_fc_cipherlist_*
Post by Baptistefetches.
ssl_sock_parse_clienthello() improperly parses the session id, which
leads
Post by Baptisteto not return any client side cipher list when a client send a SSL
session
Post by BaptisteID to resume a session.
This patch aims at fixing this issue.
Thanks for this. Would you happen to have a test case for this ? I *think*
it's correct based on some other sample fetch functions which also take a
single byte for the session ID length, but if you have a simple test case
it would be great.
Also CCing Thierry who developed it in case he can test it.
Post by BaptisteFrom f2c79803c6bcb69866f54c8a5833bd0178bea64c Mon Sep 17 00:00:00 2001
Date: Wed, 28 Nov 2018 15:20:25 +0100
Subject: [PATCH] BUG/MINOR: ssl: ssl_sock_parse_clienthello ignores
session id
Post by BaptisteIn ssl_sock_parse_clienthello(), the code considers that SSL Sessionid
size is '1', and then considers that the SSL cipher suite is availble
right after the session id size information.
This actually works in a single case, when the client does not send a
session id.
This patch fixes this issue by introducing the a propoer way to parse
the session id and move forward the cursor by the session id length when
required.
---
src/ssl_sock.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index a73fb2d..c48c4af 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1561,10 +1561,19 @@ void ssl_sock_parse_clienthello(int write_p, int
version, int content_type,
Post by Baptiste/* Expect 2 bytes for protocol version (1 byte for major and 1 byte
* for minor, the random, composed by 4 bytes for the unix time and
- * 28 bytes for unix payload, and them 1 byte for the session id.
So
Post by Baptiste- * we jump 1 + 1 + 4 + 28 + 1 bytes.
+ * 28 bytes for unix payload. So we jump 1 + 1 + 4 + 28.
*/
- msg += 1 + 1 + 4 + 28 + 1;
+ msg += 1 + 1 + 4 + 28;;
Just a minor detail, extra semi-column above. I'll fix it once merged,
don't resend for this.
Post by Baptiste+ if (msg > end)
+ return;
+
+ * if present, we have to jump by is length + 1 for the size
information
extra "is" here, same, no need to resend.
Post by Baptiste+ * if not present, we have to jump by 1 only
+ */
+ if (msg[0] > 0)
+ msg += msg[0];
+ msg += 1;
if (msg > end)
return;
Thank you,
Willy