From 4ce2d3a67488811be96853f9b83ef8da2712b5e7 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 5 Apr 2013 11:04:07 +0200 Subject: Fix recognition of 5014 (INVALID_AVP_LENGTH) errors Invalid lengths come in two flavours: ones that correctly point at the end of an AVP's payload but don't agree with its type, and ones that point elsewhere. The former are relatively harmless but the latter leave no way to recover AVP boundaries, which typically results in failure to decode subsequent AVP's in the message in question. In the case that AVP Length points past the end of the message, diameter incorrectly regarded the error as 5009, INVALID_AVP_BITS: not right since the error has nothing to do with AVP Flags. Ditto if the length was less than 8, a minimal header length. Only in the remaining case was the detected error 5014, INVALID_AVP_LENGTH. However, in this case it slavishly followed RFC 3588 in suggesting the undecodable AVP as Failed-AVP, thereby passing the woeful payload back to the sender to have equal difficulty decoding. Now follow RFC 6733 and suggest an AVP with a zero-filled payload. --- lib/diameter/src/base/diameter_codec.erl | 135 +++++++++++++++++++++-------- lib/diameter/src/base/diameter_traffic.erl | 11 ++- 2 files changed, 104 insertions(+), 42 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 09b99b2cae..0a8ea850d7 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -225,9 +225,9 @@ decode(Mod, Pkt) -> %% question. decode(?APP_ID_RELAY, _, #diameter_packet{} = Pkt) -> case collect_avps(Pkt) of - {Bs, As} -> + {E, As} -> Pkt#diameter_packet{avps = As, - errors = [Bs]}; + errors = [E]}; As -> Pkt#diameter_packet{avps = As} end; @@ -251,12 +251,12 @@ decode(Id, Mod, Bin) when is_bitstring(Bin) -> decode(Id, Mod, #diameter_packet{header = decode_header(Bin), bin = Bin}). -decode_avps(MsgName, Mod, Pkt, {Bs, Avps}) -> %% invalid avp bits ... +decode_avps(MsgName, Mod, Pkt, {E, Avps}) -> ?LOG(invalid, Pkt#diameter_packet.bin), #diameter_packet{errors = Failed} = P = decode_avps(MsgName, Mod, Pkt, Avps), - P#diameter_packet{errors = [Bs | Failed]}; + P#diameter_packet{errors = [E | Failed]}; decode_avps('', Mod, Pkt, Avps) -> %% unknown message ... ?LOG(unknown, {Mod, Pkt#diameter_packet.header}), @@ -403,8 +403,8 @@ collect_avps(Bin, N, Acc) -> {Rest, AVP} -> collect_avps(Rest, N+1, [AVP#diameter_avp{index = N} | Acc]) catch - ?FAILURE(_) -> - {Bin, Acc} + ?FAILURE(Error) -> + {Error, Acc} end. %% 0 1 2 3 @@ -422,44 +422,87 @@ collect_avps(Bin, N, Acc) -> %% split_avp/1 split_avp(Bin) -> - 8 =< size(Bin) orelse ?THROW(truncated_header), + {Code, V, M, P, Len, HdrLen} = split_head(Bin), + {Data, B} = split_data(Bin, HdrLen, Len - HdrLen), - <> - = Bin, + {B, #diameter_avp{code = Code, + vendor_id = V, + is_mandatory = 1 == M, + need_encryption = 1 == P, + data = Data}}. - 8 =< Length orelse ?THROW(invalid_avp_length), +%% split_head/1 - DataSize = Length - 8, % size(Code+Flags+Length) = 8 octets - PadSize = (4 - (DataSize rem 4)) rem 4, +split_head(<>) -> + {Code, V, M, P, Len, 12}; - DataSize + PadSize =< size(Rest) - orelse ?THROW(truncated_data), +split_head(<>) -> + {Code, undefined, M, P, Len, 8}; - <> - = Rest, - <> - = Flags, +split_head(Bin) -> + ?THROW({5014, #diameter_avp{data = Bin}}). - 0 == Vbit orelse 4 =< size(Data) - orelse ?THROW(truncated_vendor_id), +%% 3588: +%% +%% DIAMETER_INVALID_AVP_LENGTH 5014 +%% The request contained an AVP with an invalid length. A Diameter +%% message indicating this error MUST include the offending AVPs +%% within a Failed-AVP AVP. + +%% 6733: +%% +%% DIAMETER_INVALID_AVP_LENGTH 5014 +%% +%% The request contained an AVP with an invalid length. A Diameter +%% message indicating this error MUST include the offending AVPs +%% within a Failed-AVP AVP. In cases where the erroneous AVP length +%% value exceeds the message length or is less than the minimum AVP +%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +%% header length, it is sufficient to include the offending AVP +%% ^^^^^^^^^^^^^ +%% header and a zero filled payload of the minimum required length +%% for the payloads data type. If the AVP is a Grouped AVP, the +%% Grouped AVP header with an empty payload would be sufficient to +%% indicate the offending AVP. In the case where the offending AVP +%% header cannot be fully decoded when the AVP length is less than +%% the minimum AVP header length, it is sufficient to include an +%% offending AVP header that is formulated by padding the incomplete +%% AVP header with zero up to the minimum AVP header length. +%% +%% The underlined clause must be in error since (1) a header less than +%% the minimum value mean we don't know the identity of the AVP and +%% (2) the last sentence covers this case. - {Vid, D} = vid(Vbit, Data), - {R, #diameter_avp{code = Code, - vendor_id = Vid, - is_mandatory = 1 == Mbit, - need_encryption = 1 == Pbit, - data = D}}. +%% split_data/3 -%% The RFC is a little misleading when stating that OctetString is -%% padded to a 32-bit boundary while other types align naturally. All -%% other types are already multiples of 32 bits so there's no need to -%% distinguish between types here. Any invalid lengths will result in -%% decode error in diameter_types. +split_data(Bin, HdrLen, Len) + when 0 =< Len -> + split_data(Bin, HdrLen, Len, (4 - (Len rem 4)) rem 4); -vid(1, <>) -> - {Vid, Data}; -vid(0, Data) -> - {undefined, Data}. +split_data(_, _, _) -> + invalid_avp_length(). + +%% split_data/4 + +split_data(Bin, HdrLen, Len, Pad) -> + <<_:HdrLen/binary, T/bitstring>> = Bin, + case T of + <> -> + {Data, Rest}; + _ -> + invalid_avp_length() + end. + +%% invalid_avp_length/0 +%% +%% AVP Length doesn't mesh with payload. Induce a decode error by +%% returning a payload that no valid Diameter type can have. This is +%% so that a known AVP will result in 5014 error with a zero'd +%% payload. Here we simply don't know how to construct this payload. +%% (Yes, this solution is an afterthought.) + +invalid_avp_length() -> + {<<0:1>>, <<>>}. %%% --------------------------------------------------------------------------- %%% # pack_avp/1 @@ -474,20 +517,35 @@ vid(0, Data) -> pack_avp(#diameter_avp{data = [#diameter_avp{} | _] = Avps} = A) -> pack_avp(A#diameter_avp{data = encode_avps(Avps)}); -%% ... data as a type/value tuple, possibly with header data, ... +%% ... data as a type/value tuple ... pack_avp(#diameter_avp{data = {Type, Value}} = A) when is_atom(Type) -> pack_avp(A#diameter_avp{data = diameter_types:Type(encode, Value)}); + +%% ... with a header in various forms ... pack_avp(#diameter_avp{data = {{_,_,_} = T, {Type, Value}}}) -> pack_avp(T, iolist_to_binary(diameter_types:Type(encode, Value))); + pack_avp(#diameter_avp{data = {{_,_,_} = T, Bin}}) when is_binary(Bin) -> pack_avp(T, Bin); + pack_avp(#diameter_avp{data = {Dict, Name, Value}} = A) -> {Code, _Flags, Vid} = Hdr = Dict:avp_header(Name), {Name, Type} = Dict:avp_name(Code, Vid), pack_avp(A#diameter_avp{data = {Hdr, {Type, Value}}}); +pack_avp(#diameter_avp{code = undefined, data = Bin}) + when is_binary(Bin) -> + %% Reset the AVP Length of an AVP Header resulting from a 5014 + %% error. The RFC doesn't explicitly say to do this but the + %% receiver can't correctly extract this and following AVP's + %% without a correct length. On the downside, the header doesn't + %% reveal if the received header has been padded. + Pad = 8*header_length(Bin) - bit_size(Bin), + Len = size(<> = <>), + <>; + %% ... or as an iolist. pack_avp(#diameter_avp{code = Code, vendor_id = V, @@ -499,6 +557,11 @@ pack_avp(#diameter_avp{code = Code, {P, 2#00100000}]), pack_avp({Code, Flags, V}, iolist_to_binary(Data)). +header_length(<<_:32, 1:1, _/bitstring>>) -> + 12; +header_length(_) -> + 8. + flag_avp({true, B}, F) -> F bor B; flag_avp({false, _}, F) -> diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 1ba5cf0b3e..4733b67131 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -226,10 +226,10 @@ recv_R(false = No, _, _, _, _) -> %% transport has gone down collect_avps(Pkt) -> case diameter_codec:collect_avps(Pkt) of - {_Bs, As} -> - As; - As -> - As + {_Error, Avps} -> + Avps; + Avps -> + Avps end. %% recv_R/6 @@ -300,7 +300,7 @@ errors(_, #diameter_packet{header = #diameter_header{version = V}, %% AVP's definition. errors(_, #diameter_packet{errors = [Bs | Es]} = Pkt) - when is_bitstring(Bs) -> + when is_bitstring(Bs) -> %% from old code Pkt#diameter_packet{errors = [3009 | Es]}; %% DIAMETER_COMMAND_UNSUPPORTED 3001 @@ -595,7 +595,6 @@ reply([Msg], Dict, TPid, Dict0, Fs, ReqPkt) is_tuple(Msg) -> reply(Msg, Dict, TPid, Dict0, Fs, ReqPkt#diameter_packet{errors = []}); -%% No errors or a diameter_header/avp list. reply(Msg, Dict, TPid, Dict0, Fs, ReqPkt) -> Pkt = encode(Dict, reset(make_answer_packet(Msg, ReqPkt), Dict), Fs), incr(send, Pkt, Dict, TPid, Dict0), %% count outgoing result codes -- cgit v1.2.3