Re: [codec] AD Evaluation of draft-ietf-codec-opus-update-07

[Jean-Marc Valin <[e-mail]>]

Hi Ben,

Thanks for taking the time to review this document. I agree with all
your comments and I attached the XML diff for the changes I'm proposing
to address them. Let me know if you're happy with these changes and I'll
submit an updated version. See inline for more details.

On 25/07/17 06:34 PM, Ben Campbell wrote:
> —  I don’t think we can expect IETF LC and IESG reviewers to know the
> history of RFC 6716 going into this. Please expand the first
> paragraph in the introduction to  explain how the normative part of
> RFC 6716 is in the form of attached C code in appendix A, and that
> the patches in this draft patch that code.

This is the updated paragraph:

  This document addresses minor issues that were discovered in the
  reference implementation of the Opus codec.  Unlike most IETF
  specifications, Opus is defined in RFC 6716 [RFC6716] in terms of a
  normative reference decoder implementation rather than from the
  associated text description.  That's why only issues affecting the
  decoder are listed here.  An up-to-date implementation of the Opus
  encoder can be found at <https://opus-codec.org/>.

> — How can people be certain that the “formatted” patches match the
> patches in this draft, and do not change over time? Would it make
> sense to include a hash in this document, simularly to how 6716 has
> the hashes for the test vectors? (And are the IETF98 proceedings
> really where you want to store these?)

I added the hash of the patch and each testvector like in RFC6716. As
for putting the tar.gz in the proceedings, that's also what we did for
RFC6716 and as far as I know there is still no better solution for
publishing files at a fixed location.

> - 11: The draft should include hashes for the new test vectors,
> similarly to in 6716.

Done (see above).

> -12: The security considerations need some real content. For example,
> several of the fixes here seem to fix potential buffer overruns or
> other memory management errors. Does that reduce the attach surface
> for DoS or other attacks? I suspect the answer is “no”, since several
> of those sections mention that the bugs don’t appear to be
> exploitable. But the security considerations should at least
> summarize these sorts of changes and say why you believe they have no
> material impact on security.

I expanded the security considerations section to discuss the two issues
that have potential security implications and that had associated CVEs.
The new text is:

  This document fixes two security issues reported on Opus and that
  affect the reference implementation in RFC 6716 [RFC6716]: CVE-
  2013-0899 and CVE-2017-0381.  CVE-2013-0899 is fixed by Section 4 and
  could theoretically cause information leak, but the leaked
  information would at the very least go through the decoder process
  before being accessible to the attacker.  Also, the bug can only be
  triggered by Opus packets at least 24 MB in size.  CVE-2017-0381 is
  fixed by Section 7 as far as the authors are aware, could not be
  exploited in any way (despite the claims in the CVE) unless the read-
  only table was somehow placed very close to sensitive data, which is
  highly unlikely.  Beyond the two fixed CVEs, this document adds no
  new security considerations on top of RFC 6716 [RFC6716].

> -3, last paragraph: Is no “significant” impact on test vectors the
> same as no impact? Also, I note that none of the other patch sections
> talk about test vector impact, and the section on new test vectors
> explains which patches impact the vectors. Could the comment in this
> section just be removed?

Most of the changes do not affect test vectors at all, but I'm pointing
out the few that do have an impact. I agree that "no significant impact"
was a bit vague, so here's the updated text:

  This change affects the normative output of the decoder, but the
  amount of change is within the tolerance and too small to make the
  testvector check fail.

> -5, list items 2 and 3:  These paragraphs (and others in the draft)
> mention symbols from the code without any explanation what they mean.
> Some of the symbol names are self-documenting, but that is not
> consistently true. It would be helpful to add short comments about
> the meaning of each of these (perhaps in parentheses). One might
> think of this as similar to expanding acronyms on first use.

I added an explanation for nSamplesIn and fs_in_khZ. I thought the
remaining ones were clear enough, but let me know if I missed any that
would be useful.

> Also, the use of underscores for emphasis is, to my knowledge, not
> meaningful in the context of an RFC. Is the emphasis really needed?
> (I assume you don’t want to wait for the new RFC format for this sort
> of thing :-)  )

Removed the underscores.

> — paragraph after the list: "However, proving that is non- obvious.”
> - Odd sentence structure. Would it make sense to say “However, the
> authors know of no obvious approach to proving that.”?

Changed to the sentence you suggested.

> -8: Please expand NaN on first mention. 

Done (NaN = "not a number").

> -9: Please expand LCG on first mention

Simply replaced "LCG noise" by "white noise" (the fact that it's
generated by an LCG is irrelevant to the document).


Cheers,

	Jean-Marc


> 
> 
> 
> 
> _______________________________________________ codec mailing list 
> [e-mail] https://www.ietf.org/mailman/listinfo/codec
>