From 1732c9c9bd6b261cb18f2ff174a8c4d1b9488f3e Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 3 Apr 2015 11:17:58 +0200 Subject: Remove extra avp bit from diameter_avp decode In the case of a faulty AVP Length (pointing past the end of a message or not spanning the header), an extra bit is prepended to data bytes in diameter_avp:collect_avps/1 in order to force a 5014 decode error. The bit is supposed to be removed as part of the decode in diameter_gen.hrl but this didn't happen in case of an AVP that unknown to the dictionary in question. --- lib/diameter/src/base/diameter_codec.erl | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'lib/diameter/src') diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index 15a4c5e86f..bf2fe8e7ca 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -640,8 +640,12 @@ split_data(Bin, Len) -> %% payload if this is a request. Do this (in cases that we %% know the type) by inducing a decode failure and letting %% the dictionary's decode (in diameter_gen) deal with it. - %% Here we don't know type. If the type isn't known, then - %% the decode just strips the extra bit. + %% + %% Note that the extra bit can only occur in the trailing + %% AVP of a message or Grouped AVP, since a faulty AVP + %% Length is otherwise indistinguishable from a correct + %% one here, since we don't know the types of the AVPs + %% being extracted. {<<0:1, Bin/binary>>, <<>>} end. @@ -690,8 +694,8 @@ pack_avp(#diameter_avp{code = undefined, data = B}) Len = size(<> = <>), <>; -%% ... from a dictionary compiled against old code in diameter_gen ... %% ... when ignoring errors in Failed-AVP ... +%% ... during a relay encode ... pack_avp(#diameter_avp{data = <<0:1, B/binary>>} = A) -> pack_avp(A#diameter_avp{data = B}); -- cgit v1.2.3 From 49e8b11ce3620e686e3388ee7d8a0f445b5f853a Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sun, 3 May 2015 11:56:15 +0200 Subject: Fix counting error with unknown application id Statistics could be accumulated on a key like {{23,275,0}, recv} even though 23 was not the application id of the dictionary in question. Missed in commits df19c272 and 7816ab2f. --- lib/diameter/src/base/diameter_traffic.erl | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) (limited to 'lib/diameter/src') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 538ebeeeba..bdb83ee682 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -1116,19 +1116,15 @@ msg_id(#diameter_packet{header = H}, Dict) -> %% there are 2^32 (application ids) * 2^24 (command codes) = 2^56 %% pairs for an attacker to choose from. msg_id(Hdr, Dict) -> - {_ApplId, Code, R} = Id = diameter_codec:msg_id(Hdr), - case Dict:msg_name(Code, 0 == R) of - '' -> - unknown(Dict:id(), R); - _ -> - Id + {Aid, Code, R} = Id = diameter_codec:msg_id(Hdr), + if Aid == ?APP_ID_RELAY -> + {relay, R}; + true -> + choose(Aid /= Dict:id() orelse '' == Dict:msg_name(Code, 0 == R), + unknown, + Id) end. -unknown(?APP_ID_RELAY, R) -> - {relay, R}; -unknown(_, _) -> - unknown. - %% No E-bit: can't be 3xxx. is_result(RC, false, _Dict0) -> RC < 3000 orelse 4000 =< RC; -- cgit v1.2.3 From 2299840162aaeb311658a2729078313cab71add8 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 2 May 2015 15:09:39 +0200 Subject: Update appup for 17.5.3 Required load order by ticket. - OTP-12642, extra bit in diameter_avp.data - OTP-12654, Result-Code/Experimental-Result confusion - OTP-12701, counting error with unknown Application Id none --- lib/diameter/src/diameter.appup.src | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) (limited to 'lib/diameter/src') diff --git a/lib/diameter/src/diameter.appup.src b/lib/diameter/src/diameter.appup.src index a54eb24031..cff936d893 100644 --- a/lib/diameter/src/diameter.appup.src +++ b/lib/diameter/src/diameter.appup.src @@ -84,7 +84,14 @@ {load_module, diameter_gen_relay}, {update, diameter_transport_sup, supervisor}, {update, diameter_service_sup, supervisor}, - {update, diameter_sup, supervisor}]} + {update, diameter_sup, supervisor}]}, + {"1.9", [{load_module, diameter_codec}, %% 17.5 + {load_module, diameter_traffic}, + {load_module, diameter_gen_base_rfc6733}, + {load_module, diameter_gen_acct_rfc6733}, + {load_module, diameter_gen_base_rfc3588}, + {load_module, diameter_gen_base_accounting}, + {load_module, diameter_gen_relay}]} ], [ {"0.9", [{restart_application, diameter}]}, @@ -151,6 +158,13 @@ {load_module, diameter_session}, {load_module, diameter_reg}, {load_module, diameter_peer}, - {load_module, diameter_lib}]} + {load_module, diameter_lib}]}, + {"1.9", [{load_module, diameter_gen_relay}, + {load_module, diameter_gen_base_accounting}, + {load_module, diameter_gen_base_rfc3588}, + {load_module, diameter_gen_acct_rfc6733}, + {load_module, diameter_gen_base_rfc6733}, + {load_module, diameter_traffic}, + {load_module, diameter_codec}]} ] }. -- cgit v1.2.3 From 07c92d47ad578e3ddc7910bc45c4f9103a364f24 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 2 May 2015 15:46:48 +0200 Subject: Fix broken pre-17.4 appup Upgrade instructions have been added for each 17.X release without adjusting the instructions for preceeding releases: the instructions have only been sufficient to upgrading one release at a time: 17.0 to 17.1, 17.1 to 17.2, etc. Conficting load order requirements make smooth upgrade from an arbitrarily old release impossible. In this case, 17.3 looks to be as far back as we can go, so require restart from 17.[0-2] or older. Update the app suite to deal with binary regexps in appup, and to match version numbers harder. --- lib/diameter/src/diameter.appup.src | 60 +++++-------------------------------- 1 file changed, 8 insertions(+), 52 deletions(-) (limited to 'lib/diameter/src') diff --git a/lib/diameter/src/diameter.appup.src b/lib/diameter/src/diameter.appup.src index cff936d893..0ef0fd35a9 100644 --- a/lib/diameter/src/diameter.appup.src +++ b/lib/diameter/src/diameter.appup.src @@ -35,32 +35,10 @@ {"1.4.3", [{restart_application, diameter}]}, %% R16B02 {"1.4.4", [{restart_application, diameter}]}, {"1.5", [{restart_application, diameter}]}, %% R16B03 - {"1.6", [{load_module, diameter_lib}, %% 17.0 - {load_module, diameter_traffic}, - {load_module, diameter_watchdog}, - {load_module, diameter_peer_fsm}, - {load_module, diameter_service}, - {load_module, diameter_gen_base_rfc6733}, - {load_module, diameter_gen_acct_rfc6733}, - {load_module, diameter_gen_base_rfc3588}, - {load_module, diameter_gen_base_accounting}, - {load_module, diameter_gen_relay}, - {load_module, diameter_codec}, - {load_module, diameter_sctp}]}, - {"1.7", [{load_module, diameter_service}, %% 17.1 - {load_module, diameter_codec}, - {load_module, diameter_gen_base_rfc6733}, - {load_module, diameter_gen_acct_rfc6733}, - {load_module, diameter_gen_base_rfc3588}, - {load_module, diameter_gen_base_accounting}, - {load_module, diameter_gen_relay}, - {load_module, diameter_traffic}, - {load_module, diameter_peer_fsm}]}, - {"1.7.1", [{load_module, diameter_traffic}, %% 17.3 - {load_module, diameter_watchdog}, - {load_module, diameter_peer_fsm}, - {load_module, diameter_service}]}, - {"1.8", [{load_module, diameter_lib}, %% 17.4 + {"1.6", [{restart_application, diameter}]}, %% 17.0 + {"1.7", [{restart_application, diameter}]}, %% 17.[12] + {<<"^1\\.(7\\.1|8)$">>, %% 17.[34] + [{load_module, diameter_lib}, {load_module, diameter_peer}, {load_module, diameter_reg}, {load_module, diameter_session}, @@ -109,32 +87,10 @@ {"1.4.3", [{restart_application, diameter}]}, {"1.4.4", [{restart_application, diameter}]}, {"1.5", [{restart_application, diameter}]}, - {"1.6", [{load_module, diameter_sctp}, - {load_module, diameter_codec}, - {load_module, diameter_gen_relay}, - {load_module, diameter_gen_base_accounting}, - {load_module, diameter_gen_base_rfc3588}, - {load_module, diameter_gen_acct_rfc6733}, - {load_module, diameter_gen_base_rfc6733}, - {load_module, diameter_service}, - {load_module, diameter_peer_fsm}, - {load_module, diameter_watchdog}, - {load_module, diameter_traffic}, - {load_module, diameter_lib}]}, - {"1.7", [{load_module, diameter_peer_fsm}, - {load_module, diameter_traffic}, - {load_module, diameter_gen_relay}, - {load_module, diameter_gen_base_accounting}, - {load_module, diameter_gen_base_rfc3588}, - {load_module, diameter_gen_acct_rfc6733}, - {load_module, diameter_gen_base_rfc6733}, - {load_module, diameter_codec}, - {load_module, diameter_service}]}, - {"1.7.1", [{load_module, diameter_service}, - {load_module, diameter_peer_fsm}, - {load_module, diameter_watchdog}, - {load_module, diameter_traffic}]}, - {"1.8", [{update, diameter_sup, supervisor}, + {"1.6", [{restart_application, diameter}]}, + {"1.7", [{restart_application, diameter}]}, + {<<"^1\\.(7\\.1|8)$">>, + [{update, diameter_sup, supervisor}, {update, diameter_service_sup, supervisor}, {update, diameter_transport_sup, supervisor}, {load_module, diameter_gen_relay}, -- cgit v1.2.3 From a1df50b38826370b84ec557de401bc1b09d56de7 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 2 May 2015 10:14:58 +0200 Subject: Don't confuse Result-Code and Experimental-Result Decode of an answer message not setting the E-bit, and containing Experiment-Result but not Result-Code, identified Result-Code as the erroneous when Erroneous-Result-Code was 3xxx. Here's an example (from trace) of a the errors field after decode: [{5004, {diameter_avp,undefined,undefined,false,false,undefined,'Result-Code', 3001,undefined,undefined}}], The diameter_avp was just constructed from the AVP name and decoded result, without regard for which result code AVP contained the value. Fix by extracting the AVP from the incoming message. --- lib/diameter/src/base/diameter_traffic.erl | 158 ++++++++++++++++------------- 1 file changed, 89 insertions(+), 69 deletions(-) (limited to 'lib/diameter/src') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 538ebeeeba..9a9f7a3197 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -980,8 +980,8 @@ answer_message(OH, OR, RC, Dict0, #diameter_packet{avps = Avps, session_id(Code, Vid, Dict0, Avps) when is_list(Avps) -> try - {value, #diameter_avp{data = D}} = find_avp(Code, Vid, Avps), - [{'Session-Id', [Dict0:avp(decode, D, 'Session-Id')]}] + #diameter_avp{data = Bin} = find_avp(Code, Vid, Avps), + [{'Session-Id', [Dict0:avp(decode, Bin, 'Session-Id')]}] catch error: _ -> [] @@ -998,26 +998,17 @@ failed_avp(_, [] = No) -> %% find_avp/3 -find_avp(Code, Vid, Avps) - when is_integer(Code), (undefined == Vid orelse is_integer(Vid)) -> - find(fun(A) -> is_avp(Code, Vid, A) end, Avps). +%% Grouped ... +find_avp(Code, VId, [[#diameter_avp{code = Code, vendor_id = VId} | _] = As + | _]) -> + As; -%% The final argument here could be a list of AVP's, depending on the case, -%% but we're only searching at the top level. -is_avp(Code, Vid, #diameter_avp{code = Code, vendor_id = Vid}) -> - true; -is_avp(_, _, _) -> - false. +%% ... or not. +find_avp(Code, VId, [#diameter_avp{code = Code, vendor_id = VId} = A | _]) -> + A; -find(_, []) -> - false; -find(Pred, [H|T]) -> - case Pred(H) of - true -> - {value, H}; - false -> - find(Pred, T) - end. +find_avp(Code, VId, [_ | Avps]) -> + find_avp(Code, VId, Avps). %% 7. Error Handling %% @@ -1086,7 +1077,6 @@ incr_result(_, #diameter_packet{msg = undefined = No}, _, _) -> incr_result(Dir, Pkt, TPid, {Dict, AppDict, Dict0}) -> #diameter_packet{header = #diameter_header{is_error = E} = Hdr, - msg = Msg, errors = Es} = Pkt, @@ -1096,13 +1086,13 @@ incr_result(Dir, Pkt, TPid, {Dict, AppDict, Dict0}) -> recv /= Dir orelse [] == Es orelse incr_error(Dir, Id, TPid, AppDict), %% Exit on a missing result code. - T = rc_counter(Dict, Msg), + T = rc_counter(Dict, Dir, Pkt), T == false andalso ?LOGX(no_result_code, {Dict, Dir, Hdr}), - {Ctr, RC} = T, + {Ctr, RC, Avp} = T, %% Or on an inappropriate value. is_result(RC, E, Dict0) - orelse ?LOGX(invalid_error_bit, {Dict, Dir, Hdr, RC}), + orelse ?LOGX(invalid_error_bit, {Dict, Dir, Hdr, Avp}), incr(TPid, {Id, Dir, Ctr}), Ctr. @@ -1148,7 +1138,7 @@ is_result(RC, true, _) -> incr(TPid, Counter) -> diameter_stats:incr(Counter, TPid, 1). -%% rc_counter/2 +%% rc_counter/3 %% RFC 3588, 7.6: %% @@ -1156,39 +1146,45 @@ incr(TPid, Counter) -> %% applications MUST include either one Result-Code AVP or one %% Experimental-Result AVP. -rc_counter(Dict, Msg) -> - rcc(Dict, Msg, int(get_avp_value(Dict, 'Result-Code', Msg))). +rc_counter(Dict, recv, #diameter_packet{header = H, avps = As}) -> + rc_counter(Dict, [H|As]); -rcc(Dict, Msg, undefined) -> - rcc(get_avp_value(Dict, 'Experimental-Result', Msg)); +rc_counter(Dict, _, #diameter_packet{msg = Msg}) -> + rc_counter(Dict, Msg). + +rc_counter(Dict, Msg) -> + rcc(get_result(Dict, Msg)). -rcc(_, _, N) +rcc(#diameter_avp{name = 'Result-Code' = Name, value = N} = A) when is_integer(N) -> - {{'Result-Code', N}, N}. + {{Name, N}, N, A}; -%% Outgoing answers may be in any of the forms messages can be sent -%% in. Incoming messages will be records. We're assuming here that the -%% arity of the result code AVP's is 0 or 1. +rcc(#diameter_avp{name = 'Result-Code' = Name, value = [N|_]} = A) + when is_integer(N) -> + {{Name, N}, N, A}; -rcc([{_,_,N} = T | _]) +rcc(#diameter_avp{name = 'Experimental-Result', value = {_,_,N} = T} = A) when is_integer(N) -> - {T,N}; -rcc({_,_,N} = T) + {T, N, A}; + +rcc(#diameter_avp{name = 'Experimental-Result', value = [{_,_,N} = T|_]} = A) when is_integer(N) -> - {T,N}; + {T, N, A}; + rcc(_) -> false. -%% Extract the first good looking integer. There's no guarantee -%% that what we're looking for has arity 1. -int([N|_]) - when is_integer(N) -> - N; -int(N) - when is_integer(N) -> - N; -int(_) -> - undefined. +%% get_result/2 + +get_result(Dict, Msg) -> + try + [throw(A) || N <- ['Result-Code', 'Experimental-Result'], + #diameter_avp{} = A <- [get_avp(Dict, N, Msg)]] + of + [] -> false + catch + #diameter_avp{} = A -> A + end. x(T) -> exit(T). @@ -1528,10 +1524,10 @@ handle_A(Pkt, SvcName, Dict, Dict0, App, #request{transport = TPid} = Req) -> %% a missing AVP. If both are optional in the dictionary %% then this isn't a decode error: just continue on. answer(Pkt, SvcName, App, Req); - exit: {invalid_error_bit, {_, _, _, RC}} -> + exit: {invalid_error_bit, {_, _, _, Avp}} -> #diameter_packet{errors = Es} = Pkt, - E = {5004, #diameter_avp{name = 'Result-Code', value = RC}}, + E = {5004, Avp}, answer(Pkt#diameter_packet{errors = [E|Es]}, SvcName, App, Req) end. @@ -1868,7 +1864,7 @@ str([]) -> str(T) -> T. -%% get_avp_value/3 +%% get_avp/3 %% %% Find an AVP in a message of one of three forms: %% @@ -1885,47 +1881,71 @@ str(T) -> %% look for are in the common dictionary. This is required since the %% relay dictionary doesn't inherit the common dictionary (which maybe %% it should). -get_avp_value(?RELAY, Name, Msg) -> - get_avp_value(?BASE, Name, Msg); +get_avp(?RELAY, Name, Msg) -> + get_avp(?BASE, Name, Msg); -%% Message sent as a header/avps list, probably a relay case but not -%% necessarily. -get_avp_value(Dict, Name, [#diameter_header{} | Avps]) -> +%% Message as a header/avps list. +get_avp(Dict, Name, [#diameter_header{} | Avps]) -> try {Code, _, VId} = Dict:avp_header(Name), - [A|_] = lists:dropwhile(fun(#diameter_avp{code = C, vendor_id = V}) -> - C /= Code orelse V /= VId - end, - Avps), - avp_decode(Dict, Name, A) + find_avp(Code, VId, Avps) + of + A -> + avp_decode(Dict, Name, ungroup(A)) catch error: _ -> undefined end; %% Outgoing message as a name/values list. -get_avp_value(_, Name, [_MsgName | Avps]) -> +get_avp(_, Name, [_MsgName | Avps]) -> case lists:keyfind(Name, 1, Avps) of {_, V} -> - V; + #diameter_avp{name = Name, value = V}; _ -> undefined end; %% Message is typically a record but not necessarily. -get_avp_value(Dict, Name, Rec) -> +get_avp(Dict, Name, Rec) -> try - Dict:'#get-'(Name, Rec) + #diameter_avp{name = Name, value = Dict:'#get-'(Name, Rec)} catch error:_ -> undefined end. +%% get_avp_value/3 + +get_avp_value(Dict, Name, Msg) -> + case get_avp(Dict, Name, Msg) of + #diameter_avp{value = V} -> + V; + undefined = No -> + No + end. + +%% ungroup/1 + +ungroup([Avp|_]) -> + Avp; +ungroup(Avp) -> + Avp. + +%% avp_decode/3 + avp_decode(Dict, Name, #diameter_avp{value = undefined, - data = Bin}) -> - Dict:avp(decode, Bin, Name); -avp_decode(_, _, #diameter_avp{value = V}) -> - V. + data = Bin} + = Avp) -> + try Dict:avp(decode, Bin, Name) of + V -> + Avp#diameter_avp{value = V} + catch + error:_ -> + Avp + end; +avp_decode(_, _, #diameter_avp{} = Avp) -> + Avp. cb(#diameter_app{module = [_|_] = M}, F, A) -> eval(M, F, A); -- cgit v1.2.3