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

[Stephen Farrell <[e-mail]>]

Hiya,

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

Cheers,
S.

On 17/02/16 02:11, Timothy B. Terriberry wrote:
> Thanks for the detailed comments. Responses are in-line below.
> 
> Stephen Farrell wrote:
>> - 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
> 
> 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
>> have helped.
> 
> 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>.
> 
> 
>> - 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:-)
> 
> 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.
> 
>> - 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.
> 
> 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.
> 
>> - 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?
> 
> 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.
> 
>> - 5.2.1: From what namespace are the -573 and 111 values here
>> 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.
> 
>> - 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
> 
> 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
>> sure.
> 
> 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.