From f26c35306410228f3870e136502620b4177b9112 Mon Sep 17 00:00:00 2001 From: "Timothy B. Terriberry" Date: Tue, 16 Feb 2016 18:05:10 -0800 Subject: oggopus: Address Stephen Farrell's IESG comments. - Clarify that 125,829,120 is just 120 MB. - Add a figure to Section 3 of an example logical stream. - Add a reference for Q notation. - Refer to the downmixing figures in the text. - Clarify that user comments are UTF-8. - Clarify that the -573 and 111 gain values are examples. - Add specific advice to implementors on areas that have security implications. --- doc/draft-ietf-codec-oggopus.xml | 106 +++++++++++++++++++++++++++++++++++---- 1 file changed, 97 insertions(+), 9 deletions(-) diff --git a/doc/draft-ietf-codec-oggopus.xml b/doc/draft-ietf-codec-oggopus.xml index e2b38aae..ff65ae65 100644 --- a/doc/draft-ietf-codec-oggopus.xml +++ b/doc/draft-ietf-codec-oggopus.xml @@ -164,8 +164,32 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD",
-An Ogg Opus stream is organized as follows. +An Ogg Opus stream is organized as follows (see + for an example). + +
+ +
+ There are two mandatory header packets. The first packet in the logical Ogg bitstream MUST contain the identification @@ -739,7 +763,8 @@ Rates outside this range MAY be ignored by falling back to the default rate of This is a gain to be applied when decoding. It is 20*log10 of the factor by which to scale the decoder output to achieve the desired playback volume, stored in a 16-bit, signed, two's complement - fixed-point value with 8 fractional bits (i.e., Q7.8). + fixed-point value with 8 fractional bits (i.e., + Q7.8 ). To apply the gain, an implementation could use
@@ -991,9 +1016,12 @@ Players SHOULD perform channel mixing to increase or reduce the number of -Implementations MAY use the following matrices to implement downmixing from - multichannel files using Channel Mapping - Family 1, which are known to give acceptable results for stereo. +Implementations MAY use the matrices in + Figures  + through  to implement + downmixing from multichannel files using + Channel Mapping Family 1, which are + known to give acceptable results for stereo. Matrices for 3 and 4 channels are normalized so each coefficient row sums to 1 to avoid clipping. For 5 or more channels they are normalized to 2 as a compromise between @@ -1210,7 +1238,8 @@ It MUST NOT indicate that the string is longer than the rest of the packet. User Comment #i String (variable length, UTF-8 vector): -This field contains a single user comment string. +This field contains a single user comment encoded as a UTF-8 + string . There is one for each user comment indicated by the 'user comment list length' field. @@ -1246,9 +1275,9 @@ The comment header can be arbitrarily large and might be spread over a large Implementations MUST avoid attempting to allocate excessive amounts of memory when presented with a very large comment header. To accomplish this, implementations MAY treat a stream as invalid if it has a - comment header larger than 125,829,120 octets, and MAY ignore individual - comments that are not fully contained within the first 61,440 octets of - the comment header. + comment header larger than 125,829,120 octets (120 MB), and MAY + ignore individual comments that are not fully contained within the first + 61,440 octets of the comment header.
@@ -1287,6 +1316,7 @@ R128_ALBUM_GAIN=111 played as part of a particular collection of tracks. The gain is also a Q7.8 fixed point number in dB, as in the ID header's 'output gain' field. +The values '-573' and '111' given here are just examples. An Ogg Opus stream MUST NOT have more than one of each of these tags, and if @@ -1521,6 +1551,56 @@ Malicious audio input streams MUST NOT cause an implementation to overrun its attacker to attack transcoding gateways. + +Header parsing code contains the most likely area for potential overruns. +It is important for implementations to ensure their buffers contain enough + data for all of the required fields before attempting to read it (for example, + for all of the channel map data in the ID header). +Implementations would do well to validate the indices of the channel map, also, + to ensure they meet all of the restrictions outlined in + , in order to avoid attempting to read data + from channels that do not exist. + + + +To avoid excessive resource usage, we advise implementations to be especially + wary of streams that might cause them to process far more data than was + actually transmitted. +For example, a relatively small comment header may contain values for the + string lengths or user comment list length that imply that it is many + gigabytes in size. +Even computing the size of the required buffer could overflow a 32-bit integer, + and actually attempting to allocate such a buffer before verifying it would be + a reasonable size is a bad idea. +After reading the user comment list length, implementations might wish to + verify that the header contains at least the minimum amount of data for that + many comments (4 additional octets per comment, to indicate each has a + length of zero) before proceeding any further, again taking care to avoid + overflow in these calculations. +If allocating an array of pointers to point at these strings, the size of the + pointers may be larger than 4 octets, potentially requiring a separate + overflow check. + + + +Another bug in this class we have observed more than once involves the handling + of invalid data at the end of a stream. +Often, implementations will seek to the end of a stream to locate the last + timestamp in order to compute its total duration. +If they do not find a valid capture pattern and Ogg page from the desired + logical stream, they will back up and try again. +If care is not taken to avoid re-scanning data that was already scanned, this + search can quickly devolve into something with a complexity that is quadratic + in the amount of invalid data. + + + +In general when seeking, implementations will wish to be cautious about the + effects of invalid granule position values, and ensure all algorithms will + continue to make progress and eventually terminate, even if these are missing + or out-of-order. + + Like most other container formats, Ogg Opus streams SHOULD NOT be used with insecure ciphers or cipher modes that are vulnerable to known-plaintext @@ -1717,6 +1797,14 @@ In , "RFCXXXX" is to be replaced with the RFC number + + +Q (number format) +Wikipedia + + + -- cgit v1.2.1