Skip to content
Snippets Groups Projects
Verified Commit d772eb96 authored by Jean-Marc Valin's avatar Jean-Marc Valin
Browse files

update draft: addressing IETF LC comments

parent 1f38af47
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
doc/draft-ietf-codec-opus-update.xml
+ 33
− 26
View file @ d772eb96
......@@ -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>
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment