Crash in TLS1.3 handshake caused by unassigned signature algorithm

[mworrell]

Describe the bug

Crash in TLS1.3 handshake caused by trying to encode an unassigned signature algorithm during client_hello handshake

To Reproduce

Given this HelloBin binary in tls_handshake_1_3:truncate_client_hello/1:

HelloBin0 = <<1,0,1,213,3,3,224,80,22,53,173,24,195,236,126,90,97,19,120,89,229,186,70,120,73,252,215,184,142,50,134,16,84,4,60,7,89,231,32,129,11,71,132,248,183,203,23,252,145,42,154,69,82,123,172,213,137,7,235,105,178,140,163,11,186,106,97,230,22,179,162,0,24,19,2,19,3,19,1,192,44,192,43,192,48,192,47,192,36,192,35,192,40,192,39,0,255,1,0,1,116,0,0,0,26,0,24,0,0,21,119,119,119,46,120,120,120,120,120,120,120,120,120,120,120,120,120,46,99,111,109,0,11,0,4,3,0,1,2,0,10,0,22,0,20,0,29,0,23,0,30,0,25,0,24,1,0,1,1,1,2,1,3,1,4,0,35,0,0,0,5,0,5,1,0,0,0,0,0,22,0,0,0,23,0,0,0,13,0,48,0,46,4,3,5,3,6,3,8,7,8,8,8,26,8,27,8,28,8,9,8,10,8,11,8,4,8,5,8,6,4,1,5,1,6,1,3,3,3,1,3,2,4,2,5,2,6,2,0,43,0,5,4,3,4,3,3,0,45,0,2,1,1,0,51,0,38,0,36,0,29,0,32,47,17,161,47,68,184,145,148,24,172,153,151,195,110,139,12,220,63,236,88,142,36,222,42,38,251,239,157,84,148,59,72,0,41,0,174,0,121,0,115,155,62,93,115,44,106,248,45,157,98,128,178,116,82,6,153,40,143,250,26,61,154,21,37,97,52,44,76,181,32,9,130,18,163,173,131,135,62,34,125,9,104,15,168,70,134,222,96,240,76,224,24,171,110,210,0,100,181,11,26,114,24,20,67,59,24,77,88,26,204,134,155,215,203,165,155,208,45,62,191,254,6,93,167,80,22,127,195,83,180,179,88,215,195,34,30,75,189,239,50,178,76,124,235,131,68,99,57,184,107,52,232,202,165,172,75,222,53,218,0,49,48,6,136,165,215,98,30,34,60,138,162,178,39,219,246,245,246,13,234,49,176,137,24,44,148,232,172,43,211,254,1,240,203,195,248,114,78,172,157,19,100,239,81,106,115,231,255,168,20>>.

Which is decoded with:

<<Type:8/unsigned-big-integer, _Length:24/unsigned-big-integer, Body/binary>> = HelloBin0.

And with Version {3,4} (TLS 1.3):

Version = {3,4}.

If we then call:

tls_handshake:decode_handshake(Version, Type, Body).

Then the following is returned:

{client_hello,{3,3},
              <<224,80,...>>,
              <<129,11,...>>,
              undefined,
              [<<19,2>>,...],
              [0],
              #{cookie => undefined,
                client_hello_versions => {client_hello_versions,[{3,4},{3,3}]},
                certificate_authorities => undefined,
                signature_algs =>
                    {signature_algorithms,[ecdsa_secp256r1_sha256,
                                           ecdsa_secp384r1_sha384,ecdsa_secp521r1_sha512,eddsa_ed25519,
                                           eddsa_ed448,
                                           {unassigned,unassigned},
                                           {unassigned,unassigned},
                                           {unassigned,unassigned},
                                           rsa_pss_pss_sha256,rsa_pss_pss_sha384,rsa_pss_pss_sha512,
                                           rsa_pss_rsae_sha256,rsa_pss_rsae_sha384,rsa_pss_rsae_sha512,
                                           rsa_pkcs1_sha256,rsa_pkcs1_sha384,rsa_pkcs1_sha512,{...}|...]}, 
...

Note the {unassigned, unassigned} signature algorithms.

Expected behavior

That the {unassigned,unassigned} are omitted from the signature algorithms, per the comment in ssl_handshake:decode_extensions:

 %% Ignore unknown signature algorithms

Which does drop a unassigned but not {unassigned,unassigned}, with can also be returned by ssl_cipher:signature_scheme/1

Affected versions

At least 27.1, 26,2, and 24.3.
The crash is not happening on maint/master for the given algorithms.
Though with some editing of the payload the problematic construct can easily be triggered:

https://github.com/erlang/otp/blob/master/lib/ssl/src/ssl_handshake.erl#L2929
https://github.com/erlang/otp/blob/master/lib/ssl/src/ssl_handshake.erl#L2949

And the spot where {unassigned, unassigned} can be returned:

https://github.com/erlang/otp/blob/master/lib/ssl/src/ssl_cipher.erl#L639

[mworrell]

A fix for this problem has been added:

#9486

[mworrell]

Whilst looking at the code I was wondering why these two clauses are the same:

https://github.com/mworrell/otp/blob/d24c732d77a64e3e2e25076a7d61690b56d02017/lib/ssl/src/ssl_handshake.erl#L2913

decode_extensions(<<?UINT16(?SIGNATURE_ALGORITHMS_EXT), ?UINT16(Len),
		       ExtData:Len/binary, Rest/binary>>, ?TLS_1_3=Version, MessageType, Acc) ->

https://github.com/mworrell/otp/blob/d24c732d77a64e3e2e25076a7d61690b56d02017/lib/ssl/src/ssl_handshake.erl#L2923

decode_extensions(<<?UINT16(?SIGNATURE_ALGORITHMS_EXT), ?UINT16(Len),
		       ExtData:Len/binary, Rest/binary>>, ?TLS_1_3=Version, MessageType, Acc) ->

I assume the second clause can be removed?

[IngelaAndin]

@mworrell Yes the second clause will never happen, I will clean it up on the OTP-27 track as we have some refactoring activities planed anyway. Thanks for noticing.

[mworrell]

After the last changes it was a lot easier to see that the clauses are the same. Thank you for all your work!