[codec] Eric Rescorla's No Objection on draft-ietf-codec-opus-update-08: (with COMMENT)

[Eric Rescorla <[e-mail]>]

Eric Rescorla has entered the following ballot position for
draft-ietf-codec-opus-update-08: 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-opus-update/



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

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.