aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--lib/diameter/doc/src/diameter.xml21
-rw-r--r--lib/diameter/doc/src/diameter_dict.xml2
-rw-r--r--lib/diameter/include/diameter_gen.hrl2
-rw-r--r--lib/diameter/src/base/diameter.erl5
-rw-r--r--lib/diameter/src/base/diameter_config.erl14
-rw-r--r--lib/diameter/src/base/diameter_peer_fsm.erl18
-rw-r--r--lib/diameter/src/base/diameter_service.erl6
-rw-r--r--lib/diameter/src/base/diameter_traffic.erl15
-rw-r--r--lib/diameter/src/base/diameter_types.erl29
-rw-r--r--lib/diameter/test/diameter_codec_test.erl10
-rw-r--r--lib/diameter/test/diameter_config_SUITE.erl9
-rw-r--r--lib/diameter/test/diameter_traffic_SUITE.erl9
12 files changed, 113 insertions, 27 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
</item>
+<marker id="incoming_maxlen"/>
+<tag><c>{incoming_maxlen, 0..16777215}</c></tag>
+<item>
+<p>
+Bound on the expected size of incoming Diameter messages.
+Messages larger than the specified number of bytes are discarded.</p>
+
+<p>
+Defaults to <c>16777215</c>, the maximum value of the 24-bit Message
+Length field in a Diameter Header.</p>
+
+<warning>
+<p>
+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.</p>
+</warning>
+
+</item>
+
<tag><c>{restrict_connections, false
| node
| nodes
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.</p>
<p>
-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;.</p>
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]}}.
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 0d0304bf33..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,12 +672,20 @@ 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;
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/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/src/base/diameter_types.erl b/lib/diameter/src/base/diameter_types.erl
index fe7613541c..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,18 +565,29 @@ msb(false) -> ?TIME_2036.
scan_uri(Bin) ->
RE = "^(aaas?)://"
- "([-a-zA-Z0-9.]+)"
- "(:([0-9]+))?"
+ "([-a-zA-Z0-9.]{1,255})"
+ "(: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.
+ %%
+ %% 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}]),
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),
+ fqdn = 'OctetString'(decode, DN),
+ port = PortNr,
transport = to_atom(T, T0),
protocol = to_atom(P, diameter)}.
@@ -588,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 854b71ba93..5f1dbfbd61 100644
--- a/lib/diameter/test/diameter_codec_test.erl
+++ b/lib/diameter/test/diameter_codec_test.erl
@@ -352,15 +352,19 @@ 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",
+ 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",
diff --git a/lib/diameter/test/diameter_config_SUITE.erl b/lib/diameter/test/diameter_config_SUITE.erl
index 77f7aace1b..bbdf672291 100644
--- a/lib/diameter/test/diameter_config_SUITE.erl
+++ b/lib/diameter/test/diameter_config_SUITE.erl
@@ -85,6 +85,12 @@
{string_decode,
[[true], [false]],
[[0], [x]]},
+ {incoming_maxlen,
+ [[0], [65536], [16#FFFFFF]],
+ [[-1], [1 bsl 24], [infinity], [false]]},
+ {spawn_opt,
+ [[[]], [[monitor, link]]],
+ [[false]]},
{invalid_option, %% invalid service options are rejected
[],
[[x],
@@ -186,6 +192,9 @@
{private,
[[x]],
[]},
+ {spawn_opt,
+ [[[]], [[monitor, link]]],
+ [[false]]},
{invalid_option, %% invalid transport options are silently ignored
[[x],
[x,x]],
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}],
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-03 10:40:38 +0000