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

[Jean-Marc Valin <[e-mail]>]

Hi Steve,

Thanks for the review and useful comments. See below.

On 11/08/17 09:38 AM, [e-mail] 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."


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

Will fix as suggested.

Cheers,

	Jean-Marc