Last Call Review of draft-ietf-codec-opus-update-08

Request Review of draft-ietf-codec-opus-update
Requested rev. no specific revision (document currently at 10)
Type Last Call Review
Team Security Area Directorate (secdir)
Deadline 2017-08-09
Requested 2017-07-26
Authors Jean-Marc Valin, Koen Vos
Draft last updated 2017-08-17
Completed reviews Genart Last Call review of -08 by Robert Sparks (diff)
Secdir Last Call review of -08 by Steve Hanna (diff)
Assignment Reviewer Steve Hanna
State Completed
Review review-ietf-codec-opus-update-08-secdir-lc-hanna-2017-08-17
Reviewed rev. 08 (document currently at 10)
Review result Has Issues
Review completed: 2017-08-17


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., 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".