Subject: [secdir] SecDir Last Call Review of draft-ietf-codec-opus-update-08

From: <[email-masked]>
Date: Fri, 11 Aug 2017 13:38:46 +0000

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

---

From: Jean-Marc Valin <[email-masked]>
Date: Mon, 14 Aug 2017 16:45:34 -0400

Hi Steve,

Thanks for the review and useful comments. See below.

On 11/08/17 09:38 AM, [email-masked] wrote:
> 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.

So I spent some time to evaluate the consequences of the problem and
came to the conclusion that no out-of-bounds read or write is possible.
That's because the buffers involved (buf and S->sFIR) are actually
larger than they needed to be for the code to work. So the extra bytes
copied do not overrun any of the buffers. I'll update the text to state
to make it clear that the bug is harmless. It's still good to fix it
because it *does* look a bit scary until you've done the full analysis.

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

I tried getting the CVE corrected at the time, but didn't get far aside
from having the severity lowered slightly. It's worth noting that the
CVE came in about 6 months after the bug was fixed.

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

Here's the revised text I would suggest:

"CVE-2017-0381 is fixed by Section 7 and can only result in a 16-bit
out-of-bounds read to a fixed location 256 bytes before a constant
table. That location would normally be part of an executable's read-only
data segment, but if that is not the case, the bug could at worst
results in either a crash or the leakage of 16 bits of information from
that fixed memory location (if the attacker has access to the decoded
output). Despite the claims of the CVE, the bug cannot results in
arbitrary code execution."

> * 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”.

Will fix as suggested.

Cheers,

	Jean-Marc

---

From: <[email-masked]>
Date: Tue, 29 Aug 2017 22:11:01 +0000

Jean-Marc,

Sorry to be slow in responding to your email.

All the fixes that you propose seem completely appropriate to me and resolve my concerns. I see that these changes have already been made and the document approved for RFC status. Bravo!

Thanks,

Steve

-----Original Message-----
From: Jean-Marc Valin [mailto:[email-masked]] 
Sent: Monday, August 14, 2017 4:46 PM
To: Hanna Steve (IFAM CCS SMD AMR); [email-masked]; [email-masked]; [email-masked]
Subject: Re: SecDir Last Call Review of draft-ietf-codec-opus-update-08

Hi Steve,

On 11/08/17 09:38 AM, [email-masked] wrote:
> 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.

So I spent some time to evaluate the consequences of the problem and came to the conclusion that no out-of-bounds read or write is possible.
That's because the buffers involved (buf and S->sFIR) are actually larger than they needed to be for the code to work. So the extra bytes copied do not overrun any of the buffers. I'll update the text to state to make it clear that the bug is harmless. It's still good to fix it because it *does* look a bit scary until you've done the full analysis.

> Although the CVE description says that "A remote code execution 
> corruption during media file and data processing.", subsequent 
> analysis by many parties (e.g.,
> 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.

I tried getting the CVE corrected at the time, but didn't get far aside from having the severity lowered slightly. It's worth noting that the CVE came in about 6 months after the bug was fixed.

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

"CVE-2017-0381 is fixed by Section 7 and can only result in a 16-bit out-of-bounds read to a fixed location 256 bytes before a constant table. That location would normally be part of an executable's read-only data segment, but if that is not the case, the bug could at worst results in either a crash or the leakage of 16 bits of information from that fixed memory location (if the attacker has access to the decoded output). Despite the claims of the CVE, the bug cannot results in arbitrary code execution."

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

Will fix as suggested.

Cheers,

	Jean-Marc