Updates to the Opus Audio Codec
draft-ietf-codec-opus-update-10

I don't have much to add, other than agreeing with Mirja's (and others') observation that this whole situation is a bit weird, and that we need a better system for things like reference implementations, and bug-fixes to same. 
Benoit, Richard and Joe's https://datatracker.ietf.org/doc/draft-claise-semver/ is a start, but this document reiterates the need.

Also, Spencer's review mean that I now need a new keyboard, mine's full of coffee...

Document: draft-ietf-codec-opus-update-08.txt

These patches are kind of hard to understand and evaluate without
context. For instance, consider the following fragment


          /* Padding flag is bit 6 */
          if (ch&0x40)
          {
   -         int padding=0;
             int p;
             do {
                if (len<=0)
                   return OPUS_INVALID_PACKET;
                p = *data++;
                len--;
   -            padding += p==255 ? 254: p;
   +            len -= p==255 ? 254: p;
             } while (p==255);
   -         len -= padding;
          }

Without knowing the type of len, it is not possible to determine
whether this code is correct. For instance, say len is uint32_t,
then the decrement of len may wrap.


In S 5, why did you change one of these memcpys to a memmove,
but not the other.


S 6.
   LPC_inverse_pred_gain_QA(), causing a wrap-around.  Although the
   error is harmless in practice, the C standard considers the behavior
   as undefined, so the following patch to line 87 of silk/
   LPC_inv_pred_gain.c detects values that do not fit in a 32-bit
   integer and considers the corresponding filters unstable:

Claiming that this is harmless in practice seems over-strong, given
that undefined behavior can do anything. Even if it's harmless
in practice today, it might not be in some other future compiler.


S 11.

   In addition, any Opus implementation that passes the original test
   vectors from RFC 6716 [RFC6716] is still compliant with the Opus
   specification.  However, newer implementations SHOULD be based on the
   new test vectors rather than the old ones.

I'm not sure I understand what compliance means in this case. Are you
saying that *either* behavior is compliant?


Additionally, WRT the discussion on list, I don't think it's useful
to go into a lot of detail about why you don't think this stuff is
exploitable. It takes a huge amount of effort to validate those kinds
of claims (and such claims have been wrong in other code bases in
the past). I would just say what the best known attack is and leave
it at that.

I agree with Mirja, we do need a better way for this type of update.

While I support the publication as this seems to be the right thing for me to do in this situation, I find it really weird that we have to publish an RFC to fix bugs in an example implementation in the appendix (that is even encoded). While it is a good thing to have code in an RFC as far as this helps implementation, maybe rfc6716 went to far with putting a whole reference implementation in there. I guess that has also led to a situation where everybody is just using the provided code while efforts to reimplement the spec could have detected these bugs earlier in the process. Maybe it's an item for the IESG to thinking about how to provide a reference implementation with an RFC (that maybe can be updated easier) other than just putting it in the appendix.

One more actionable comment: I think the abstract could say more about the updates, especially maybe noticing that this only updates the code in the appendix and not the normative language in the body.

Thanks for producing this specification. I have some questions, but nothing at Discuss level.

I have no idea what an “extreme bitstream” is. Aren’t they all pretty much 0s and 1s? 

More seriously, it would be nice to know what about the bitstream is extreme. Googling for ‘opus “extreme bitstream”’ turns up one hit for side effects from a drug being tested, so that wasn’t much help.

Is there an obvious way that the decoder can know whether audio will be downmixed outside the decoder? Section 10 thinks decoders can know that, apparently.

In the security considerations, why is it highly unlikely that the read-only table will be placed next to sensitive data? I’m not balloting Discuss on this because the assertion is nested in a reported vulnerability with no known exploit, but I am curious.