Re: [secdir] Secdir review of draft-ietf-ippm-stamp-option-tlv-06

[Greg Mirsky <[e-mail]>]

Hi Charlie,
you might be interested to know that another case of using ICMP in our
document was pointed out in LC comments. That was used to report a failure
of HMAC authentication verification. The authors discussed it and have
agreed to use a new STAMP TLV Flag. You can find updates (also addressing
GenART IETF LC comments by Dan Romascanu) in the attached copies of the
working version of the draft and the diff to -06.
I greatly appreciate your consideration and feedback.

Regards,
Greg

On Mon, Jun 29, 2020 at 1:19 PM Greg Mirsky <[e-mail]> wrote:

> Hi Charlie,
> thank you for your thorough review, detailed questions, and
> thoughtful suggestions. We've discussed your comments and propose changes
> that are described below. Also, you can find the changes highlighted in the
> attached diff between the -06 and the new working version of the document.
> Please find my answers, notes in-line below under the GIM>> tag.
>
> Regards,
> Greg
>
> On Fri, Jun 26, 2020 at 10:34 PM Charlie Kaufman <
> [e-mail]> wrote:
>
>> I have reviewed this document as part of the security directorate's
>> ongoing effort to review all IETF documents being processed by the IESG.
>> These comments were written primarily for the benefit of the security area
>> directors.  Document editors and WG chairs should treat these comments just
>> like any other last call comments.
>>
>> This I-D specifies extensions to the STAMP protocol defined in RFC 8762.
>> The STAMP protocol runs over UDP and is similar to an "echo" protocol
>> intended to be used to test network speed and bandwidth. This I-D specifies
>> a series of TLV encoded extensions that can be appended to the end of the
>> UDP packets of the STAMP protocol (as well as defining the TLV mechanism
>> itself).
>>
>> I could not find anything objectionable from a security standpoint in
>> this extension mechanism. It is a bit odd that when extensions are added to
>> an HMAC protected packet, instead of adding the extensions before the HMAC
>> and having the HMAC protect the entire contents, they keep the existing
>> HMAC that protects the base packet and add a second HMAC at the end of the
>> extensions that protects only the extensions. This would potentially allow
>> an attacker to "mix and match" the extensions from one packet with the base
>> content of a different packet, but it's not clear what an attacker might be
>> able to gain by doing that.
>>
> GIM>> I think that the STAMP YANG data model could be used to minimize the
> possible attack vector you've described. We will certainly keep that as one
> of the objectives when updating draft-ietf-ippm-stamp-yang
> <https://datatracker.ietf.org/doc/draft-ietf-ippm-stamp-yang/>. Also,
> HMAC TLV might be required even in unauthenticated STAMP mode to protect
> the integrity on a certain TLV(s). Do you see this as a useful scenario or
> unnecessary feature?
>
>>
>> One of the pieces of information an extension can request is the 6 byte
>> MAC address of the incoming packet as seen by the peer (which will normally
>> by the MAC address of the last router on the path). That information is not
>> normally available to someone on a distant network, but it's not obvious
>> what harm could come from their knowing it.
>>
> GIM>> As mentioned above, HMAC TLV may follow some STAMP TLVs. Do you
> think that the document should recommend using HMAC TLV with the Location
> TLV?
>
>>
>> The protocol can also request the source and destination IP addresses as
>> seen by the peer (presumably to detect NAT gateways), but the protocol
>> assumes the originator will know whether the peer will see IPv4 or IPv6
>> addresses, so dealing with 6-to-4 gateways could be tricky.
>>
>> On page 19 table 2, this document asks IANA to assign extension type
>> codes. In all other documents of this sort that I have seen, the document
>> assigns the codes and asks IANA to track subsequent additions.
>>
> GIM>> Martin Duke, the Responsible AD for IPPM WG, asked this question in
> his review. He has agreed to leave the text as-is since no one asked for a
> particular number.
>
>>
>> On some kinds of errors, the protocol specifies that the error is
>> reported via and ICMP message. For others, it specifies that a field be
>> returned containing all zeros. It's probably better to report errors "in
>> band" because so many routers swallow ICMP messages. The spec does not -
>> and should - specify what to do if the length of the TLV coded extensions
>> does not match the length of the UDP packet containing them (or at least I
>> couldn't find where it was specified).
>>
> GIM>> Thank you for pointing out this awkward handling. We had continued
> discussion with Martin Duke and Rakesh Gandhi on improving the mechanism of
> reporting TLV errors. The discussion is in the updated Section 4 of the new
> working version and is copied below:
> 4.  TLV Extensions to STAMP
>
>    Type-Length-Value (TLV) encoding scheme provides a flexible extension
>    mechanism for optional informational elements.  TLV is an optional
>    field in the STAMP test packet.  Multiple TLVs MAY be placed in the
>    STAMP test packet.  A TLV MAY be enclosed in a TLV.  TLVs have the
>    two octets long Type field, two octets long Length field that is
>    equal to the length of the Value field in octets.  If a Type value
>    for TLV or sub-TLV is in the range for Vendor Private Use, the Length
>    MUST be at least 4, and the first four octets MUST be that vendor's
>    the Structure of Management Information (SMI) Private Enterprise
>    Codes, as recorded in IANA's SMI Private Enterprise Codes sub-
>    registry, in network octet order.  The rest of the Value field is
>    private to the vendor.  The following sections describe the use of
>    TLVs for STAMP that extends STAMP capability beyond its base
>    specification.
>
>        0                   1                   2                   3
>        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>       |U|L|R|R|R|R|R|R|     Type      |           Length              |
>       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>       ~                            Value                              ~
>       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>               Figure 5: TLV Format in a STAMP Extended Packet
>
>    where fields are defined as the following:
>
>    o  U - a one-bit flag, located as specified in Section 5.2.  A
>       Session-Sender MUST set the U flag to 0 before transmitting an
>       extended STAMP test packet.  A Session-Reflector MUST set the U
>       flag to 1 if the Session-Reflector has not understood the TLV.
>
>    o  L - a one-bit flag, located as specified in Section 5.2.  A
>       Session-Sender MUST set the L flag to 0 before transmitting an
>       extended STAMP test packet.  A Session-Reflector MUST set the L
>       flag to 1 if the Session-Reflector determined the TLV is
>       malformed, i.e., the Length field value of the fixed-size TLV is
>       not equal to the value defined for the particular type, or the
>       remaining length of the extended STAMP packet is less than the
>       size of the TLV.
>
>    o  R - reserved flags for future use.  These flags MUST be zeroed on
>       transmit and ignored on receipt.
>
>    o  Type - one-octet long field that characterizes the interpretation
>       of the Value field.  It is allocated by IANA, as specified in
>       Section 5.1
>
>    o  Length - two-octets long field equals length on the Value field in
>       octets.
>
>    o  Value - a variable-length field.  Its interpretation and encoding
>       determined by the value of the Type field.
>
>    A STAMP node, whether Session-Sender or Session-Reflector, receiving
>    a test packet MUST determine whether the packet is a base STAMP
>    packet or includes one or more TLVs.  The node MUST compare the value
>    in the Length field of the UDP header and the length of the base
>    STAMP test packet in the mode, unauthenticated or authenticated based
>    on the configuration of the particular STAMP test session.  If the
>    difference between the two values is larger than the length of UDP
>    header, then the test packet includes one or more STAMP TLVs that
>    immediately follow the base STAMP test packet.
>
>    A system that has received a STAMP test packet with extension TLVs
>    MUST validate each TLV:
>
>       if the U flag is set, the STAMP system MUST skip the processing of
>       the TLV.  The implementation MUST try to process the next TLV if
>       present in the extended STAMP packet.
>
>       if the L flag is set, the STAMP system MUST stop processing the
>       remainder of the extended STAMP packet.
>
>       if an implementation of a Session-Reflector does not recognize the
>       Type field value, it MUST include the copy of the TLV into the
>       reflected STAMP packet.  The Session-Reflector MUST set the U flag
>       to 1.  The Session-Reflector MUST try to process the next TLV in
>       the extended STAMP packet;
>
>       if a TLV is malformed, the processing of extension TLVs MUST be
>       stopped.  The Session-Reflector MUST copy the remainder of the
>       received extended STAMP packet into the reflected STAMP packet.
>       The Session-Reflector MUST set the L flag to 1.
>
>    Detected error events MUST be logged.  Note that rate of logging MUST
>    be controlled.
>
> Please note, that IANA is also requested to create a STAMP TLV Flags
> sub-registry.
> We hope this is an acceptable solution. Always welcome your questions,
> comments, and suggestions.
>
>
>> --Charlie
>>
>