Last Call Review of draft-ietf-codec-opus-update-08
review-ietf-codec-opus-update-08-secdir-lc-hanna-2017-08-17-00

I have reviewed this document as part of the security directorate's  ongoing
effort to review all IETF documents being processed by the IESG.  These
comments were written primarily for the benefit of the security area directors.
 Document editors and WG chairs should treat these comments just like any other
last call comments. I am not an expert in codecs but I do have considerable
experience with C coding and with security, both of which are relevant topics
for this review.

The summary of the review is Ready with Issues.

This draft fixes bugs in the reference implementation of the Opus codec
(defined in RFC 6716). Some of the bugs fixed have previously been identified
as security vulnerabilities (CVE-2013-0899 and CVE-2017-0381). RFC 6716 defines
the source code of the reference implementation to be normative, rather than
the English text in the RFC. This means that the code in RFC 6716 might well be
copied by implementers, thus exposing them to these vulnerabilities.

Here are the substantive issues:

1) In section 5, the third problem with the SILK resampler involves a possible
buffer overrun. This bug looks like to me like it could result in unauthorized
writes to the stack beyond the end of the buffer. These writes could be used
for return-oriented programming, to modify variables stored in the stack, or
even to execute arbitrary code. The draft says that "the code never produced
any error in testing" and "suggests that in practice [...] none of the issues
above was ever a problem." Many security vulnerabilities do not produce any
errors in testing but lead to serious security problems. Unless the authors can
prove that the bug is harmless, they should not minimize its potential impact.
Therefore, I suggest that this language in the draft be changed to avoid
minimizing the impact of the bug in any way. Rather, the draft should warn that
a buffer overrun permitting writes into the stack is a dangerous vulnerability
that can in some cases lead to arbitrary code execution, although the authors
are not aware at this time of any successful exploits. Minimizing the impact of
the bug may discourage readers from rapidly deploying the fix or adopting other
countermeasures.

2) Section 7 discusses and fixes the CVE-2017-0381 vulnerability. Although the
CVE description says that "A remote code execution vulnerability in
silk/NLSF_stabilize.c in libopus in Mediaserver could enable an attacker using
a specially crafted file to cause memory corruption during media file and data
processing.", subsequent analysis by many parties (e.g.,
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=851612#10) indicates that
this vulnerability cannot lead to remote code execution. Rather, it can only
result in an illegal read access to the stack. That's not good but it's not
nearly as bad as remote code execution. The draft correctly states that "The
end result of the wrap-around is an illegal read access on the stack". The text
in the CVE description should be corrected, if possible.

3) Section 12 says that "CVE-2017-0381 [...] could not be exploited in any way
(despite the claims in the CVE), unless the read-only table was somehow placed
very close to sensitive data, which is highly unlikely." Those who write
library code don't control the placement of input parameters such as buffers.
That's especially true for open source reference code that may be adapted in
almost any way. Therefore, the statement that the CVE could not be exploited in
any way seems to be a stretch. Rather, it would be better to warn about the
worst possible risks of the bug than to minimize its impact. Still, there would
be some benefit in explaining that community analysis of this vulnerability has
not supported the claim in the CVE that it enables remote code execution.

Also, here are a few nits:

* At the end of section 1, the text "has a SHA1" should be "has a SHA-1 hash
of". * In section 10 at the end of the first paragraph, "artefacts" should be
"artifacts". * In section 11 just before the hexadecimal values, "SHA1 hash"
should be "SHA-1 hashes". * In section 12, "theoretically cause information
leak" should be "theoretically cause an information leak". This text is split
by a line break so search for "information leak".