update draft: addressing IETF LC comments

1 files changed, 33 insertions, 26 deletions

diff --git a/doc/draft-ietf-codec-opus-update.xml b/doc/draft-ietf-codec-opus-update.xml
index 6e52a166..926c332f 100644
--- a/doc/draft-ietf-codec-opus-update.xml
+++ b/doc/draft-ietf-codec-opus-update.xml
@@ -10,7 +10,7 @@
 <?rfc inline="yes"?>
 <?rfc compact="yes"?>
 <?rfc subcompact="no"?>
-<rfc category="std" docName="draft-ietf-codec-opus-update-08"
+<rfc category="std" docName="draft-ietf-codec-opus-update-09"
      ipr="trust200902" updates="6716">
   <front>
     <title abbrev="Opus Update">Updates to the Opus Audio Codec</title>
@@ -47,11 +47,13 @@
 
 
 
-    <date day="26" month="July" year="2017" />
+    <date day="17" month="August" year="2017" />
 
     <abstract>
       <t>This document addresses minor issues that were found in the specification
-      of the Opus audio codec in RFC 6716.</t>
+      of the Opus audio codec in RFC 6716. It updates the nornative decoder implementation
+      included in the appendix of RFC 6716. The changes fixes real and potential security-related
+      issues, as well minor quality-related issues.</t>
     </abstract>
   </front>
 
@@ -79,7 +81,7 @@
     of the following line are not part of the patch. A properly formatted patch
     including all changes is available at
     <eref target="https://www.ietf.org/proceedings/98/slides/materials-98-codec-opus-update-00.patch"/>
-    and has a SHA1 029e3aa88fc342c91e67a21e7bfbc9458661cd5f.
+    and has a SHA-1 hash of 029e3aa88fc342c91e67a21e7bfbc9458661cd5f.
     </t>
 
     </section>
@@ -123,8 +125,9 @@
       <t>It was discovered that some invalid packets of very large size could trigger
       an out-of-bounds read in the Opus packet parsing code responsible for padding.
       This is due to an integer overflow if the signaled padding exceeds 2^31-1 bytes
-      (the actual packet may be smaller). The code can be fixed by applying the following
-      changes at line 596 of src/opus_decoder.c:
+      (the actual packet may be smaller). The code can be fixed by decrementing the
+      (signed) len value, instead of incrementing a separate padding counter.
+      This is done by applying the following changes at line 596 of src/opus_decoder.c:
     </t>
 <figure>
 <artwork><![CDATA[
@@ -162,7 +165,7 @@
     <t>The calls to memcpy() were using sizeof(opus_int32), but the type of the
         local buffer was opus_int16.</t>
     <t>Because the size was wrong, this potentially allowed the source
-        and destination regions of the memcpy() to overlap.
+        and destination regions of the memcpy() to overlap on the copy from "buf" to "buf".
           We believe that nSamplesIn (number of input samples) is at least fs_in_khZ (sampling rate in kHz),
           which is at least 8.
        Since RESAMPLER_ORDER_FIR_12 is only 8, that should not be a problem once
@@ -172,10 +175,9 @@
         (nSamplesIn&lt;&lt;1).</t>
       </list></t>
       <t>
-      The fact that the code never produced any error in testing (including when run under the
-      Valgrind memory debugger), suggests that in practice
-     the batch sizes are reasonable enough that none of the issues above
-     was ever a problem. However, the authors know of no obvious approach to proving that.
+      The allocated buffers involved (buf and S->sFIR) are actually larger than they need to be for
+      the batch size used, so no out-of-bounds read or write is possible. Therefore the bug cannot be
+      exploited.
     </t>
     <t>The code can be fixed by applying the following changes to line 78 of silk/resampler_private_IIR_FIR.c:
     </t>
@@ -243,8 +245,9 @@ RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
       <t>
         It was discovered through decoder fuzzing that some bitstreams could produce
         integer values exceeding 32-bits in LPC_inverse_pred_gain_QA(), causing
-        a wrap-around. Although the error is harmless in practice, the C standard considers
-        the behavior as undefined, so the following patch to line 87 of silk/LPC_inv_pred_gain.c
+        a wrap-around. Although the authors are not aware of any way to exploit the bug,
+        the C standard considers
+        the behavior as undefined. The following patch to line 87 of silk/LPC_inv_pred_gain.c
         detects values that do not fit in a 32-bit integer and considers the corresponding filters unstable:
       </t>
 <figure>
@@ -273,7 +276,7 @@ rc_mult2 ), mult2Q);
     <section title="Integer wrap-around in LSF decoding" anchor="lsf_overflow">
       <t>
         It was discovered -- also from decoder fuzzing -- that an integer wrap-around could
-        occur when decoding line spectral frequency coefficients from extreme bitstreams.
+        occur when decoding bitstreams with extremely large values for the high LSF parameters.
         The end result of the wrap-around is an illegal read access on the stack, which
         the authors do not believe is exploitable but should nonetheless be fixed. The following
         patch to line 137 of silk/NLSF_stabilize.c prevents the problem:
@@ -402,11 +405,11 @@ effective_lowband+N);
       optionally coding the two channels 180-degrees out of phase on a per-band basis.
       This provides better stereo quality than forcing the two channels to be in phase,
       but when the output is downmixed to mono, the energy in the affected bands is cancelled
-      sometimes resulting in audible artefacts.
+      sometimes resulting in audible artifacts.
       </t>
       <t>As a work-around for this issue, the decoder MAY choose not to apply the 180-degree
-      phase shift when the output is meant to be downmixed (inside or
-      outside of the decoder).
+      phase shift. This can be useful when downmixing to mono inside or
+      outside of the decoder (e.g. user-controllable).
       </t>
     </section>
 
@@ -421,14 +424,14 @@ effective_lowband+N);
         implementation is compliant as long as it passes either set of vectors.
       </t>
       <t>
-        In addition, any Opus implementation
-        that passes the original test vectors from <xref target="RFC6716">RFC 6716</xref>
-        is still compliant with the Opus specification. However, newer implementations
+        Any Opus implementation
+        that passes either the original test vectors from <xref target="RFC6716">RFC 6716</xref>
+        or one of the new sets of test vectors is compliant with the Opus specification. However, newer implementations
         SHOULD be based on the new test vectors rather than the old ones.
       </t>
       <t>The new test vectors are located at
         <eref target="https://www.ietf.org/proceedings/98/slides/materials-98-codec-opus-newvectors-00.tar.gz"/>.
-        The SHA1 hash of the test vectors are:
+        The SHA-1 hashes of the test vectors are:
 <figure>
 <artwork>
 <![CDATA[
@@ -481,13 +484,17 @@ fd3d3a7b0dfbdab98d37ed9aa04b659b9fefbd18  testvector11m.dec
         <eref target="https://nvd.nist.gov/vuln/detail/CVE-2013-0899"/>
         and CVE-2017-0381 <eref target="https://nvd.nist.gov/vuln/detail/CVE-2017-0381"/>.
         CVE-2013-0899 is fixed by <xref target="padding"/> and
-        could theoretically cause information leak, but the
+        could theoretically cause an 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 <xref target="lsf_overflow"/> and, 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.
+        at least 24 MB in size. CVE-2017-0381 is fixed by Section 7 and can only result in a 16-bit
+        out-of-bounds read to a fixed location 256 bytes before a constant
+        table. That location would normally be part of an executable's read-only
+        data segment, but if that is not the case, the bug could at worst
+        results in either a crash or the leakage of 16 bits of information from
+        that fixed memory location (if the attacker has access to the decoded
+        output). Despite the claims of the CVE, the bug cannot results in
+        arbitrary code execution.
         Beyond the two fixed CVEs, this document adds no new security considerations on top of
         <xref target="RFC6716">RFC 6716</xref>.
       </t>