From 3fe744ea04fdcc418fb85c2c133d13372ebb019b Mon Sep 17 00:00:00 2001 From: Jean-Marc Valin Date: Tue, 25 Jul 2017 23:09:14 -0400 Subject: [PATCH] Update draft: addressing AD comments --- doc/draft-ietf-codec-opus-update.xml | 93 +++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 16 deletions(-) diff --git a/doc/draft-ietf-codec-opus-update.xml b/doc/draft-ietf-codec-opus-update.xml index ad0d5697..6e52a166 100644 --- a/doc/draft-ietf-codec-opus-update.xml +++ b/doc/draft-ietf-codec-opus-update.xml @@ -10,7 +10,7 @@ - Updates to the Opus Audio Codec @@ -47,7 +47,7 @@ - + This document addresses minor issues that were found in the specification @@ -58,8 +58,11 @@
This document addresses minor issues that were discovered in the reference - implementation of the Opus codec that serves as the specification in - RFC 6716. Only issues affecting the decoder are + implementation of the Opus codec. Unlike most IETF specifications, Opus is defined + in RFC 6716 in terms of a normative reference + decoder implementation rather than from the associated text description. + That RFC includes the reference decoder implementation as Appendix A. + That's why only issues affecting the decoder are listed here. An up-to-date implementation of the Opus encoder can be found at . @@ -75,7 +78,8 @@ at the end of a line and the white space at the beginning of the following line are not part of the patch. A properly formatted patch including all changes is available at - . + + and has a SHA1 029e3aa88fc342c91e67a21e7bfbc9458661cd5f.
@@ -110,8 +114,8 @@ ]]> - This change affects the normative part of the decoder, although the - amount of change is too small to make a significant impact on testvectors. + 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. @@ -146,7 +150,7 @@ This packet parsing issue is limited to reading memory up to about 60 kB beyond the compressed buffer. This can only be triggered by a compressed packet more than about 16 MB long, so it's not a problem - for RTP. In theory, it could crash a file + for RTP. In theory, it could crash a file decoder (e.g. Opus in Ogg) if the memory just after the incoming packet is out-of-range, but our attempts to trigger such a crash in a production application built using an affected version of the Opus decoder failed. @@ -159,19 +163,19 @@ local buffer was opus_int16. Because the size was wrong, this potentially allowed the source and destination regions of the memcpy() to overlap. - We believe that nSamplesIn is at least fs_in_khZ, + 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 the type size is fixed. The size of the buffer used RESAMPLER_MAX_BATCH_SIZE_IN, but the - data stored in it was actually _twice_ the input batch size + data stored in it was actually twice the input batch size (nSamplesIn<<1). 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, proving that is non-obvious. + was ever a problem. However, the authors know of no obvious approach to proving that. The code can be fixed by applying the following changes to line 78 of silk/resampler_private_IIR_FIR.c: @@ -266,7 +270,7 @@ rc_mult2 ), mult2Q); -
+
It was discovered -- also from decoder fuzzing -- that an integer wrap-around could occur when decoding line spectral frequency coefficients from extreme bitstreams. @@ -294,7 +298,7 @@ silk_ADD_SAT16( NLSF_Q15[i-1], NDeltaMin_Q15[i] ) );
On extreme bit-streams, it is possible for log-domain band energy levels to exceed the maximum single-precision floating point value once converted - to a linear scale. This would later cause the decoded values to be NaN, + to a linear scale. This would later cause the decoded values to be NaN (not a number), possibly causing problems in the software using the PCM values. This can be avoided with the following patch to line 552 of celt/quant_bands.c: @@ -318,7 +322,7 @@ silk_ADD_SAT16( NLSF_Q15[i-1], NDeltaMin_Q15[i] ) ); enough bits to code a single CELT band (8 - 9.6 kHz). When that happens, the second band (CELT band 18, from 9.6 to 12 kHz) cannot use folding because it is wider than the amount already coded, and falls back to - LCG noise. Because it can also happen on transients (e.g. stops), it + white noise. Because it can also happen on transients (e.g. stops), it can cause audible pre-echo. @@ -424,11 +428,67 @@ effective_lowband+N); The new test vectors are located at . + The SHA1 hash of the test vectors are: +
+ + + +
+ Note that the decoder input bitstream files (.bit) are unchanged.
- This document adds no new security considerations on top of + This document fixes two security issues reported on Opus and that affect the + reference implementation in RFC 6716: CVE-2013-0899 + + and CVE-2017-0381 . + CVE-2013-0899 is fixed by 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 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. + Beyond the two fixed CVEs, this document adds no new security considerations on top of RFC 6716.
@@ -442,7 +502,8 @@ effective_lowband+N);
We would like to thank Juri Aedla for reporting the issue with the parsing of - the Opus padding. Also, thanks to Jonathan Lennox and Mark Harris for their + the Opus padding. Thanks to Felicia Lim for reporting the LSF integer overflow issue. + Also, thanks to Tina le Grand, Jonathan Lennox, and Mark Harris for their feedback on this document.