Subject: [codec] Stephen Farrell's No Objection on draft-ietf-codec-oggopus-13: (with COMMENT)

From: "Stephen Farrell" <[email-masked]>
Date: Mon, 15 Feb 2016 10:02:33 -0800

Stephen Farrell has entered the following ballot position for
draft-ietf-codec-oggopus-13: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-codec-oggopus/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

- general, (but a nit): there are some odd unexplained numbers
here, which is fine ... but odd. (E.g. 125,829,120) It might be
nicer to explain to the reader why these values were chosen.  I
assume there are unstated reasons that an implementer need not
know - if an implementer really should grok why one of these odd
numbers is used, then that would be better explained.

- An example or diagram in the intro or section 3 would maybe
have helped.

- section 5: Q7.8 is a new one to me. Maybe add a reference?

- 5.1.1.5: Could figures 3-8 do with some body text to explain
them? That's (having body text that refers to the figures),
general good practice and I'm not sure if they're sufficiently
clear for an implementer to get right. (But as I'm not coding
this, I don't know, so just checking:-)

- 5.2: You don't say that the user comment strings must also be
UTF8, but you do for the vendor string. Why not? I think it'd be
good to call that out.

- 5.2.1: [vorbis-comment] is, correctly, a normative reference
here but I wonder if a xiph.org URL is a good enough reference
for that. Is there anything better, or are we confident enough
in that URL? 

- 5.2.1: From what namespace are the -573 and 111 values here
selected? How is that managed? (Just wondering.)

- section 9: While I like the MUST NOT here, it's really only
wishful thinking and isn't a strictly valid use of 2119 terms.
I'm ok with that though. The SHOULD NOT statement is also kind
of bogus. Generally this section would be better if it had
guidance on where implementers are likely to go wrong in ways
that cause security issues.  Reference such as are provided in
RFC 6562 about the dangers of VBR might also be useful here, not
sure.

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Tue, 16 Feb 2016 18:11:12 -0800

Thanks for the detailed comments. Responses are in-line below.

> nicer to explain to the reader why these values were chosen.  I

The number quoted is just 120 MB (120*1024*1024). I added a 
parenthetical to clarify this. The 61,440 number in the same paragraph 
is explained later in the text. Happy to clarify any others you think 
should be explained better.

> - An example or diagram in the intro or section 3 would maybe

The appropriate figure for the introduction is probably the one from 
Section 5 of RFC 3533. We could copy it here, but I think the reference 
is enough (it would probably require more text to explain it, text which 
is already in RFC 3533).

Section 3 is a bit more specific to this draft. I added the following 
figure:

         Page 1         Pages 2 ... n        Pages (n+1) ...
      +------------+ +---+ +---+ ... +---+ +-----------+ +---------+ +--
      |            | |   | |   |     |   | |           | |         | |
      |+----------+| |+-----------------+| |+-------------------+ +-----
      |||ID Header|| ||  Comment Header || ||Audio Data Packet 1| | ...
      |+----------+| |+-----------------+| |+-------------------+ +-----
      |            | |   | |   |     |   | |           | |         | |
      +------------+ +---+ +---+ ... +---+ +-----------+ +---------+ +--
      ^      ^                           ^
      |      |                           |
      |      |                           Mandatory page break
      |      |
      |      ID header is contained on a single page
      |
      'Beggining Of Stream'

     Figure 1: Example packet organization for a logical Ogg Opus stream

> - section 5: Q7.8 is a new one to me. Maybe add a reference?

Added a reference to 
<https://en.wikipedia.org/w/index.php?title=Q_%28number_format%29&oldid=697252615>.

> this, I don't know, so just checking:-)

I think the meaning is relatively unambiguous, but certainly adding a 
reference to the figures instead of just saying "the following matrices" 
would be clearer, so I have done that.

> UTF8, but you do for the vendor string. Why not? I think it'd be

The text doesn't but the header says:

"User Comment #i String (variable length, UTF-8 vector):"

I've normalized the text to match that of the vendor string for 
consistency, however.

> for that. Is there anything better, or are we confident enough

According to archive.org, that URL has been stable since at least 2005 
(since the year 2000, according to our svn logs, but the final version 
for Vorbis 1.0 wasn't posted until July 11th 2002). I don't know of a 
better authority.

> selected? How is that managed? (Just wondering.)

Those values are just examples. I've added a sentence saying that, since 
it must not have been clear. The syntax for them is described in the 
paragraph below. If people feel that description is imprecise we could 
probably add some EBNF.

> that cause security issues.  Reference such as are provided in

I agree. The current text was adapted from RFC 6716, but I think we have 
enough implementation experience at this point to write something 
better. I propose the following additional text:

    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 Section 5.1.1, 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.

> RFC 6562 about the dangers of VBR might also be useful here, not

In most of the expected uses of Ogg Opus on the internet (e.g., https 
streaming), the encryption and network packetization would be separate 
from the container's packetization/pagination, so I don't think the 
dangers referred to by RFC 6562 apply (if you can see the Ogg packet 
sizes, it's because you already have access to the decrypted plaintext). 
In other contexts, RFC 6716 already includes a reference to 6562.

I've attached a diff of the relevant changes to the XML.
>From f26c35306410228f3870e136502620b4177b9112 Mon Sep 17 00:00:00 2001
From: "Timothy B. Terriberry" <[email-masked]>
Date: Tue, 16 Feb 2016 18:05:10 -0800
Subject: [PATCH] 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 e2b38aa..ff65ae6 100644
--- a/doc/draft-ietf-codec-oggopus.xml
+++ b/doc/draft-ietf-codec-oggopus.xml
@@ -159,18 +159,42 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD",
  "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this
  document are to be interpreted as described in <xref target="RFC2119"/>.
 </t>
 
 </section>
 
 <section anchor="packet_organization" title="Packet Organization">
 <t>
-An Ogg Opus stream is organized as follows.
+An Ogg Opus stream is organized as follows (see
+ <xref target="packet-org-example"/> for an example).
 </t>
+
+<figure anchor="packet-org-example"
+ title="Example packet organization for a logical Ogg Opus stream"
+ align="center">
+<artwork align="center"><![CDATA[
+    Page 1         Pages 2 ... n        Pages (n+1) ...
+ +------------+ +---+ +---+ ... +---+ +-----------+ +---------+ +--
+ |            | |   | |   |     |   | |           | |         | |
+ |+----------+| |+-----------------+| |+-------------------+ +-----
+ |||ID Header|| ||  Comment Header || ||Audio Data Packet 1| | ...
+ |+----------+| |+-----------------+| |+-------------------+ +-----
+ |            | |   | |   |     |   | |           | |         | |
+ +------------+ +---+ +---+ ... +---+ +-----------+ +---------+ +--
+ ^      ^                           ^
+ |      |                           |
+ |      |                           Mandatory Page Break
+ |      |
+ |      ID header is contained on a single page
+ |
+ 'Beggining Of Stream'
+]]></artwork>
+</figure>
+
 <t>
 There are two mandatory header packets.
 The first packet in the logical Ogg bitstream MUST contain the identification
  (ID) header, which uniquely identifies a stream as Opus audio.
 The format of this header is defined in <xref target="id_header"/>.
 It is placed alone (without any other packet data) on the first page of
  the logical Ogg bitstream, and completes on that page.
 This page has its 'beginning of stream' flag set.
@@ -734,17 +758,18 @@ Rates outside this range MAY be ignored by falling back to the default rate of
  48&nbsp;kHz instead.
 <vspace blankLines="1"/>
 </t>
 <t>Output Gain (16 bits, signed, little endian):
 <vspace blankLines="1"/>
 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&nbsp;<xref target="q-notation"/>).
 <vspace blankLines="1"/>
 To apply the gain, an implementation could use
 <figure align="center">
 <artwork align="center"><![CDATA[
 sample *= pow(10, output_gain/(20.0*256)) ,
 ]]></artwork>
 </figure>
  where output_gain is the raw 16-bit value from the header.
@@ -986,19 +1011,22 @@ A demuxer implementation encountering a reserved channel mapping family value
 An Ogg Opus player MUST support any valid channel mapping with a channel
  mapping family of 0 or 1, even if the number of channels does not match the
  physically connected audio hardware.
 Players SHOULD perform channel mixing to increase or reduce the number of
  channels as needed.
 </t>
 
 <t>
-Implementations MAY use the following matrices to implement downmixing from
- multichannel files using <xref target="channel_mapping_1">Channel Mapping
- Family 1</xref>, which are known to give acceptable results for stereo.
+Implementations MAY use the matrices in
+ Figures&nbsp;<xref target="downmix-matrix-3" format="counter"/>
+ through&nbsp;<xref target="downmix-matrix-8" format="counter"/> to implement
+ downmixing from multichannel files using
+ <xref target="channel_mapping_1">Channel Mapping Family 1</xref>, 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
  clipping and dynamic range reduction.
 </t>
 <t>
 In these matrices the front left and front right channels are generally
 passed through directly.
@@ -1205,17 +1233,18 @@ It MUST NOT indicate that there are so many comments that the comment string
 This field gives the length of the following user comment string, in octets.
 There is one for each user comment indicated by the 'user comment list length'
  field.
 It MUST NOT indicate that the string is longer than the rest of the packet.
 <vspace blankLines="1"/>
 </t>
 <t>User Comment #i String (variable length, UTF-8 vector):
 <vspace blankLines="1"/>
-This field contains a single user comment string.
+This field contains a single user comment encoded as a UTF-8
+ string&nbsp;<xref target="RFC3629"/>.
 There is one for each user comment indicated by the 'user comment list length'
  field.
 </t>
 </list>
 </t>
 
 <t>
 The vendor string length and user comment list length are REQUIRED, and
@@ -1241,19 +1270,19 @@ This allows informal experimentation with the format of this binary data until
 </t>
 
 <t>
 The comment header can be arbitrarily large and might be spread over a large
  number of Ogg pages.
 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&nbsp;octets, and MAY ignore individual
- comments that are not fully contained within the first 61,440&nbsp;octets of
- the comment header.
+ comment header larger than 125,829,120&nbsp;octets (120&nbsp;MB), and MAY
+ ignore individual comments that are not fully contained within the first
+ 61,440&nbsp;octets of the comment header.
 </t>
 
 <section anchor="comment_format" title="Tag Definitions">
 <t>
 The user comment strings follow the NAME=value format described by
  <xref target="vorbis-comment"/> with the same recommended tag names:
  ARTIST, TITLE, DATE, ALBUM, and so on.
 </t>
@@ -1282,16 +1311,17 @@ This tag is similar to the REPLAYGAIN_TRACK_GAIN tag in
 R128_ALBUM_GAIN=111
 ]]></artwork>
 </figure>
 <t>
  representing the volume shift needed to normalize the overall volume when
  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.
 </t>
 <t>
 An Ogg Opus stream MUST NOT have more than one of each of these tags, and if
  present their values MUST be an integer from -32768 to 32767, inclusive,
  represented in ASCII as a base 10 number with no whitespace.
 A leading '+' or '-' character is valid.
 Leading zeros are also permitted, but the value MUST be represented by
  no more than 6 characters.
@@ -1517,16 +1547,66 @@ Malicious payloads MUST NOT cause an implementation to overrun its allocated
 Although problems in encoding applications are typically rarer, the same
  applies to the muxer.
 Malicious audio input streams MUST NOT cause an implementation to overrun its
  allocated memory or consume excessive resources because this would allow an
  attacker to attack transcoding gateways.
 </t>
 
 <t>
+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
+ <xref target="channel_mapping"/>, in order to avoid attempting to read data
+ from channels that do not exist.
+</t>
+
+<t>
+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&nbsp;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&nbsp;octets, potentially requiring a separate
+ overflow check.
+</t>
+
+<t>
+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.
+</t>
+
+<t>
+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.
+</t>
+
+<t>
 Like most other container formats, Ogg Opus streams SHOULD NOT be used with
  insecure ciphers or cipher modes that are vulnerable to known-plaintext
  attacks.
 Elements such as the Ogg page capture pattern and the magic signatures in the
  ID header and the comment header all have easily predictable values, in
  addition to various elements of the codec data itself.
 </t>
 </section>
@@ -1712,16 +1792,24 @@ In&nbsp;<xref target="iana"/>, "RFCXXXX" is to be replaced with the RFC number
   <title>Autocorrelation LPC coeff generation algorithm
     (Vorbis source code)</title>
 <author initials="J." surname="Degener" fullname="Jutta Degener"/>
 <author initials="C." surname="Bormann" fullname="Carsten Bormann"/>
 <date month="November" year="1994"/>
 </front>
 </reference>
 
+<reference anchor="q-notation"
+ target="https://en.wikipedia.org/w/index.php?title=Q_%28number_format%29&amp;oldid=697252615">
+<front>
+<title>Q (number format)</title>
+<author><organization>Wikipedia</organization></author>
+<date month="December" year="2015"/>
+</front>
+</reference>
 
 <reference anchor="replay-gain"
  target="https://wiki.xiph.org/VorbisComment#Replay_Gain">
 <front>
 <title>VorbisComment: Replay Gain</title>
 <author initials="C." surname="Parker" fullname="Conrad Parker"/>
 <author initials="M." surname="Leese" fullname="Martin Leese"/>
 <date month="June" year="2009"/>
-- 
1.8.3.2

---

From: Stephen Farrell <[email-masked]>
Date: Wed, 17 Feb 2016 02:26:09 +0000

Hiya,

Thanks - I'm fine with all that and especially the new security
considerations text which seems much better,

Cheers,
S.