From aaff5f36b836c65a72fb38a27e31a88d199a3155 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Wed, 25 Mar 2015 06:27:05 +0100 Subject: Add guard to reject {spawn_opt, false} as transport/service_opt() It was possible to configure the option, but doing so caused the service to fail when starting a watchdog process: {function_clause, [{diameter_service,'-spawn_opts/1-lc$^0/1-0-', [false], [{file,"base/diameter_service.erl"},{line,846}]}, {diameter_service,start,5, [{file,"base/diameter_service.erl"},{line,820}]}, {diameter_service,start,3, [{file,"base/diameter_service.erl"},{line,782}]}, {diameter_service,handle_call,3, [{file,"base/diameter_service.erl"},{line,385}]}, {gen_server,try_handle_call,4,[{file,"gen_server.erl"},{line,607}]}, {gen_server,handle_msg,5,[{file,"gen_server.erl"},{line,639}]}, {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,237}]}]} Tests for the option in the config suite were also missing. Bungled in commit 78b3dc6. --- lib/diameter/src/base/diameter_config.erl | 6 +++++- lib/diameter/test/diameter_config_SUITE.erl | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl index 0d0304bf33..edfb91dbf4 100644 --- a/lib/diameter/src/base/diameter_config.erl +++ b/lib/diameter/src/base/diameter_config.erl @@ -675,7 +675,11 @@ opt(spawn_opt, L) L; opt(K, false = B) - when K /= sequence -> + when K == share_peers; + K == use_shared_peers; + K == monitor; + K == restrict_connections; + K == string_decode -> B; opt(K, true = B) diff --git a/lib/diameter/test/diameter_config_SUITE.erl b/lib/diameter/test/diameter_config_SUITE.erl index 77f7aace1b..ea77aa3716 100644 --- a/lib/diameter/test/diameter_config_SUITE.erl +++ b/lib/diameter/test/diameter_config_SUITE.erl @@ -85,6 +85,9 @@ {string_decode, [[true], [false]], [[0], [x]]}, + {spawn_opt, + [[[]], [[monitor, link]]], + [[false]]}, {invalid_option, %% invalid service options are rejected [], [[x], @@ -186,6 +189,9 @@ {private, [[x]], []}, + {spawn_opt, + [[[]], [[monitor, link]]], + [[false]]}, {invalid_option, %% invalid transport options are silently ignored [[x], [x,x]], -- cgit v1.2.3 From 545ff7783cebddc2ca5b2af67a6f13b1a01a4d03 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Wed, 25 Mar 2015 07:21:46 +0100 Subject: Add service_opt() incoming_maxlen To bound the length of incoming messages that will be decoded. A message longer than the specified number of bytes is discarded. An incoming_maxlen_exceeded counter is incremented to make note of the occurrence. The motivation is to prevent a sufficiently malicious peer from generating significant load by sending long messages with many AVPs for diameter to decode. The 24-bit message length header accomodates (16#FFFFFF - 20) div 12 = 1398099 Unsigned32 AVPs for example, which the current record-valued decode is too slow with in practice. A bound of 16#FFFF bytes allows for 5461 small AVPs, which is probably more than enough for the majority of applications, but the default is the full 16#FFFFFF. --- lib/diameter/doc/src/diameter.xml | 21 +++++++++++++++++++++ lib/diameter/src/base/diameter.erl | 5 +++++ lib/diameter/src/base/diameter_config.erl | 8 +++++++- lib/diameter/src/base/diameter_peer_fsm.erl | 18 +++++++++++++++--- lib/diameter/src/base/diameter_service.erl | 6 ++++-- lib/diameter/src/base/diameter_traffic.erl | 15 +++++++++++++-- lib/diameter/test/diameter_config_SUITE.erl | 3 +++ lib/diameter/test/diameter_traffic_SUITE.erl | 9 +++++++++ 8 files changed, 77 insertions(+), 8 deletions(-) diff --git a/lib/diameter/doc/src/diameter.xml b/lib/diameter/doc/src/diameter.xml index 37e67d8630..6e41b01c44 100644 --- a/lib/diameter/doc/src/diameter.xml +++ b/lib/diameter/doc/src/diameter.xml @@ -783,6 +783,27 @@ be matched by corresponding &capability; configuration, of + +{incoming_maxlen, 0..16777215} + +

+Bound on the expected size of incoming Diameter messages. +Messages larger than the specified number of bytes are discarded.

+ +

+Defaults to 16777215, the maximum value of the 24-bit Message +Length field in a Diameter Header.

+ + +

+This option should be set to as low a value as is sufficient for the +Diameter applications and peers in question, since decoding incoming +messages from a malicious peer can otherwise generate significant +load.

+ + + + {restrict_connections, false | node | nodes diff --git a/lib/diameter/src/base/diameter.erl b/lib/diameter/src/base/diameter.erl index 67dfc7bdbf..010f977b97 100644 --- a/lib/diameter/src/base/diameter.erl +++ b/lib/diameter/src/base/diameter.erl @@ -45,6 +45,7 @@ -export_type([evaluable/0, restriction/0, + message_length/0, remotes/0, sequence/0, app_alias/0, @@ -298,6 +299,9 @@ call(SvcName, App, Message) -> | [node()] | evaluable(). +-type message_length() + :: 0..16#FFFFFF. + %% Options passed to start_service/2 -type service_opt() @@ -307,6 +311,7 @@ call(SvcName, App, Message) -> | {sequence, sequence() | evaluable()} | {share_peers, remotes()} | {string_decode, boolean()} + | {incoming_maxlen, message_length()} | {use_shared_peers, remotes()} | {spawn_opt, list()}. diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl index edfb91dbf4..8ac3b9d6ca 100644 --- a/lib/diameter/src/base/diameter_config.erl +++ b/lib/diameter/src/base/diameter_config.erl @@ -159,7 +159,8 @@ stop_service(SvcName) -> %% # add_transport/2 %% -------------------------------------------------------------------------- --spec add_transport(diameter:service_name(), {connect|listen, [diameter:transport_opt()]}) +-spec add_transport(diameter:service_name(), + {connect|listen, [diameter:transport_opt()]}) -> {ok, diameter:transport_ref()} | {error, term()}. @@ -645,6 +646,7 @@ make_config(SvcName, Opts) -> {false, monitor}, {?NOMASK, sequence}, {nodes, restrict_connections}, + {16#FFFFFF, incoming_maxlen}, {true, string_decode}, {[], spawn_opt}]), @@ -670,6 +672,10 @@ make_opts(Opts, Defs) -> [{K, opt(K,V)} || {K,V} <- Known]. +opt(incoming_maxlen, N) + when 0 =< N, N < 1 bsl 24 -> + N; + opt(spawn_opt, L) when is_list(L) -> L; diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index aac2685514..2255d0a76b 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -125,7 +125,8 @@ %% outgoing DPR; boolean says whether or not %% the request was sent explicitly with %% diameter:call/4. - length_errors :: exit | handle | discard}). + length_errors :: exit | handle | discard, + incoming_maxlen :: integer() | infinity}). %% There are non-3588 states possible as a consequence of 5.6.1 of the %% standard and the corresponding problem for incoming CEA's: we don't @@ -203,6 +204,7 @@ i({Ack, WPid, {M, Ref} = T, Opts, {SvcOpts, Nodes, Dict0, Svc}}) -> diameter_stats:reg(Ref), diameter_codec:setopts([{common_dictionary, Dict0} | SvcOpts]), {_,_} = Mask = proplists:get_value(sequence, SvcOpts), + Maxlen = proplists:get_value(incoming_maxlen, SvcOpts, 16#FFFFFF), {[Cs,Ds], Rest} = proplists:split(Opts, [capabilities_cb, disconnect_cb]), putr(?CB_KEY, {Ref, [F || {_,F} <- Cs]}), putr(?DPR_KEY, [F || {_, F} <- Ds]), @@ -223,7 +225,8 @@ i({Ack, WPid, {M, Ref} = T, Opts, {SvcOpts, Nodes, Dict0, Svc}}) -> dictionary = Dict0, mode = M, service = svc(Svc, Addrs), - length_errors = OnLengthErr}. + length_errors = OnLengthErr, + incoming_maxlen = Maxlen}. %% The transport returns its local ip addresses so that different %% transports on the same service can use different local addresses. %% The local addresses are put into Host-IP-Address avps here when @@ -326,11 +329,14 @@ handle_info(T, #state{} = State) -> {?MODULE, Tag, Reason} -> ?LOG(stop, Tag), {stop, {shutdown, Reason}, State} - end. + end; %% The form of the throw caught here is historical. It's %% significant that it's not a 2-tuple, as in ?FAILURE(Reason), %% since these are caught elsewhere. +handle_info(T, S) -> %% started in old code + handle_info(T, #state{} = erlang:append_element(S, infinity)). + %% Note that there's no guarantee that the service and transport %% capabilities are good enough to build a CER/CEA that can be %% succesfully encoded. It's not checked at diameter:add_transport/2 @@ -561,6 +567,12 @@ recv(Bin, S) -> %% recv1/3 +recv1(_, + #diameter_packet{header = H, bin = Bin}, + #state{incoming_maxlen = M}) + when M < size(Bin) -> + invalid(false, incoming_maxlen_exceeded, {size(Bin), H}); + %% Incoming request after outgoing DPR: discard. Don't discard DPR, so %% both ends don't do so when sending simultaneously. recv1(Name, diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl index a01bcdd4e7..86e744dfbe 100644 --- a/lib/diameter/src/base/diameter_service.erl +++ b/lib/diameter/src/base/diameter_service.erl @@ -131,7 +131,8 @@ | {share_peers, diameter:remotes()} %% broadcast to | {use_shared_peers, diameter:remotes()} %% use from | {restrict_connections, diameter:restriction()} - | {string_decode, boolean()}]}). + | {string_decode, boolean()} + | {incoming_maxlen, diameter:message_length()}]}). %% shared_peers reflects the peers broadcast from remote nodes. %% Record representing an RFC 3539 watchdog process implemented by @@ -698,7 +699,8 @@ service_options(Opts) -> Opts, ?RESTRICT)}, {spawn_opt, proplists:get_value(spawn_opt, Opts, [])}, - {string_decode, proplists:get_value(string_decode, Opts, true)}]. + {string_decode, proplists:get_value(string_decode, Opts, true)}, + {incoming_maxlen, proplists:get_value(incoming_maxlen, Opts, 16#FFFFFF)}]. %% The order of options is significant since we match against the list. mref(false = No) -> diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 784f9ca08f..538ebeeeba 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -78,7 +78,11 @@ service_name :: diameter:service_name(), apps :: [#diameter_app{}], sequence :: diameter:sequence(), - codec :: list()}). + codec :: [{string_decode, boolean()} + | {incoming_maxlen, diameter:message_length()}]}). +%% Note that incoming_maxlen is currently handled in diameter_peer_fsm, +%% so that any message exceeding the maximum is discarded. Retain the +%% option in case we want to extend the values and semantics. %% Record stored in diameter_request for each outgoing request. -record(request, @@ -102,7 +106,9 @@ make_recvdata([SvcName, PeerT, Apps, SvcOpts | _]) -> peerT = PeerT, apps = Apps, sequence = Mask, - codec = [T || {K,_} = T <- SvcOpts, K == string_decode]}. + codec = [T || {K,_} = T <- SvcOpts, + lists:member(K, [string_decode, + incoming_maxlen])]}. %% --------------------------------------------------------------------------- %% peer_up/1 @@ -233,6 +239,8 @@ receive_message(TPid, Pkt, Dict0, RecvData) Dict0, RecvData). +%% recv/6 + %% Incoming request ... recv(true, false, TPid, Pkt, Dict0, T) -> spawn_request(TPid, Pkt, Dict0, T); @@ -240,6 +248,7 @@ recv(true, false, TPid, Pkt, Dict0, T) -> %% ... answer to known request ... recv(false, #request{ref = Ref, handler = Pid} = Req, _, Pkt, Dict0, _) -> Pid ! {answer, Ref, Req, Dict0, Pkt}; + %% Note that failover could have happened prior to this message being %% received and triggering failback. That is, both a failover message %% and answer may be on their way to the handler process. In the worst @@ -1693,6 +1702,8 @@ send({TPid, Pkt, #request{handler = Pid} = Req0, SvcName, Timeout, TRef}) -> end. %% recv/4 +%% +%% Relay an answer from a remote node. recv(TPid, Pid, TRef, Ref) -> receive diff --git a/lib/diameter/test/diameter_config_SUITE.erl b/lib/diameter/test/diameter_config_SUITE.erl index ea77aa3716..bbdf672291 100644 --- a/lib/diameter/test/diameter_config_SUITE.erl +++ b/lib/diameter/test/diameter_config_SUITE.erl @@ -85,6 +85,9 @@ {string_decode, [[true], [false]], [[0], [x]]}, + {incoming_maxlen, + [[0], [65536], [16#FFFFFF]], + [[-1], [1 bsl 24], [infinity], [false]]}, {spawn_opt, [[[]], [[monitor, link]]], [[false]]}, diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 10c58ab6e7..7dd9f39f85 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -59,6 +59,7 @@ send_unexpected_mandatory_decode/1, send_unexpected_mandatory/1, send_long/1, + send_maxlen/1, send_nopeer/1, send_noapp/1, send_discard/1, @@ -182,6 +183,7 @@ {'Acct-Application-Id', [?DIAMETER_APP_ID_ACCOUNTING]}, {restrict_connections, false}, {string_decode, Decode}, + {incoming_maxlen, 1 bsl 21}, {spawn_opt, [{min_heap_size, 5000}]} | [{application, [{dictionary, D}, {module, ?MODULE}, @@ -317,6 +319,7 @@ tc() -> send_unexpected_mandatory_decode, send_unexpected_mandatory, send_long, + send_maxlen, send_nopeer, send_noapp, send_discard, @@ -635,6 +638,12 @@ send_long(Config) -> ['STA', _SessionId, {'Result-Code', ?SUCCESS} | _] = call(Config, Req). +%% Send something longer than the configure incoming_maxlen. +send_maxlen(Config) -> + Req = ['STR', {'Termination-Cause', ?LOGOUT}, + {'User-Name', [lists:duplicate(1 bsl 21, $X)]}], + {timeout, _} = call(Config, Req). + %% Send something for which pick_peer finds no suitable peer. send_nopeer(Config) -> Req = ['STR', {'Termination-Cause', ?LOGOUT}], -- cgit v1.2.3 From f3e95a4d4278fda5a0648943020bdf0026219f7c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Tue, 24 Mar 2015 13:20:36 +0100 Subject: Limit DiameterURI ports to 0-65535 digits on decode A port number is a 16-bit integer, but the regexp used to parse it in commit 1590920 slavishly followed the RFC 6733 grammar in matching an arbitrary number of digits. Make decode fail if it's anything more than 5, to avoid doing erlang:list_to_integer/1 on arbitrarily large lists. Also make it fail if the resulting integer is outside of the expected range. --- lib/diameter/src/base/diameter_types.erl | 12 ++++++++++-- lib/diameter/test/diameter_codec_test.erl | 6 ++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/diameter/src/base/diameter_types.erl b/lib/diameter/src/base/diameter_types.erl index fe7613541c..96407efc09 100644 --- a/lib/diameter/src/base/diameter_types.erl +++ b/lib/diameter/src/base/diameter_types.erl @@ -566,17 +566,25 @@ msb(false) -> ?TIME_2036. scan_uri(Bin) -> RE = "^(aaas?)://" "([-a-zA-Z0-9.]+)" - "(:([0-9]+))?" + "(:0{0,5}([0-9]{1,5}))?" "(;transport=(tcp|sctp|udp))?" "(;protocol=(diameter|radius|tacacs\\+))?$", + %% A port number is 16-bit, so an arbitrary number of digits is + %% just a vulnerability, but provide a little slack with leading + %% zeros in a port number just because the regexp was previously + %% [0-9]+ and it's not inconceivable that a value might be padded. + %% Don't fantasize about this padding being more than the number + %% of digits in the port number proper. {match, [A, DN, PN, T, P]} = re:run(Bin, RE, [{capture, [1,2,4,6,8], binary}]), Type = to_atom(A), {PN0, T0} = defaults(diameter_codec:getopt(rfc), Type), + PortNr = to_int(PN, PN0), + 0 = PortNr bsr 16, %% assert #diameter_uri{type = Type, fqdn = from_bin(DN), - port = to_int(PN, PN0), + port = PortNr, transport = to_atom(T, T0), protocol = to_atom(P, diameter)}. diff --git a/lib/diameter/test/diameter_codec_test.erl b/lib/diameter/test/diameter_codec_test.erl index 854b71ba93..11fa82cfa1 100644 --- a/lib/diameter/test/diameter_codec_test.erl +++ b/lib/diameter/test/diameter_codec_test.erl @@ -352,14 +352,16 @@ values('DiameterURI') -> {[], ["aaa" ++ S ++ "://diameter.se" ++ P ++ Tr ++ Pr || S <- ["", "s"], - P <- ["", ":1234"], + P <- ["", ":1234", ":0", ":65535"], Tr <- ["" | [";transport=" ++ X || X <- ["tcp", "sctp", "udp"]]], Pr <- ["" | [";protocol=" ++ X || X <- ["diameter","radius","tacacs+"]]], Tr /= ";transport=udp" orelse (Pr /= ";protocol=diameter" andalso Pr /= "")], - ["aaa://diameter.se;transport=udp;protocol=diameter", + ["aaa://diameter.se:65536", + "aaa://diameter.se:-1", + "aaa://diameter.se;transport=udp;protocol=diameter", "aaa://diameter.se;transport=udp", "aaa://:3868", "aaax://diameter.se", -- cgit v1.2.3 From 7edb0dd681b09df8865855eda1150e4a92b54a0a Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 26 Mar 2015 12:52:04 +0100 Subject: Limit FQDN in DiameterURI to 255 octets As for the port number in the parent commit, a FQDN can't be arbitrarily long, at most 255 octets. Make decode fail if it's more. --- lib/diameter/doc/src/diameter_dict.xml | 2 +- lib/diameter/src/base/diameter_types.erl | 17 ++++++----------- lib/diameter/test/diameter_codec_test.erl | 4 +++- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/lib/diameter/doc/src/diameter_dict.xml b/lib/diameter/doc/src/diameter_dict.xml index 9db9bcffde..5cf1b174a0 100644 --- a/lib/diameter/doc/src/diameter_dict.xml +++ b/lib/diameter/doc/src/diameter_dict.xml @@ -529,7 +529,7 @@ answer record and passed to a &app_handle_request; callback upon reception of an incoming request.

-In cases in which there is a choice between list() and binary() types +In cases in which there is a choice between string() and binary() types for OctetString() and derived types, the representation is determined by the value of &mod_string_decode;.

diff --git a/lib/diameter/src/base/diameter_types.erl b/lib/diameter/src/base/diameter_types.erl index 96407efc09..87a0f0663d 100644 --- a/lib/diameter/src/base/diameter_types.erl +++ b/lib/diameter/src/base/diameter_types.erl @@ -93,7 +93,7 @@ case diameter_codec:getopt(string_decode) of true -> binary_to_list(Bin); - _ -> + false -> Bin end; @@ -565,7 +565,7 @@ msb(false) -> ?TIME_2036. scan_uri(Bin) -> RE = "^(aaas?)://" - "([-a-zA-Z0-9.]+)" + "([-a-zA-Z0-9.]{1,255})" "(:0{0,5}([0-9]{1,5}))?" "(;transport=(tcp|sctp|udp))?" "(;protocol=(diameter|radius|tacacs\\+))?$", @@ -575,6 +575,9 @@ scan_uri(Bin) -> %% [0-9]+ and it's not inconceivable that a value might be padded. %% Don't fantasize about this padding being more than the number %% of digits in the port number proper. + %% + %% Similarly, a FQDN can't be arbitrarily long: at most 255 + %% octets. {match, [A, DN, PN, T, P]} = re:run(Bin, RE, [{capture, [1,2,4,6,8], binary}]), @@ -583,7 +586,7 @@ scan_uri(Bin) -> PortNr = to_int(PN, PN0), 0 = PortNr bsr 16, %% assert #diameter_uri{type = Type, - fqdn = from_bin(DN), + fqdn = 'OctetString'(decode, DN), port = PortNr, transport = to_atom(T, T0), protocol = to_atom(P, diameter)}. @@ -596,14 +599,6 @@ defaults(6733, aaa) -> defaults(6733, aaas) -> {5658, tcp}. -from_bin(B) -> - case diameter_codec:getopt(string_decode) of - true -> - binary_to_list(B); - false -> - B - end. - to_int(<<>>, N) -> N; to_int(B, _) -> diff --git a/lib/diameter/test/diameter_codec_test.erl b/lib/diameter/test/diameter_codec_test.erl index 11fa82cfa1..5f1dbfbd61 100644 --- a/lib/diameter/test/diameter_codec_test.erl +++ b/lib/diameter/test/diameter_codec_test.erl @@ -358,11 +358,13 @@ values('DiameterURI') -> Pr <- ["" | [";protocol=" ++ X || X <- ["diameter","radius","tacacs+"]]], Tr /= ";transport=udp" - orelse (Pr /= ";protocol=diameter" andalso Pr /= "")], + orelse (Pr /= ";protocol=diameter" andalso Pr /= "")] + ++ ["aaa://" ++ lists:duplicate(255, $x)], ["aaa://diameter.se:65536", "aaa://diameter.se:-1", "aaa://diameter.se;transport=udp;protocol=diameter", "aaa://diameter.se;transport=udp", + "aaa://" ++ lists:duplicate(256, $x), "aaa://:3868", "aaax://diameter.se", "aaa://diameter.se;transport=tcpx", -- cgit v1.2.3 From 39acfdb005626ae1bf2f68808f9e8116637c7121 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 26 Mar 2015 23:07:59 +0100 Subject: Remove potentially large error reason in call to diameter_lib:log/4 The function is intended to be traced on, to see abnormalities (mostly) without producing excessive output. In the case of decode failure, the error reason can be things like {badmatch, HugeBinary}. Missed in commit 0058430. --- lib/diameter/include/diameter_gen.hrl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index 8272904856..0eef218a07 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -395,7 +395,7 @@ d(false, Reason, Name, Avp, {Avps, Acc}) -> diameter_lib:log(decode_error, ?MODULE, ?LINE, - {Reason, Name, Avp#diameter_avp.name, Stack}), + {Name, Avp#diameter_avp.name, Stack}), {Rec, Failed} = Acc, {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}}. -- cgit v1.2.3