Thanks for the quick response. A few minor comments inline. I removed sections that do not seem to need further discussion.
Otherwise the proposed update looks good. Please feel free to submit a new revision after resolving the comments below to your own satisfaction. Thanks! Ben. > On Jul 25, 2017, at 11:09 PM, Jean-Marc Valin <[email protected]> wrote: > > 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/>. I suggest adding a sentence to the effect of the following after “… associated text description.”: "That RFC includes the reference decoder implementation as Appendix A." >> -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. Thanks, this is exactly the sort of text I had in mind. I only have a couple of nit comments, below: > 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 Is there a missing word? It’s not clear if you mean to say that as far as the authors are aware it is fixed, or as far as the authors are aware it could not be exploited. > 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]. Can you add some context about the CVEs, such as where they are reported and where they can be found? […] >> -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. I think that’s fine. > >> 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. So, as I looked at the XML diff, I realize the emphasis is added using XML tags rather than by hand entering the underscores. So I may have been incorrect to say they have no meaning in the context of an RFC :-) I think the text is still better without them, but do not have strong feelings if you prefer to keep them. […] _______________________________________________ codec mailing list [email protected] https://www.ietf.org/mailman/listinfo/codec
