Do not crash on unknown hash and signature algorithms

[mremond]

Do not merge as is.

Hi @IngelaAndin,

Here is a workaround for a crash on SSL when trying to connect on a server for which Erlang does not yet know hash / signature algorithm.

The crash is:

** {function_clause,[{ssl_cipher,hash_algorithm,"ï",[{file,"ssl_cipher.erl"},{line,1174}]},{ssl_handshake,'-decode_handshake/3-blc$^0/1-0-',1,[{file,"ssl_handshake.erl"},{line,898}]},{ssl_handshake,'-decode_handshake/3-blc$^0/1-0-',1,[{file,"ssl_handshake.erl"},{line,899}]},{ssl_handshake,decode_handshake,3,[{file,"ssl_handshake.erl"},{line,898}]},{tls_handshake,get_tls_handshake_aux,3,[{file,"tls_handshake.erl"},{line,153}]},{tls_connection,next_state,4,[{file,"tls_connection.erl"},{line,454}]},{tls_connection,next_state,4,[{file,"tls_connection.erl"},{line,458}]},{gen_fsm,handle_msg,7,[{file,"gen_fsm.erl"},{line,505}]}]}

You can reproduce it by using any random .pem with the following command (against Apple Server, be careful):

ssl:start().
ssl:connect("gateway.sandbox.push.apple.com", 2195, [{certfile, "test_cert.pem"}, {active, true}, binary], 5000).

Our fix is certainly not the final one, as you probably have a better idea on how to integrate it to your test in Erlang code base. Instead of reporting unknown hash or signature as anon/none, it is better to report them as unknown and have them ignore at a later stage in the negotiation process.

Anyway, we thought this patch and test case could help you understand the issue.

[OTP-Maintainer]

Patch has passed first testings and has been assigned to be reviewed

---

I am a script, I am not human

---

[IngelaAndin]

I think the following patch would solve the problem, in a good way.

I am not sure why they send an invalid value instead of no value, but this way invalid values will be ignored and ssl will fallback to default values if there are no valid values in the extension.

diff --git a/lib/ssl/src/ssl_cipher.erl b/lib/ssl/src/ssl_cipher.erl
index 8584e56..fd101ef 100644
--- a/lib/ssl/src/ssl_cipher.erl
+++ b/lib/ssl/src/ssl_cipher.erl
@@ -1573,8 +1573,9 @@ hash_algorithm(?SHA) -> sha;
hash_algorithm(?SHA224) -> sha224;
hash_algorithm(?SHA256) -> sha256;
hash_algorithm(?SHA384) -> sha384;

-hash_algorithm(?SHA512) -> sha512.

+hash_algorithm(?SHA512) -> sha512;
+hash_algorithm(_) -> undefined.

• sign_algorithm(anon) -> ?ANON;
  sign_algorithm(rsa) -> ?RSA;
  sign_algorithm(dsa) -> ?DSA;
  @@ -1582,7 +1583,8 @@ sign_algorithm(ecdsa) -> ?ECDSA;
  sign_algorithm(?ANON) -> anon;
  sign_algorithm(?RSA) -> rsa;
  sign_algorithm(?DSA) -> dsa;
  -sign_algorithm(?ECDSA) -> ecdsa.
  +sign_algorithm(?ECDSA) -> ecdsa;
  +sign_algorithm(_) -> undefined.

  hash_size(null) ->
  0;
  diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl
  index 12a17cb..32da478 100644
  --- a/lib/ssl/src/ssl_handshake.erl
  +++ b/lib/ssl/src/ssl_handshake.erl
  @@ -587,7 +587,11 @@ select_hashsign(#hash_sign_algos{hash_sign_algos = HashSigns}, Cert, {Major, Min
  #'OTPCertificate'{tbsCertificate = TBSCert} =public_key:pkix_decode_cert(Cert, otp),
  #'OTPSubjectPublicKeyInfo'{algorithm = {_,Algo, }} = TBSCert#'OTPTBSCertificate'.subjectPublicKeyInfo,
  DefaultHashSign = {, Sign} = select_hashsign_algs(undefined, Algo, Version),

• case lists:filter(fun({sha, dsa}) ->

• case lists:filter(fun({_, undefined}) -> %% ignore invalid extension signature values

•             false;
  

•        ({undefined, _}) -> %% ignore invalid extension hash values
  

•             false;
  

•        ({sha, dsa}) ->
            true;
       ({_, dsa}) ->
            false;
  

[RoadRunnr]

The comment "ignore invalid extension values", should be rephrased as "ignore unknown extension values".

See also RFC5246, Sect. 7.4.1.4.1 (http://tools.ietf.org/html/rfc5246#section-7.4.1.4.1). Section 7.4.4 defines the Certificate Request, but omits the handling of unknown hash and signature values, but I think applying section 7.4.1.4.1 to it is appropriate.

[IngelaAndin]

Humm, well I take the patch back, I was a little to quick there. And the server code
should do this http://tools.ietf.org/html/rfc5246#section-7.4.1.4.1. And it should not have to be changed, it is the client that crashes ... I will investigate it a little more ...

[IngelaAndin]

Ok, the following patch should take away the crash and hopefully make it possible negotiate a valid connection (without making things unsafe),
in the short perspective. In a longer perspective we probably will want to add some kind of callbacks to let the application handle proprietary algorithms in the certificate-request and in the client hello. We will also consider adding code for ignoring valid but currently not specified codes.

diff --git a/lib/ssl/src/ssl_cipher.erl b/lib/ssl/src/ssl_cipher.erl
index 3ed53b7..0039f24 100644
--- a/lib/ssl/src/ssl_cipher.erl
+++ b/lib/ssl/src/ssl_cipher.erl
@@ -1574,8 +1574,8 @@ hash_algorithm(?SHA224) -> sha224;
hash_algorithm(?SHA256) -> sha256;
hash_algorithm(?SHA384) -> sha384;
hash_algorithm(?SHA512) -> sha512;

-hash_algorithm(Other) when is_integer(Other) -> Other.

+hash_algorithm(Other) when is_integer(Other) andalso ((Other >= 224) and (Other =< 255)) -> Other.
+
sign_algorithm(anon) -> ?ANON;
sign_algorithm(rsa) -> ?RSA;
sign_algorithm(dsa) -> ?DSA;
@@ -1584,7 +1584,7 @@ sign_algorithm(?ANON) -> anon;
sign_algorithm(?RSA) -> rsa;
sign_algorithm(?DSA) -> dsa;
sign_algorithm(?ECDSA) -> ecdsa;
-sign_algorithm(Other) when is_integer(Other) -> Other.
+sign_algorithm(Other) when is_integer(Other) andalso ((Other >= 224) and (Other =< 255)) -> Other.

hash_size(null) ->
0;

[OTP-Maintainer]

The summary line of the commit message is too long and/or ends with a "."
Make sure the whole message follows the guidelines here: https://github.com/erlang/otp/wiki/Writing-good-commit-messages.

Bad message: Revert "Do not crash on unknown hash and signature algorithms"

---

I am a script, I am not human

---

[IngelaAndin]

I will merge my own branch with this patch for 18 and I will probably have it look like this:

@ -1574,6 +1574,7 @@ hash_algorithm(?SHA224) -> sha224;
hash_algorithm(?SHA256) -> sha256;
hash_algorithm(?SHA384) -> sha384;
hash_algorithm(?SHA512) -> sha512;
+hash_algorithm(Other) when is_integer(Other) andalso ((Other >= 7) and (Other =< 223)) -> unassigned;
hash_algorithm(Other) when is_integer(Other) andalso ((Other >= 224) and (Other =< 255)) -> Other.

sign_algorithm(anon) -> ?ANON;
@@ -1584,6 +1585,7 @@ sign_algorithm(?ANON) -> anon;
sign_algorithm(?RSA) -> rsa;
sign_algorithm(?DSA) -> dsa;
sign_algorithm(?ECDSA) -> ecdsa;
+sign_algorithm(Other) when is_integer(Other) andalso ((Other >= 4) and (Other =< 223)) -> unassigned;
sign_algorithm(Other) when is_integer(Other) andalso ((Other >= 224) and (Other =< 255)) -> Other.

hash_size(null) ->

[mremond]

Thanks, @IngelaAndin !