TLS 1.3 handshake is successful even when explicitly limiting to prior TLS versions in sni_fun.

[sa-mm]

Describe the bug

With the following ssl options passed to the sni_fun callback, we were expecting that a client capable of TLS 1.3 would have to negotiate to use TLS 1.2. However, based on the "Success" output from the server and the logging from the client, it appears the handshake completes successfully with TLS 1.3.

One oddity is that even when the connection_information reported from the socket is {protocol,'tlsv1.3'}, turning on debug logging shows that the last handshake to "finish" involves TLS 1.2.

To Reproduce

Call ssl:handshake/2, passing in the ssl options outlined below.

% git clone https://github.com/michaelklishin/tls-gen.git
% make -C tls-gen/basic
% erlc cipher_issue.erl && erl -noshell -s cipher_issue start -s init stop
% openssl version
% OpenSSL 3.0.0 7 sep 2021 (Library: OpenSSL 3.0.0 7 sep 2021)
% openssl s_client -connect localhost:9999 -CAfile tls-gen/basic/result/ca_certificate.pem -servername $(hostname)
% curl --resolve $(hostname):9999:127.0.0.1 "https://$(hostname):9999" --cacert "./tls-gen/basic/result/ca_certificate.pem" -i -v

-module(cipher_issue).
-export([start/0]).

start() ->
    ssl:start(),
    Versions = [tlsv1 ,'tlsv1.1' ,'tlsv1.2'],
    Ciphers = lists:usort(lists:flatten([ssl:cipher_suites(default ,Vsn) || Vsn <- Versions])),
    CiphersAndVersions = [{versions, Versions}, {ciphers, Ciphers}],
    SslOpts = [
      {verify, verify_peer},
      {cacertfile, "./tls-gen/basic/result/ca_certificate.pem"},
      {certfile,   "./tls-gen/basic/result/server_certificate.pem"},
      {keyfile,    "./tls-gen/basic/result/server_key.pem"},
      {log_level, error}
    ],
    {ok, ListenSocket} = gen_tcp:listen(9999, [{active, true}, binary]),
    {ok, AcceptSocket} = gen_tcp:accept(ListenSocket),
    case ssl:handshake(AcceptSocket, [{sni_fun, fun (_SNI) ->
      SslOpts ++ CiphersAndVersions
    end}]) of
      {ok ,Socket} -> 
        {ok, [Protocol]} = ssl:connection_information(Socket, [protocol]),
        io:format("Success: ~p~n", [Protocol]),
        ssl:close(Socket);
      {error ,Error} -> io:format("Error: ~p~n", [Error])
    end,
    ok.
% Success: {protocol,'tlsv1.3'}

If we add CiphersAndVersions as top-level options to ssl:handshake/2, then the TLS version is enforced:

-module(cipher_issue).
-export([start/0]).

start() ->
    ssl:start(),
    Versions = [tlsv1 ,'tlsv1.1' ,'tlsv1.2'],
    Ciphers = lists:usort(lists:flatten([ssl:cipher_suites(default ,Vsn) || Vsn <- Versions])),
    CiphersAndVersions = [{versions, Versions}, {ciphers, Ciphers}],
    SslOpts = [
      {verify, verify_peer},
      {cacertfile, "./tls-gen/basic/result/ca_certificate.pem"},
      {certfile,   "./tls-gen/basic/result/server_certificate.pem"},
      {keyfile,    "./tls-gen/basic/result/server_key.pem"},
      {log_level, error}
    ],
    {ok, ListenSocket} = gen_tcp:listen(9999, [{active, true}, binary]),
    {ok, AcceptSocket} = gen_tcp:accept(ListenSocket),
    case ssl:handshake(AcceptSocket, [{sni_fun, fun (_SNI) ->
      SslOpts
    end}] ++ CiphersAndVersions) of
      {ok ,Socket} -> 
        {ok, [Protocol]} = ssl:connection_information(Socket, [protocol]),
        io:format("Success: ~p~n", [Protocol]),
        ssl:close(Socket);
      {error ,Error} -> io:format("Error: ~p~n", [Error])
    end,
    ok.
% Success: {protocol,'tlsv1.2'}

Finally, if we don't return options in the sni callback, we get a badmatch. This is a change from OTP 22.

-module(cipher_issue).
-export([start/0]).

start() ->
    ssl:start(),
    Versions = [tlsv1 ,'tlsv1.1' ,'tlsv1.2'],
    Ciphers = lists:usort(lists:flatten([ssl:cipher_suites(default ,Vsn) || Vsn <- Versions])),
    CiphersAndVersions = [{versions, Versions}, {ciphers, Ciphers}],
    SslOpts = [
      {verify, verify_peer},
      {cacertfile, "./tls-gen/basic/result/ca_certificate.pem"},
      {certfile,   "./tls-gen/basic/result/server_certificate.pem"},
      {keyfile,    "./tls-gen/basic/result/server_key.pem"},
      {log_level, error}
    ],
    {ok, ListenSocket} = gen_tcp:listen(9999, [{active, true}, binary]),
    {ok, AcceptSocket} = gen_tcp:accept(ListenSocket),
    case ssl:handshake(AcceptSocket, [{sni_fun, fun (_SNI) ->
      []
    end}]) of
      {ok ,Socket} -> 
        {ok, [Protocol]} = ssl:connection_information(Socket, [protocol]),
        io:format("Success: ~p~n", [Protocol]),
        ssl:close(Socket);
      {error ,Error} -> io:format("Error: ~p~n", [Error])
    end,
    ok.
% OTP 24 (and 23): badmatch at {tls_handshake_1_3,do_start,2,[{file,"tls_handshake_1_3.erl"},{line,652}]}
% OTP 22: Error: {tls_alert,{handshake_failure,"TLS server: In state hello at tls_handshake.erl:231 generated SERVER ALERT: Fatal - Handshake Failure\n malformed_handshake_data"}}

Expected behavior

We were expecting that explicitly limiting the TLS versions in the sni_fun would cause negotiation to one of the TLS versions specified there.

Affected versions

The following were run with Erlang/OTP 24 [erts-12.1.3] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit] and {ssl_app,"10.5.2"}.

[IngelaAndin]

Yes, I believe this is a bug or two. OTP 22 did not have TLS-1.3 support and its state machine is quite different from the previous protocol versions. We will plan to fix this in our next sprint as believe it is a bigger job. I would also want to point out that your test program has a little problem, that is if you upgrade from a TCP connection, you will want to create the listen socket in passive mode {active, false} and supply {active, true} in the handshake options, otherwise TLS data might arrive at the wrong process.

[sa-mm]

Thanks for looking into this, and it's great to hear that there's a fix planned.

I would also want to point out that your test program has a little problem, that is if you upgrade from a TCP connection, you will want to create the listen socket in passive mode {active, false} and supply {active, true} in the handshake options, otherwise TLS data might arrive at the wrong process.

Got it. Thanks! I hope I remember that in the future.