Re: [secdir] Secdir review of draft-ietf-tsvwg-rlc-fec-scheme

[Vincent Roca <[e-mail]>]

Hello Alan,

Thanks a lot for your SECDIR review.

NB: I have added IESG (you forgot to do so) and TSVWG (to let everybody know) to the CC list of this answer.

> Le 18 sept. 2018 à 16:08, Alan DeKok <[e-mail]> a écrit :
> 
> 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.
> 
> The summary of the review is read with nits
> 
>  The document seems to clearly describe what it's doing and why. 
> 
>  One nit I have is the formulae in the document, e.g.:
> 
>   dw_max_size = (max_lat * br_out * cr) / (8 * E)
> 
>  My concern here is about numerical overflow, which could lead to improper calculations and security problems.
> 
>  Experience has shown that naive implementations will just do the calculations no matter what the inputs.  Garbage inputs then lead to garbage outputs, and we have problems.
> 
>  It would be good to:
> 
> * state the limits on each of the variables, so that implementations can know that values *outside* of these limits are erroneous, and should be ignored
> 
> * state the minimum size of each of the above variables.  e.g. "dw_max_size" is likely not going to be an 8-bit integer.  So what data types are sufficient to perform the calculations correctly?
> 
>  This information may be in another document, but it wasn't clear on cursory inspection.

This is a very good point. 

NEW in section 3.1.1:
   The ew_max_size is the main parameter at a FECFRAME sender.  It is
   RECOMMENDED to check that the ew_max_size value stays within
   reasonnable bounds in order to avoid hazardous behaviours.

and later (same section):
   It is RECOMMENDED to check that the ls_max_size value
   stays within reasonnable bounds in order to avoid hazardous
   behaviours.

NEW section:

8.5.  Additional Security Considerations for Numerical Computations

   In addition to the above security considerations, inherited from
   [RFC6363], the present document introduces several formulae, in
   particular in Section 3.1.1.  It is RECOMMENDED to check that the
   computed values stay within reasonnable bounds since numerical
   overflows, caused by an erroneous implementation or an erroneous
   input value, may lead to hazardous behaviours.  However what
   "reasonnable bounds" means is use-case and implementation dependent
   and is not detailed in this document.

   Section 3.1.2 also mentions the possibility of "using the timestamp
   field of an RTP packet header" when applicable.  A malicious attacker
   may deliberately corrupt this header field in order to trigger
   hazardous behaviours at a FECFRAME receiver.  Protection against this
   type of content corruption can be addressed with the above
   recommendations on a baseline secure operation.  In addition, it is
   also RECOMMENDED to check that the timestamp value be within
   reasonnable bounds.

> 3.1.2:
> 
>   Another approach consists in using ADU timing information (e.g.,
>   using the timestamp field of an RTP packet header
> 
>  What happens if the timestamp is outside of reasonable bounds?  There could be a recommendation that only timestamps within a certain window are used.
> 
>  e.g. an adversary sends timestamps which are unreasonable, and a naive implementation could use those, and engage in integer overflow, etc.
> 
>  Section 8 describes protections against many adversarial attacks, which is good.
> 
>  I would suggest adding a sub-section of Section 8, discussion computation / numerical issues, as discussed above.  

Good point.
In our implementation we do not rely on NTP timestamp but on the 2nd possibility mentioned
in the I-D:
   "Another approach consists in using ADU timing information (e.g., using the timestamp 
     field of an RTP packet header, or registering the time upon receiving a new ADU)."
Using the gettimeofday() (or a similar system call) relieves from this problem and avoids to
dig into packet headers to get the RTP timestamp. However we have added a sentence to
warn an implementer about this risk (see the 2nd paragraph on new section 8.5 above)


>  Other than that, the document looks fine.

Thanks a lot for these suggestions.
We’re preparing a new version that will also answer the Genart review from Russ H.

Cheers,

  Vincent and Belkacem