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

From: "Barry Leiba" <[email-masked]>
Date: Wed, 17 Feb 2016 09:14:53 -0800

Barry Leiba 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:
----------------------------------------------------------------------

-- Section 3 --

   A demuxer SHOULD NOT attempt to decode the data for the
   first packet on a page with the 'continued packet' flag set if the
   previous page with packet data does not end in a continued packet
   (i.e., did not end with a lacing value of 255) or if the page
   sequence numbers are not consecutive, unless the demuxer has some
   special knowledge that would allow it to interpret this data despite
   the missing pieces.

This "SHOULD NOT" requirement is in a very long sentence with a few "ifs"
and "buts", which make it hard to see what's really being required. 
Rewording to put the conditions up front might help.  It'd also be good
to explain why it's SHOULD NOT, rather than, say, MUST NOT.  Under what
conditions might I want to decode the first packet anyway, and what are
the consequences of my doing that?  It looks like the "unless" clause is
there to explain that, in which case "MUST NOT ... unless" seems right.

For the rewording, maybe this?:

NEW
If a page has the "continued packet" flag set and one of the following
conditions is also true:
- the previous page with packet data does not end in a continued packet
(a lacing value of 255) OR
- the page sequence numbers are not consecutive,
then the demuxer SHOULD NOT attempt to decode the data for the first
packet on the page unless the demuxer has some special knowledge that
would allow it to interpret this data despite the missing pieces.
END

...and then consider whether it really should be "MUST NOT ... unless".

You should take similar action with the "SHOULD NOT" sentence in the next
paragraph as well, as it has the same convolution problem.

-- Section 5.2 --
In bullet 4, might it not be clearer to call it "User Comment List
Count", rather than "Length"?

In the list in general, I think it'd be clearer and more accurate to
group all the "MUST NOT indicate that there are so many..." sorts of
things into one one-sentence pargraph that says that the Vendor String
and all the User Comments, together, MUST fit into the packet.  No?

-- Section 9 --

   Malicious payloads MUST NOT cause an implementation to overrun its
   allocated memory or to take an excessive amount of resources to
   decode.
...
   Malicious audio input streams
   MUST NOT cause an implementation to overrun its allocated memory or
   consume excessive resources

It's a small thing, rather at the nit level, but it doesn't make much
sense to levy normative requirements on malicious actors.  These would be
much better if worded as requirements on the implementation, somewhat
like this:

NEW
Malicious payloads and/or input streams can be used to attack codec
implementations.  Implementations MUST NOT overrun their allocated memory
nor take an excessive amount of resources when decoding payloads or
processing input streams.
END

-- Section 11 --

   Modifications to this registry follow the "Specification
   Required with Expert Review" registration policy as defined in
   [RFC5226].

RFC 5226 does not define a policy with that name.  The name is just
"Specification Required"; please change this.  And thanks for the
paragraph giving guidance to the designated expert.

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Wed, 17 Feb 2016 18:28:05 -0800

Thanks for these comments. Responses in-line below.

> would allow it to interpret this data despite the missing pieces.

Applied (with minor edits).

> ...and then consider whether it really should be "MUST NOT ... unless".

I think you're right. Originally we had "SHOULD NOTs" for _muxers_ 
discouraging them from creating such streams, but we changed them to 
requirements specifying how _demuxers_ should handle them (because the 
burden of avoiding them in the live streaming case is such that we don't 
expect muxers to do it, and they have not historically in the past). But 
I can't think of a good reason (other than the provided exception) for a 
demuxer to do anything else.

> paragraph as well, as it has the same convolution problem.

Done.

> Count", rather than "Length"?

It would be, but I wanted consistency with the terminology in the Vorbis 
spec.

> and all the User Comments, together, MUST fit into the packet.  No?

The intention here was that each MUST NOT corresponded to a specific 
check an implementation would do. That makes it easy for someone writing 
a parser to say, "Do I have a check corresponding to this restriction?" 
and realize they are missing something if they do not (though they do 
not cover everything, see also the new security considerations text).

> processing input streams.

Applied (with additional revisions to the subsequent sentences).

> RFC 5226 does not define a policy with that name.  The name is just

Fixed.

> paragraph giving guidance to the designated expert.

You're welcome!

A diff of the changes against the XML source is attached.

>From 2c038009119a27fd7223500feadca7397cfe29ca Mon Sep 17 00:00:00 2001
From: "Timothy B. Terriberry" <[email-masked]>
Date: Wed, 17 Feb 2016 18:24:35 -0800
Subject: [PATCH] oggopus: Address Barry Leiba's IESG comments.

Thanks to Barry for proposing specific text for the changes.
---
 doc/draft-ietf-codec-oggopus.xml | 52 +++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/doc/draft-ietf-codec-oggopus.xml b/doc/draft-ietf-codec-oggopus.xml
index 1b4c131..61da699 100644
--- a/doc/draft-ietf-codec-oggopus.xml
+++ b/doc/draft-ietf-codec-oggopus.xml
@@ -243,40 +243,49 @@ The combination of coding mode, audio bandwidth, and frame size is referred to
 </t>
 <t>
 Packets are placed into Ogg pages in order until the end of stream.
 Audio data packets might span page boundaries.
 The first audio data page could have the 'continued packet' flag set
  (indicating the first audio data packet is continued from a previous page) if,
  for example, it was a live stream joined mid-broadcast, with the headers
  pasted on the front.
-A demuxer SHOULD NOT attempt to decode the data for the first packet on a page
- with the 'continued packet' flag set if the previous page with packet data
- does not end in a continued packet (i.e., did not end with a lacing value of
- 255) or if the page sequence numbers are not consecutive, unless the demuxer
- has some special knowledge that would allow it to interpret this data
- despite the missing pieces.
+If a page has the 'continued packet' flag set and one of the following
+ conditions is also true:
+<list style="symbols">
+<t>the previous page with packet data does not end in a continued packet (does
+ not end with a lacing value of 255) OR</t>
+<t>the page sequence numbers are not consecutive,</t>
+</list>
+ then a demuxer MUST NOT attempt to decode the data for the first packet on the
+ page unless the demuxer has some special knowledge that would allow it to
+ interpret this data despite the missing pieces.
 An implementation MUST treat a zero-octet audio data packet as if it were a
  malformed Opus packet as described in
  Section&nbsp;3.4 of&nbsp;<xref target="RFC6716"/>.
 </t>
 <t>
 A logical stream ends with a page with the 'end of stream' flag set, but
  implementations need to be prepared to deal with truncated streams that do not
  have a page marked 'end of stream'.
 There is no reason for the final packet on the last page to be a continued
  packet, i.e., for the final lacing value to be 255.
 However, demuxers might encounter such streams, possibly as the result of a
  transfer that did not complete or of corruption.
-A demuxer SHOULD NOT attempt to decode the data from a packet that continues
- onto a subsequent page (i.e., when the page ends with a lacing value of 255)
- if the next page with packet data does not have the 'continued packet' flag
- set or does not exist, or if the page sequence numbers are not consecutive,
- unless the demuxer has some special knowledge that would allow it to interpret
- this data despite the missing pieces.
+If a packet continues onto a subsequent page (i.e., when the page ends with a
+ lacing value of 255) and one of the following conditions is also true:
+<list style="symbols">
+<t>the next page with packet data does not have the 'continued packet' flag
+ set OR</t>
+<t>there is no next page with packet data OR</t>
+<t>the page sequence numbers are not consecutive,</t>
+</list>
+ then a demuxer MUST NOT attempt to decode the data from that packet unless the
+ demuxer has some special knowledge that would allow it to interpret this data
+ despite the missing pieces.
 There MUST NOT be any more pages in an Opus logical bitstream after a page
  marked 'end of stream'.
 </t>
 </section>
 
 <section anchor="granpos" title="Granule Position">
 <t>
 The granule position MUST be zero for the ID header page and the
@@ -1536,24 +1545,23 @@ A brief summary of major implementations of this draft is available
 </t>
 </section>
 
 <section anchor="security" title="Security Considerations">
 <t>
 Implementations of the Opus codec need to take appropriate security
  considerations into account, as outlined in <xref target="RFC4732"/>.
 This is just as much a problem for the container as it is for the codec itself.
-Robustness against malicious payloads is extremely important.
-Malicious payloads MUST NOT cause an implementation to overrun its allocated
- memory or to take an excessive amount of resources to decode.
-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.
+Malicious payloads and/or input streams can be used to attack codec
+ implementations.
+Implementations MUST NOT overrun their allocated memory nor consume excessive
+ resources when decoding payloads or processing input streams.
+Although problems in encoding applications are typically rarer, this still
+ applies to a muxer, as vulnerabilities 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,
@@ -1663,18 +1671,18 @@ This document updates the IANA Media Types registry to add .opus
 <t>
 This document defines a new registry "Opus Channel Mapping Families" to
  indicate how the semantic meanings of the channels in a multi-channel Opus
  stream are described.
 IANA is requested to create a new name space of "Opus Channel Mapping
  Families".
 This will be a new registry on the IANA Matrix, and not a subregistry of an
  existing registry.
-Modifications to this registry follow the "Specification Required with Expert
- Review" registration policy as defined in <xref target="RFC5226"/>.
+Modifications to this registry follow the "Specification Required" registration
+ policy as defined in <xref target="RFC5226"/>.
 Each registry entry consists of a Channel Mapping Family Number, which is
  specified in decimal in the range 0 to 255, inclusive, and a Reference (or
  list of references)
 Each Reference must point to sufficient documentation to describe what
  information is coded in the Opus identification header for this channel
  mapping family, how a demuxer determines the Stream Count ('N') and Coupled
  Stream Count ('M') from this information, and how it determines the proper
  interpretation of each of the decoded channels.
-- 
1.8.3.2