From ba25ad268065f48ad7647464c8f4c237d07d18b1 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Wed, 25 Apr 2018 08:51:00 +0200 Subject: Move check ip to before SSL handshake --- lib/ssl/src/inet_tls_dist.erl | 140 +++++++++++++++++++--------------- lib/ssl/test/ssl_dist_bench_SUITE.erl | 7 +- 2 files changed, 81 insertions(+), 66 deletions(-) (limited to 'lib') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index f43a7b03e6..b084135d1a 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -222,46 +222,58 @@ gen_accept(Driver, Listen) -> accept_loop(Driver, Listen, Kernel) -> case Driver:accept(Listen) of {ok, Socket} -> - {Opts,CertNodesFun} = - setup_verify_client( - Driver, Socket, get_ssl_options(server)), - wait_for_code_server(), - case - ssl:ssl_accept( - Socket, - trace([{active, false},{packet, 4}|Opts]), - net_kernel:connecttime()) - of - {ok, #sslsocket{pid = DistCtrl} = SslSocket} -> - trace( - Kernel ! - {accept, self(), DistCtrl, - Driver:family(), tls}), - receive - {Kernel, controller, Pid} -> - ok = ssl:controlling_process(SslSocket, Pid), - trace( - Pid ! {self(), controller, CertNodesFun}); - {Kernel, unsupported_protocol} -> - exit(trace(unsupported_protocol)) - end, - accept_loop(Driver, Listen, Kernel); - {error, {options, _}} = Error -> - %% Bad options: that's probably our fault. - %% Let's log that. + case check_ip(Driver, Socket) of + true -> + accept_loop(Driver, Listen, Kernel, Socket); + {false,IP} -> error_logger:error_msg( - "Cannot accept TLS distribution connection: ~s~n", - [ssl:format_error(Error)]), - gen_tcp:close(Socket), - exit(trace(Error)); - Other -> - gen_tcp:close(Socket), - exit(trace(Other)) + "** Connection attempt from " + "disallowed IP ~w ** ~n", [IP]), + ?shutdown2(no_node, trace({disallowed, IP})) end; Error -> exit(trace(Error)) end. +accept_loop(Driver, Listen, Kernel, Socket) -> + {Opts,CertNodesFun} = + setup_verify_client( + Driver, Socket, get_ssl_options(server)), + wait_for_code_server(), + case + ssl:ssl_accept( + Socket, + trace([{active, false},{packet, 4}|Opts]), + net_kernel:connecttime()) + of + {ok, #sslsocket{pid = DistCtrl} = SslSocket} -> + trace( + Kernel ! + {accept, self(), DistCtrl, + Driver:family(), tls}), + receive + {Kernel, controller, Pid} -> + ok = ssl:controlling_process(SslSocket, Pid), + trace( + Pid ! {self(), controller, CertNodesFun}); + {Kernel, unsupported_protocol} -> + exit(trace(unsupported_protocol)) + end, + accept_loop(Driver, Listen, Kernel); + {error, {options, _}} = Error -> + %% Bad options: that's probably our fault. + %% Let's log that. + error_logger:error_msg( + "Cannot accept TLS distribution connection: ~s~n", + [ssl:format_error(Error)]), + gen_tcp:close(Socket), + exit(trace(Error)); + Other -> + gen_tcp:close(Socket), + exit(trace(Other)) + end. + + %% {verify_fun,{fun ?MODULE:verify_client/3,_}} is used %% as a configuration marker that verify_client/3 shall be used. %% @@ -326,6 +338,8 @@ verify_client(PeerCert, valid_peer, {CertNodesFun,Driver,PeerIP} = S) -> [] -> {fail,cert_missing_node_name}; CertNodes -> + %% Check if the IP address of one of the nodes + %% in the peer certificate has the peer's IP address case filter_nodes_by_ip(CertNodes, Driver, PeerIP) of [] -> {fail,cert_no_host_with_peer_ip}; @@ -334,6 +348,8 @@ verify_client(PeerCert, valid_peer, {CertNodesFun,Driver,PeerIP} = S) -> end end. +%% Filter out the nodes that has got the given IP address +%% filter_nodes_by_ip(Nodes, Driver, IP) -> [Node || Node <- Nodes, @@ -405,30 +421,25 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> receive {AcceptPid, controller, CertNodesFun} -> Timer = dist_util:start_timer(SetupTime), - case check_ip(Driver, SslSocket) of - true -> - NewAllowed = - allowed_nodes( - Driver, CertNodesFun, SslSocket, Allowed), - HSData0 = hs_data_common(SslSocket), - HSData = - HSData0#hs_data{ - kernel_pid = Kernel, - this_node = MyNode, - socket = DistCtrl, - timer = Timer, - this_flags = 0, - allowed = NewAllowed}, - link(DistCtrl), - dist_util:handshake_other_started(trace(HSData)); - {false,IP} -> - error_logger:error_msg( - "** Connection attempt from " - "disallowed IP ~w ** ~n", [IP]), - ?shutdown2(no_node, trace({disallowed, IP})) - end + NewAllowed = + allowed_nodes( + Driver, CertNodesFun, SslSocket, Allowed), + HSData0 = hs_data_common(SslSocket), + HSData = + HSData0#hs_data{ + kernel_pid = Kernel, + this_node = MyNode, + socket = DistCtrl, + timer = Timer, + this_flags = 0, + allowed = NewAllowed}, + link(DistCtrl), + dist_util:handshake_other_started(trace(HSData)) end. +%% Return a list of allowed nodes according to +%% the given Allowed list that matches the peer certificate +%% allowed_nodes(_Driver, undefined, _SslSocket, Allowed) -> Allowed; allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> @@ -444,6 +455,10 @@ allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> [] -> ?shutdown(cert_missing_node_name); CertNodes -> + %% Filter out all nodes in the + %% allowed list that is in peer + %% certificate and that has got + %% the same IP address as the peer allowed( filter_nodes_by_ip( CertNodes, Driver, PeerIP), @@ -457,7 +472,8 @@ allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> end. allowed(CertNodes, []) -> - %% All allowed -> allow only certificate nodes + %% Empty allowed list means all allowed + %% -> allow only certificate nodes CertNodes; allowed(CertNodes, Allowed) -> %% Find the intersection of the allowed list and certificate nodes @@ -602,6 +618,8 @@ verify_server(PeerCert, valid_peer, {CertNodesFun,Node} = S) -> [] -> {fail,cert_missing_node_name}; CertNodes -> + %% Check that the node we are connecting to + %% is in the peer certificate case dist_util:is_allowed(Node, CertNodes) of true -> {valid,S}; @@ -615,15 +633,15 @@ verify_server(PeerCert, valid_peer, {CertNodesFun,Node} = S) -> %% Do only accept new connection attempts from nodes at our %% own LAN, if the check_ip environment parameter is true. %% ------------------------------------------------------------ -check_ip(Driver, SslSocket) -> +check_ip(Driver, Socket) -> case application:get_env(check_ip) of {ok, true} -> - case get_ifs(SslSocket) of + case get_ifs(Socket) of {ok, IFs, IP} -> check_ip(Driver, IFs, IP); Other -> ?shutdown2( - no_node, trace({check_ip_failed, SslSocket, Other})) + no_node, trace({check_ip_failed, Socket, Other})) end; _ -> true @@ -637,7 +655,7 @@ check_ip(Driver, [{OwnIP, _, Netmask}|IFs], PeerIP) -> check_ip(_Driver, [], PeerIP) -> {false, PeerIP}. -get_ifs(#sslsocket{fd = {gen_tcp, Socket, _}}) -> +get_ifs(Socket) -> case inet:peername(Socket) of {ok, {IP, _}} -> %% XXX this is seriously broken for IPv6 diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 77c42de3f2..4ffc486fc4 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -246,7 +246,7 @@ setup(Config) -> run_nodepair_test(fun setup/5, Config). setup(A, B, Prefix, HA, HB) -> - Rounds = 10, + Rounds = 50, [] = ssl_apply(HA, erlang, nodes, []), [] = ssl_apply(HB, erlang, nodes, []), {SetupTime, CycleTime} = @@ -276,10 +276,7 @@ setup_loop(A, B, T, N) -> receive {'DOWN',Mref,process,_,_} -> [] = erlang:nodes(), - receive - after 500 -> - setup_loop(A, B, Time + T, N - 1) - end + setup_loop(A, B, Time + T, N - 1) end. -- cgit v1.2.3