Re: [secdir] Review of draft-trammell-ipfix-tcpcontrolbits-revision-04

[Brian Trammell <[e-mail]>]

Hi, Simon, all,

Thanks for the review! I'm applying comments to a new -05 revision.
Comments thereon inline.

Simon Josefsson wrote:
> Hi.
> 
> 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.
> 
> Summary: This is an informational document fixing what appears as a bug
> in the IPFIX IANA registry that was introduced in RFC 5102.  The error
> was that RFC 5102 used the wrong size for TCP header field, and did not
> include three TCP header flags defined after RFC793 (but long before
> RFC 5102).  It also changes the semantics to be future proof in case
> new TCP header flags are used.
> 
> From a security review point of view, I see no significant issues with
> this document.
> 
> While reading the document, I noticed a couple of things.
> 
> MAJOR:
> 
> * The document says that processes that can't observe the ECN Nonce
>   Sum (NS) bit or Future Use bits "should" export the 8-byte value as
>   the old 8-bit value.  I believe RFC 2119 terms is warranted here, as
>   it appears to influence bytes-on-the-wire.  Is this SHOULD or MUST?

This would be a SHOULD. One, the ADT is unsigned16, so we'll almost
certainly get EPs that will ignore a MUST here and just export 16 bits
always even if they only support the bottom 6. Two, there's already
guidance on handling the ambiguity in the next paragraph.

This was a should instead of a SHOULD because the text in this section
goes into the IANA IPFIX Elements Registry, and (perhaps this is a bit
pedantic) the registry does not contain a reference to 2119 to explain
the meaning of the terms. However, on review, I see there is already
2119 language there, so SHOULD it is.

> MINOR:
> 
> * If the above SHOULD/MUST is honored, some information will be
>   potentially be lost.  Consider a process that can observe the CWR/ECE
>   bits but not the NS or Future Use bits.  That process would export
>   the tcpControlBits as a 8-bit value.  However, earlier specifications
>   said that CWR/ECE bits must be zero even if observed. This means that
>   instead of being able to trust the CWR/ECE bit values, those fields
>   are unreliable.  If instead a 16-bit value is always sent, the CWR/ECE
>   bits will be reliable.

This is a good point. But given the inconsistent interpretation of this
field by exporting process implementations, we'll have to deal with the
ambiguity here anyway.

>   However, it may be that this aspect is entirely theoretical, and that
>   all implementations that support CWR/ECE also support NS or Future Use
>   bits.

Nope. At this point I know of at least two implementations that ignore
the current CWR/ECE must be zero and export them anyway (if one is lazy,
it's a lot easier just to save the whole byte than to worry about which
end of it to mask). This revision is in part an attempt to make the
registry match this reality.

> * The document says "Octets 12 and 13" throughout, however if I'm
>   counting the TCP header octets properly, it should be "Octets 13 and
>   14".  I call the first octet octet 1 rather than 0, maybe that was
>   the origin of the +-1 issue.  Whenever you start counting on 0 rather
>   than 1, I think that should be spelled out; it might make things more
>   clear to spell that out regardless.

I've never seen anyone count bits or octets from 1. But I'm happy to add
a (counting from zero) note here to make that explicit.

> * The acronym IE is not spelled out.

Oops. We get sick of typing/saying Information Element all the time in
IPFIX. Sometimes I forget to search/replace back. Thanks!

>   OLD:
>   length), the corresponding bits in this IE must be exported as
>   NEW:
>   length), the corresponding bits in this Information Element (IE) must
>   be exported as
> 
> * The Security Considerations section could mention that this document
>   changes the size of the tcpControlBits IE from unsigned8 to
>   unsigned16, which is probably about the only thing you would want to
>   consider when studying the security aspect of this document.

Yep. Done.

> * In section 2 there is one sentence:
> 
>       The values of each bit are shown below, per the definition of the
>       bits in the TCP header [RFC0793]:
> 
>   That isn't really correct, since what is shown below the text above is
>   not consistent with RFC793 alone.  I would instead say:
> 
>       The values of each bit are shown below, per the definition of the
>       bits in the TCP header [RFC0793][RFC3168][RFC3540]:

Yep, fixed.

> * The IANA-IPFIX reference looks bad in section 6.2.

xml2rfc's reference handling isn't really designed for corporate
authorship and undated documents. I'll hack the differences out of the
resulting txt.

Thanks again, best regards,

Brian