specrec: better handle unexpected PS (CVE-2018-20360/CVE-2018-20199) #38
Conversation
Parametric Stereo (PS) can arrive at any moment in input files. PS changes the number of output channels and therefore requires more allocated memory in various structures from hDecoder. The current faad2 code attempts to perform allocation surgery in hDecoder to recover from this. This works well when there is only one frame channel, else it creates large number of memory corruption issues. If there is more than one input channel, return cleanly with error code. It would be nice to handle this, but this is likely to be a lot of work and is beyond the scope of a security fix. This commit addresses CVE-2018-20360 and CVE-2018-20199 (fixes knik0#32, fixes knik0#24).
|
Please, do not merge this right now, I'd like to make a second review of my code a bit later :-) |
|
Alright, I am waiting for your "go!". 👍 |
index range is supposed to be withing -7 and 7 or -15 and 15 depending on iid_mode (see Table 8.24, ISO/IEC 14496-3:2005). Indexes outside these boundaries are likely to be errors and should be sanitized to avoid memory corruption. Replace any index under -no_iid_steps (-7 or -15 depending on iid_mode) by -no_iid_steps. Replace any index above no_iid_steps by no_iid_steps. Try to process further. This commit addresses CVE-2019-6956 (fixes knik0#39).
|
I had a second look at "specrec: better handle unexpected PS", looks fine to me. I have pushed a new fix "ps_dec: sanitize iid_index before mixing" which addresses #39 (CVE-2019-6956). This is the best I can do with my current knowledge of the standard. It fixes the issue, and I don't expect it to break anything which was already working before. It would be great if you could have a look at it before merging :) |
|
Thanks for that! Now there is not much left to fix, I guess. 😁 |
Great ! I'm glad to hear that ! :) Fabian, are you planning to prepare a Debian testing upload? I will upload a jessie update shortly and will coordinate with the security team for stretch and buster fixes. |
Honestly, no, I didn't have such plans. My idea was to wait until the dust has settled a bit, tag a new release in this repository and upload that to Debian unstable. |
Well, I don't see clear benefits in waiting, at least in the jessie/stretch/buster cases. I'd like to see some of these issues fixed in Jessie (the stack buffer overflow in particular), and if I am not mistaken the security team also wants to release a DSA (at least they have a dsa-needed entry for faad2, I will discuss it again with them). If you need help, I can provide a NMU for unstable, featuring targeted patches, until you push a new release. |
Yes, please, that would be highly appreciated! |
Great, I have prepared a NMU, see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914641 :) |
Parametric Stereo (PS) can arrive at any moment in input files. PS changes the number of output channels and therefore requires more allocated memory in various structures from hDecoder.
The current faad2 code attempts to perform allocation surgery in hDecoder to recover from this. This works well when there is only one frame channel, else it creates large number of memory corruption issues.
If there is more than one input channel, return cleanly with error code. It would be nice to handle this, but this is likely to be a lot of work and is beyond the scope of a security fix.
This commit addresses CVE-2018-20360 and CVE-2018-20199 (fixes #32, fixes #24).