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.

4 files changed, 153 insertions, 87 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index 03aa557c2e..1eb1a8e288 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -227,23 +227,20 @@ decode(Name, #diameter_avp{code = Code, vendor_id = Vid} = Avp, Acc) ->
 
 %% decode/4
 
-%% Don't know this AVP: see if it can be packed in an 'AVP' field
-%% undecoded, unless it's mandatory. Need to give Failed-AVP special
-%% treatment since it'll contain any unrecognized mandatory AVP's.
-decode(Name, 'AVP', #diameter_avp{is_mandatory = M} = Avp, {Avps, Acc}) ->
-    {[Avp | Avps], if M, Name /= 'Failed-AVP' ->
-                           unknown(Avp, Acc);
-                      true ->
-                           pack_AVP(Name, Avp, Acc)
-                   end};
-%% Note that the type field is 'undefined' in this case.
-
-%% Or try to decode.
 decode(Name, {AvpName, Type}, Avp, Acc) ->
-    d(Name, Avp#diameter_avp{name = AvpName, type = Type}, Acc).
+    d(Name, Avp#diameter_avp{name = AvpName, type = Type}, Acc);
+
+decode(Name, 'AVP', Avp, Acc) ->
+    decode_AVP(Name, Avp, Acc).
 
 %% d/3
 
+%% Don't try to decode the value of a Failed-AVP component since it
+%% probably won't.
+d('Failed-AVP' = Name, Avp, Acc) ->
+    decode_AVP(Name, Avp, Acc);
+
+%% Or try to decode.
 d(Name, Avp, {Avps, Acc}) ->
     #diameter_avp{name = AvpName,
                   data = Data}
@@ -265,17 +262,32 @@ d(Name, Avp, {Avps, Acc}) ->
                              ?LINE,
                              {Reason, Avp, erlang:get_stacktrace()}),
             {Rec, Failed} = Acc,
-            {[Avp|Avps], {Rec, [{rc(Reason), Avp} | Failed]}}
+            {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}}
     end.
 
+%% decode_AVP/3
+%%
+%% Don't know this AVP: see if it can be packed in an 'AVP' field
+%% undecoded, unless it's mandatory. Need to give Failed-AVP special
+%% treatment since it'll contain any unrecognized mandatory AVP's.
+%% Note that the type field is 'undefined' in this case.
+
+decode_AVP(Name, #diameter_avp{is_mandatory = M} = Avp, {Avps, Acc}) ->
+    {[Avp | Avps], if Name == 'Failed-AVP';
+                      not M ->
+                           pack_AVP(Name, Avp, Acc);
+                      true ->
+                           unknown(Avp, Acc)
+                   end}.
+
 %% rc/1
 
 %% diameter_types will raise an error of this form to communicate
 %% DIAMETER_INVALID_AVP_LENGTH (5014). A module specified to a
 %% @custom_types tag in a spec file can also raise an error of this
 %% form.
-rc({'DIAMETER', RC, _}) ->
-    RC;
+rc({'DIAMETER', 5014 = RC, _}, #diameter_avp{name = AvpName} = Avp) ->
+    {RC, Avp#diameter_avp{data = empty_value(AvpName)}};
 
 %% 3588:
 %%
@@ -283,8 +295,8 @@ rc({'DIAMETER', RC, _}) ->
 %%      The request contained an AVP with an invalid value in its data
 %%      portion.  A Diameter message indicating this error MUST include
 %%      the offending AVPs within a Failed-AVP AVP.
-rc(_) ->
-    5004.
+rc(_, Avp) ->
+    {5004, Avp}.
 
 %% ungroup/2
 %%
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),
 
-    <<Code:32, Flags:1/binary, Length:24, Rest/bitstring>>
-        = 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:32, 1:1, M:1, P:1, _:5, Len:24, V:32, _/bitstring>>) ->
+    {Code, V, M, P, Len, 12};
 
-    DataSize + PadSize =< size(Rest)
-        orelse ?THROW(truncated_data),
+split_head(<<Code:32, 0:1, M:1, P:1, _:5, Len:24, _/bitstring>>) ->
+    {Code, undefined, M, P, Len, 8};
 
-    <<Data:DataSize/binary, _:PadSize/binary, R/bitstring>>
-        = Rest,
-    <<Vbit:1, Mbit:1, Pbit:1, _Reserved:5>>
-        = 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:32, Data/bitstring>>) ->
-    {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:Len/binary, _:Pad/binary, Rest/bitstring>> ->
+            {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(<<H:5/binary, _:24, T/binary>> = <<Bin/bitstring, 0:Pad>>),
+    <<H/binary, Len:24, T/binary>>;
+
 %% ... 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
diff --git a/lib/diameter/test/diameter_3xxx_SUITE.erl b/lib/diameter/test/diameter_3xxx_SUITE.erl
index 89c78d8b57..0ec0d5020f 100644
--- a/lib/diameter/test/diameter_3xxx_SUITE.erl
+++ b/lib/diameter/test/diameter_3xxx_SUITE.erl
@@ -40,7 +40,7 @@
          send_unknown_application/1,
          send_unknown_command/1,
          send_ok/1,
-         send_invalid_avp_bits/1,
+         send_invalid_hdr_bits/1,
          send_missing_avp/1,
          send_ignore_missing_avp/1,
          send_double_error/1,
@@ -136,7 +136,7 @@ tc() ->
     [send_unknown_application,
      send_unknown_command,
      send_ok,
-     send_invalid_avp_bits,
+     send_invalid_hdr_bits,
      send_missing_avp,
      send_ignore_missing_avp,
      send_double_error,
@@ -216,27 +216,26 @@ send_ok([_,_]) ->
 send_ok(Config) ->
     send_ok(?group(Config)).
 
-%% send_invalid_avp_bits/1
+%% send_invalid_hdr_bits/1
 %%
-%% Send a request with an incorrect length on the optional
-%% Origin-State-Id that a callback ignores.
+%% Send a request with an incorrect E-bit that a callback ignores.
 
 %% Callback answers.
-send_invalid_avp_bits([callback, _]) ->
+send_invalid_hdr_bits([callback, _]) ->
     #diameter_base_STA{'Result-Code' = 2001,  %% SUCCESS
                        'Failed-AVP' = [],
                        'AVP' = []}
         = call();
 
 %% diameter answers.
-send_invalid_avp_bits([_,_]) ->
-    #'diameter_base_answer-message'{'Result-Code' = 3009, %% INVALID_AVP_BITS
+send_invalid_hdr_bits([_,_]) ->
+    #'diameter_base_answer-message'{'Result-Code' = 3008, %% INVALID_HDR_BITS
                                     'Failed-AVP' = [],
                                     'AVP' = []}
         = call();
 
-send_invalid_avp_bits(Config) ->
-    send_invalid_avp_bits(?group(Config)).
+send_invalid_hdr_bits(Config) ->
+    send_invalid_hdr_bits(?group(Config)).
 
 %% send_missing_avp/1
 %%
@@ -282,8 +281,7 @@ send_ignore_missing_avp(Config) ->
 
 %% send_double_error/1
 %%
-%% Send a request with both an incorrect length on the optional
-%% Origin-State-Id and a missing AVP.
+%% Send a request with both an invalid E-bit and a missing AVP.
 
 %% Callback answers with STA.
 send_double_error([callback, _]) ->
@@ -294,8 +292,8 @@ send_double_error([callback, _]) ->
 
 %% diameter answers with answer-message.
 send_double_error([_,_]) ->
-    #'diameter_base_answer-message'{'Result-Code' = 3009, %% INVALID_AVP_BITS
-                                    'Failed-AVP' = [_],
+    #'diameter_base_answer-message'{'Result-Code' = 3008, %% INVALID_HDR_BITS
+                                    'Failed-AVP' = [],
                                     'AVP' = []}
         = call();
 
@@ -392,20 +390,16 @@ prepare(Pkt, Caps, T)
        T == send_5xxx ->
     sta(Pkt, Caps);
 
-prepare(Pkt0, Caps, send_invalid_avp_bits) ->
-    Req0 = sta(Pkt0, Caps),
-    %% Append an Origin-State-Id with an incorrect AVP Length in order
-    %% to force 3009.
-    Req = Req0#diameter_base_STR{'Origin-State-Id' = [7]},
-    #diameter_packet{bin = Bin}
+prepare(Pkt0, Caps, send_invalid_hdr_bits) ->
+    Req = sta(Pkt0, Caps),
+    %% Set the E-bit to force 3008.
+    #diameter_packet{bin = <<H:34, 0:1, T/bitstring>>}
         = Pkt
         = diameter_codec:encode(?DICT, Pkt0#diameter_packet{msg = Req}),
-    Offset = size(Bin) - 12 + 5,
-    <<H:Offset/binary, Len:24, T/binary>> = Bin,
-    Pkt#diameter_packet{bin = <<H/binary, (Len + 2):24, T/binary>>};
+    Pkt#diameter_packet{bin = <<H:34, 1:1, T/bitstring>>};
 
 prepare(Pkt0, Caps, send_double_error) ->
-    dehost(prepare(Pkt0, Caps, send_invalid_avp_bits));
+    dehost(prepare(Pkt0, Caps, send_invalid_hdr_bits));
 
 prepare(Pkt, Caps, T)
   when T == send_missing_avp;
@@ -480,9 +474,7 @@ request(send_3xxx, _Req, _Caps) ->
 request(send_5xxx, _Req, _Caps) ->
     {answer_message, 5999};
 
-request(send_invalid_avp_bits, Req, Caps) ->
-    #diameter_base_STR{'Origin-State-Id' = []}
-        = Req,
+request(send_invalid_hdr_bits, Req, Caps) ->
     %% Default errors field but a non-answer-message and only 3xxx
     %% errors detected means diameter sets neither Result-Code nor
     %% Failed-AVP.