From 8c419a6edecc86dc4c682d040c4bb3e3506c7876 Mon Sep 17 00:00:00 2001 From: Alexey Lebedeff Date: Thu, 19 May 2016 15:11:37 +0300 Subject: Improve SSL diagnostics There are a lot of cases where `ssl` application just returns unhelpful `handshake failure` or `internal error`. This patch tries to provide better diagnostics so operator can debug his SSL misconfiguration without doing hardcore erlang debugging. Here is an example escript that incorrectly uses server certificate as a client one: https://gist.github.com/binarin/35c34c2df7556bf04c8a878682ef3d67 With the patch it is properly reported as an error in "extended key usage". --- lib/ssl/src/ssl_alert.erl | 14 ++++++--- lib/ssl/src/ssl_alert.hrl | 4 ++- lib/ssl/src/ssl_cipher.erl | 8 ++--- lib/ssl/src/ssl_connection.erl | 4 +-- lib/ssl/src/ssl_handshake.erl | 68 +++++++++++++++++++++------------------- lib/ssl/src/tls_handshake.erl | 2 +- lib/ssl/test/ssl_basic_SUITE.erl | 29 +++++++++++++++++ 7 files changed, 84 insertions(+), 45 deletions(-) diff --git a/lib/ssl/src/ssl_alert.erl b/lib/ssl/src/ssl_alert.erl index 3e35e24527..db71b16d80 100644 --- a/lib/ssl/src/ssl_alert.erl +++ b/lib/ssl/src/ssl_alert.erl @@ -73,10 +73,14 @@ reason_code(#alert{description = Description}, _) -> %% %% Description: Returns the error string for given alert. %%-------------------------------------------------------------------- - -alert_txt(#alert{level = Level, description = Description, where = {Mod,Line}}) -> +alert_txt(#alert{level = Level, description = Description, where = {Mod,Line}, reason = undefined}) -> Mod ++ ":" ++ integer_to_list(Line) ++ ":" ++ - level_txt(Level) ++" "++ description_txt(Description). + level_txt(Level) ++" "++ description_txt(Description); +alert_txt(#alert{reason = Reason} = Alert) -> + BaseTxt = alert_txt(Alert#alert{reason = undefined}), + FormatDepth = 9, % Some limit on printed representation of an error + ReasonTxt = lists:flatten(io_lib:format("~P", [Reason, FormatDepth])), + BaseTxt ++ " - " ++ ReasonTxt. %%-------------------------------------------------------------------- %%% Internal functions @@ -85,7 +89,7 @@ alert_txt(#alert{level = Level, description = Description, where = {Mod,Line}}) %% It is very unlikely that an correct implementation will send more than one alert at the time %% So it there is more than 10 warning alerts we consider it an error decode(<>, _, N) when Level == ?WARNING, N > ?MAX_ALERTS -> - ?ALERT_REC(?FATAL, ?DECODE_ERROR); + ?ALERT_REC(?FATAL, ?DECODE_ERROR, too_many_remote_alerts); decode(<>, Acc, N) when Level == ?WARNING -> Alert = ?ALERT_REC(Level, Description), decode(Rest, [Alert | Acc], N + 1); @@ -93,7 +97,7 @@ decode(<>, Acc, _) when Level == Alert = ?ALERT_REC(Level, Description), lists:reverse([Alert | Acc]); %% No need to decode rest fatal alert will end the connection decode(<>, _, _) -> - ?ALERT_REC(?FATAL, ?ILLEGAL_PARAMETER); + ?ALERT_REC(?FATAL, ?ILLEGAL_PARAMETER, failed_to_decode_remote_alert); decode(<<>>, Acc, _) -> lists:reverse(Acc, []). diff --git a/lib/ssl/src/ssl_alert.hrl b/lib/ssl/src/ssl_alert.hrl index 8c4bd08d31..38facb964f 100644 --- a/lib/ssl/src/ssl_alert.hrl +++ b/lib/ssl/src/ssl_alert.hrl @@ -109,6 +109,7 @@ -define(NO_APPLICATION_PROTOCOL, 120). -define(ALERT_REC(Level,Desc), #alert{level=Level,description=Desc,where={?FILE, ?LINE}}). +-define(ALERT_REC(Level,Desc,Reason), #alert{level=Level,description=Desc,where={?FILE, ?LINE},reason=Reason}). -define(MAX_ALERTS, 10). @@ -116,6 +117,7 @@ -record(alert, { level, description, - where = {?FILE, ?LINE} + where = {?FILE, ?LINE}, + reason }). -endif. % -ifdef(ssl_alert). diff --git a/lib/ssl/src/ssl_cipher.erl b/lib/ssl/src/ssl_cipher.erl index dc0a0c2cc4..e935c033c7 100644 --- a/lib/ssl/src/ssl_cipher.erl +++ b/lib/ssl/src/ssl_cipher.erl @@ -214,7 +214,7 @@ decipher(?RC4, HashSz, CipherState = #cipher_state{state = State0}, Fragment, _, %% alerts may permit certain attacks against CBC mode as used in %% TLS [CBCATT]. It is preferable to uniformly use the %% bad_record_mac alert to hide the specific type of the error." - ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC) + ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC, decryption_failed) end; decipher(?DES, HashSz, CipherState, Fragment, Version, PaddingCheck) -> @@ -272,7 +272,7 @@ block_decipher(Fun, #cipher_state{key=Key, iv=IV} = CipherState0, %% alerts may permit certain attacks against CBC mode as used in %% TLS [CBCATT]. It is preferable to uniformly use the %% bad_record_mac alert to hide the specific type of the error." - ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC) + ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC, decryption_failed) end. aead_ciphertext_to_state(chacha20_poly1305, SeqNo, _IV, AAD0, Fragment, _Version) -> @@ -296,11 +296,11 @@ aead_decipher(Type, #cipher_state{key = Key, iv = IV} = CipherState, Content when is_binary(Content) -> {Content, CipherState}; _ -> - ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC) + ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC, decryption_failed) end catch _:_ -> - ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC) + ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC, decryption_failed) end. %%-------------------------------------------------------------------- diff --git a/lib/ssl/src/ssl_connection.erl b/lib/ssl/src/ssl_connection.erl index 089b3615c6..22d107ff9c 100644 --- a/lib/ssl/src/ssl_connection.erl +++ b/lib/ssl/src/ssl_connection.erl @@ -1488,7 +1488,7 @@ rsa_key_exchange(Version, PremasterSecret, PublicKeyInfo = {Algorithm, _, _}) {premaster_secret, PremasterSecret, PublicKeyInfo}); rsa_key_exchange(_, _, _) -> - throw (?ALERT_REC(?FATAL,?HANDSHAKE_FAILURE)). + throw (?ALERT_REC(?FATAL,?HANDSHAKE_FAILURE, pub_key_is_not_rsa)). rsa_psk_key_exchange(Version, PskIdentity, PremasterSecret, PublicKeyInfo = {Algorithm, _, _}) @@ -1505,7 +1505,7 @@ rsa_psk_key_exchange(Version, PskIdentity, PremasterSecret, {psk_premaster_secret, PskIdentity, PremasterSecret, PublicKeyInfo}); rsa_psk_key_exchange(_, _, _, _) -> - throw (?ALERT_REC(?FATAL,?HANDSHAKE_FAILURE)). + throw (?ALERT_REC(?FATAL,?HANDSHAKE_FAILURE, pub_key_is_not_rsa)). request_client_cert(#state{ssl_options = #ssl_options{verify = verify_peer, signature_algs = SupportedHashSigns}, diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl index 598d4e4112..6f0cfb5cc1 100644 --- a/lib/ssl/src/ssl_handshake.erl +++ b/lib/ssl/src/ssl_handshake.erl @@ -167,7 +167,7 @@ certificate(OwnCert, CertDbHandle, CertDbRef, server) -> {ok, _, Chain} -> #certificate{asn1_certificates = Chain}; {error, _} -> - ?ALERT_REC(?FATAL, ?INTERNAL_ERROR) + ?ALERT_REC(?FATAL, ?INTERNAL_ERROR, server_has_no_suitable_certificates) end. %%-------------------------------------------------------------------- @@ -195,7 +195,7 @@ client_certificate_verify(OwnCert, MasterSecret, Version, PrivateKey, {Handshake, _}) -> case public_key:pkix_is_fixed_dh_cert(OwnCert) of true -> - ?ALERT_REC(?FATAL, ?UNSUPPORTED_CERTIFICATE); + ?ALERT_REC(?FATAL, ?UNSUPPORTED_CERTIFICATE, fixed_diffie_hellman_prohibited); false -> Hashes = calc_certificate_verify(Version, HashAlgo, MasterSecret, Handshake), @@ -353,7 +353,7 @@ verify_server_key(#server_key_params{params_bin = EncParams, %% Description: Checks that the certificate_verify message is valid. %%-------------------------------------------------------------------- certificate_verify(_, _, _, undefined, _, _) -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, invalid_certificate_verify_message); certificate_verify(Signature, PublicKeyInfo, Version, HashSign = {HashAlgo, _}, MasterSecret, {_, Handshake}) -> @@ -417,7 +417,7 @@ certify(#certificate{asn1_certificates = ASN1Certs}, CertDbHandle, CertDbRef, catch error:_ -> %% ASN-1 decode of certificate somehow failed - ?ALERT_REC(?FATAL, ?CERTIFICATE_UNKNOWN) + ?ALERT_REC(?FATAL, ?CERTIFICATE_UNKNOWN, failed_to_decode_certificate) end. %%-------------------------------------------------------------------- @@ -605,7 +605,7 @@ select_hashsign(#hash_sign_algos{hash_sign_algos = HashSigns}, Cert, KeyExAlgo, false end, HashSigns) of [] -> - ?ALERT_REC(?FATAL, ?INSUFFICIENT_SECURITY); + ?ALERT_REC(?FATAL, ?INSUFFICIENT_SECURITY, no_suitable_signature_algorithm); [HashSign | _] -> HashSign end; @@ -668,7 +668,7 @@ master_secret(RecordCB, Version, #session{master_secret = Mastersecret}, Report = io_lib:format("Key calculation failed due to ~p", [Reason]), error_logger:error_report(Report), - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE) + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, key_calculation_failure) end; master_secret(RecordCB, Version, PremasterSecret, ConnectionStates, Role) -> @@ -687,7 +687,7 @@ master_secret(RecordCB, Version, PremasterSecret, ConnectionStates, Role) -> Report = io_lib:format("Master secret calculation failed" " due to ~p", [Reason]), error_logger:error_report(Report), - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE) + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, master_secret_calculation_failure) end. %%-------------Encode/Decode -------------------------------- @@ -958,8 +958,8 @@ decode_handshake(_Version, ?CLIENT_KEY_EXCHANGE, PKEPMS) -> #client_key_exchange{exchange_keys = PKEPMS}; decode_handshake(_Version, ?FINISHED, VerifyData) -> #finished{verify_data = VerifyData}; -decode_handshake(_, _, _) -> - throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE)). +decode_handshake(_, Message, _) -> + throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, {unknown_or_malformed_handshake, Message})). %%-------------------------------------------------------------------- -spec decode_hello_extensions({client, binary()} | binary()) -> #hello_extensions{}. @@ -1031,8 +1031,8 @@ dec_server_key(< - throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE)). +dec_server_key(_, KeyExchange, _) -> + throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, {unknown_or_malformed_key_exchange, KeyExchange})). %%-------------------------------------------------------------------- -spec decode_suites('2_bytes'|'3_bytes', binary()) -> list(). @@ -1253,8 +1253,12 @@ handle_server_hello_extensions(RecordCB, Random, CipherSuite, Compression, Protocol -> {ConnectionStates, npn, Protocol} end; - _ -> %% {error, _Reason} or a list of 0/2+ protocols. - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE) + {error, Reason} -> + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, Reason); + [] -> + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, no_protocols_in_server_hello); + [_|_] -> + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, too_many_protocols_in_server_hello) end. select_version(RecordCB, ClientVersion, Versions) -> @@ -1316,14 +1320,14 @@ handle_renegotiation_info(_RecordCB, client, #renegotiation_info{renegotiated_co true -> {ok, ConnectionStates}; false -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE) + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, client_renegotiation) end; handle_renegotiation_info(_RecordCB, server, #renegotiation_info{renegotiated_connection = ClientVerify}, ConnectionStates, true, _, CipherSuites) -> case is_member(?TLS_EMPTY_RENEGOTIATION_INFO_SCSV, CipherSuites) of true -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, {server_renegotiation, empty_renegotiation_info_scsv}); false -> CS = ssl_record:current_connection_state(ConnectionStates, read), Data = CS#connection_state.client_verify_data, @@ -1331,7 +1335,7 @@ handle_renegotiation_info(_RecordCB, server, #renegotiation_info{renegotiated_co true -> {ok, ConnectionStates}; false -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE) + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, server_renegotiation) end end; @@ -1341,7 +1345,7 @@ handle_renegotiation_info(RecordCB, client, undefined, ConnectionStates, true, S handle_renegotiation_info(RecordCB, server, undefined, ConnectionStates, true, SecureRenegotation, CipherSuites) -> case is_member(?TLS_EMPTY_RENEGOTIATION_INFO_SCSV, CipherSuites) of true -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, {server_renegotiation, empty_renegotiation_info_scsv}); false -> handle_renegotiation_info(RecordCB, ConnectionStates, SecureRenegotation) end. @@ -1350,7 +1354,7 @@ handle_renegotiation_info(_RecordCB, ConnectionStates, SecureRenegotation) -> CS = ssl_record:current_connection_state(ConnectionStates, read), case {SecureRenegotation, CS#connection_state.secure_renegotiation} of {_, true} -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, already_secure); {true, false} -> ?ALERT_REC(?FATAL, ?NO_RENEGOTIATION); {false, false} -> @@ -1523,8 +1527,8 @@ path_validation_alert({bad_cert, selfsigned_peer}) -> ?ALERT_REC(?FATAL, ?BAD_CERTIFICATE); path_validation_alert({bad_cert, unknown_ca}) -> ?ALERT_REC(?FATAL, ?UNKNOWN_CA); -path_validation_alert(_) -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE). +path_validation_alert(Reason) -> + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, Reason). encrypted_premaster_secret(Secret, RSAPublicKey) -> try @@ -1533,8 +1537,8 @@ encrypted_premaster_secret(Secret, RSAPublicKey) -> rsa_pkcs1_padding}]), #encrypted_premaster_secret{premaster_secret = PreMasterSecret} catch - _:_-> - throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE)) + _:_-> + throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, premaster_encryption_failed)) end. digitally_signed({3, Minor}, Hash, HashAlgo, Key) when Minor >= 3 -> @@ -1751,12 +1755,12 @@ dec_client_key(PKEPMS, ?KEY_EXCHANGE_RSA, {3, 0}) -> dec_client_key(<>, ?KEY_EXCHANGE_RSA, _) -> #encrypted_premaster_secret{premaster_secret = PKEPMS}; dec_client_key(<<>>, ?KEY_EXCHANGE_DIFFIE_HELLMAN, _) -> - throw(?ALERT_REC(?FATAL, ?UNSUPPORTED_CERTIFICATE)); + throw(?ALERT_REC(?FATAL, ?UNSUPPORTED_CERTIFICATE, empty_dh_public)); dec_client_key(<>, ?KEY_EXCHANGE_DIFFIE_HELLMAN, _) -> #client_diffie_hellman_public{dh_public = DH_Y}; dec_client_key(<<>>, ?KEY_EXCHANGE_EC_DIFFIE_HELLMAN, _) -> - throw(?ALERT_REC(?FATAL, ?UNSUPPORTED_CERTIFICATE)); + throw(?ALERT_REC(?FATAL, ?UNSUPPORTED_CERTIFICATE, empty_dh_public)); dec_client_key(<>, ?KEY_EXCHANGE_EC_DIFFIE_HELLMAN, _) -> #client_ec_diffie_hellman_public{dh_public = DH_Y}; @@ -1800,7 +1804,7 @@ dec_server_key_signature(Params, <>, _) -> dec_server_key_signature(Params, <>, _) -> {Params, undefined, Signature}; dec_server_key_signature(_, _, _) -> - throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE)). + throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, failed_to_decrypt_server_key_sign)). dec_hello_extensions(<<>>, Acc) -> Acc; @@ -1955,8 +1959,8 @@ key_exchange_alg(_) -> %%-------------Extension handling -------------------------------- %% Receive protocols, choose one from the list, return it. -handle_alpn_extension(_, {error, _Reason}) -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); +handle_alpn_extension(_, {error, Reason}) -> + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, Reason); handle_alpn_extension([], _) -> ?ALERT_REC(?FATAL, ?NO_APPLICATION_PROTOCOL); handle_alpn_extension([ServerProtocol|Tail], ClientProtocols) -> @@ -1976,7 +1980,7 @@ handle_next_protocol(#next_protocol_negotiation{} = NextProtocols, true -> select_next_protocol(decode_next_protocols(NextProtocols), NextProtocolSelector); false -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE) % unexpected next protocol extension + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, unexpected_next_protocol_extension) end. @@ -1996,17 +2000,17 @@ handle_next_protocol_on_server(#next_protocol_negotiation{extension_data = <<>>} Protocols; handle_next_protocol_on_server(_Hello, _Renegotiation, _SSLOpts) -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE). % unexpected next protocol extension + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, unexpected_next_protocol_extension). next_protocol_extension_allowed(NextProtocolSelector, Renegotiating) -> NextProtocolSelector =/= undefined andalso not Renegotiating. -select_next_protocol({error, _Reason}, _NextProtocolSelector) -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); +select_next_protocol({error, Reason}, _NextProtocolSelector) -> + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, Reason); select_next_protocol(Protocols, NextProtocolSelector) -> case NextProtocolSelector(Protocols) of ?NO_PROTOCOL -> - ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE); + ?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, no_next_protocol); Protocol when is_binary(Protocol) -> Protocol end. diff --git a/lib/ssl/src/tls_handshake.erl b/lib/ssl/src/tls_handshake.erl index f34eebb0e4..871eb970eb 100644 --- a/lib/ssl/src/tls_handshake.erl +++ b/lib/ssl/src/tls_handshake.erl @@ -167,7 +167,7 @@ handle_client_hello(Version, #client_hello{session_id = SugesstedId, SslOpts, Cache, CacheCb, Cert), case CipherSuite of no_suite -> - ?ALERT_REC(?FATAL, ?INSUFFICIENT_SECURITY); + ?ALERT_REC(?FATAL, ?INSUFFICIENT_SECURITY, no_suitable_ciphers); _ -> {KeyExAlg,_,_,_} = ssl_cipher:suite_definition(CipherSuite), case ssl_handshake:select_hashsign(ClientHashSigns, Cert, KeyExAlg, SupportedHashSigns, Version) of diff --git a/lib/ssl/test/ssl_basic_SUITE.erl b/lib/ssl/test/ssl_basic_SUITE.erl index aa45d55406..686cdc569d 100644 --- a/lib/ssl/test/ssl_basic_SUITE.erl +++ b/lib/ssl/test/ssl_basic_SUITE.erl @@ -96,6 +96,8 @@ basic_tests() -> [app, appup, alerts, + alert_details, + alert_details_not_too_big, version_option, connect_twice, connect_dist, @@ -477,6 +479,33 @@ alerts(Config) when is_list(Config) -> end end, Alerts). %%-------------------------------------------------------------------- +alert_details() -> + [{doc, "Test that ssl_alert:alert_txt/1 result contains extendend error description"}]. +alert_details(Config) when is_list(Config) -> + Unique = make_ref(), + UniqueStr = lists:flatten(io_lib:format("~w", [Unique])), + Alert = ?ALERT_REC(?WARNING, ?CLOSE_NOTIFY, Unique), + case string:str(ssl_alert:alert_txt(Alert), UniqueStr) of + 0 -> + ct:fail(error_details_missing); + _ -> + ok + end. + +%%-------------------------------------------------------------------- +alert_details_not_too_big() -> + [{doc, "Test that ssl_alert:alert_txt/1 limits printed depth of extended error description"}]. +alert_details_not_too_big(Config) when is_list(Config) -> + Reason = lists:duplicate(10, lists:duplicate(10, lists:duplicate(10, {some, data}))), + Alert = ?ALERT_REC(?WARNING, ?CLOSE_NOTIFY, Reason), + case length(ssl_alert:alert_txt(Alert)) < 1000 of + true -> + ok; + false -> + ct:fail(ssl_alert_text_too_big) + end. + +%%-------------------------------------------------------------------- new_options_in_accept() -> [{doc,"Test that you can set ssl options in ssl_accept/3 and not only in tcp upgrade"}]. new_options_in_accept(Config) when is_list(Config) -> -- cgit v1.2.3