4 ms·

Remote code execution vulnerability in VLC remains unpatched

7 points by jmsflknr 7y ago

deathanatos 7y ago
The bug itself: https://trac.videolan.org/vlc/ticket/22474 https://trac.videolan.org/vlc/ticket/22474 The response seems … snippy: > If you land on this ticket through a news article claiming a critical flaw in VLC, I suggest you to read the above comment first and reconsider your (fake) news sources. So, the code at hand is as follows: for (std::vector<matroska_stream_c*>::reverse_iterator i = streams.rbegin(); i != streams.rend(); ++i) { matroska_stream_c *p_s = *i; if( !p_s->isUsed() ) { std::advance(i, 1); streams.erase( i.base() ); delete p_s; } } My big assumption here is that we're going to want to remove the p_s that we delete; I think that's a safe bet given the function's name. The key lines, I think, are the advance and erase. They … do not make sense to me. The erase call, as a side effect, invalidates the iterator. Regardless, I would expect us to be deleting the original position of i from the vector, not the advanced position. That is, I'd expect something like, auto old_i = i; ++i; streams.erase(old_i.base()); In addition to an, else { ++i } and the removal of the ++i from the for loop's last part. You need to step the iterator prior to erasing, then erase the old position, and then not inadvertently step the iterator a second time in the for() call. Alternatively, call remove_if() followed by a range-erase; though here I think you'd need to supply remove_if with a function with side-effects (for the delete), which is a bit "ew" in the understandability/readability department. remove_if would probably be the way to go, and a lot clearer, if the vector were holding something that RAII'd/understood the ownership of its contents, instead of a raw pointer. (Edit: actually, now that I've thought about it, no, even with the side-effects in the predicate, it's worth it to not need to manually deal with iterators.) I haven't tried to reproduce it, but no crashes does not mean there isn't a bug, and the code looks wrong. Am I missing something?
- dvtrn 7y ago
  The response seems … snippy: Am I missing something? It does appear from the link you provided that the maintainers regressed back across the last three major releases and also couldn't reproduce the issue, and I suspect their snippiness is coming from Gizmodo's[1] haste to publish an article asserting that their product contained vulnerabilities when it so far cannot be proved to.Possibly even the assertion that "VideoLAN is also aware of the issue and is currently working on a patch, though right now" which has me wondering: Did anyone at Gizmodo actually read the issue tracker to see the testing done by JBK? I'd be a bit annoyed and prone to snippiness at this kind of reporting about my code in a major online publication as well, honestly. --- [1] https://gizmodo.com/you-might-want-to-uninstall-vlc-immediately-1836641101?fbclid=IwAR0ekZB3YuSBYezsxSvd1UjKEoU_8zvqRdxCipxms4f5ayWMJTyR1GtzeLk https://gizmodo.com/you-might-want-to-uninstall-vlc-immediat...
  - deathanatos 7y ago
    That's fair; the article is probably taking its information from the CVE and the links to Symantec's "Security Focus" where apparently the research claims to have prepared an exploit. The bug itself (which perhaps is all the VLC people are looking at, aside from the news, now?) is pretty bare bones. I too have no idea where Gizmodo is getting the notion that it is being worked on. Perhaps they're getting that from the failed reproduction attempts.
    - dvtrn 7y ago
      Judging from the statement: VideoLAN is also aware of the issue and is currently working on a patch, though right now I'm going to guess someone quickly glanced at the issue tracker, saw a percentage of work done and presumed "vulnerability: valid" and pressed on with publication without bothering to try reaching JBK for comment. Both sites really ought to update their posts for clarity. Terrible form, IMO --in the interest of security disclosures. VideoLAN does not seem amused about it either: https://twitter.com/videolan/status/1153715138333220864 https://twitter.com/videolan/status/1153715138333220864
    - av3csr 7y ago
      They probably read the third comment, Component: Demuxers → Demuxers: MKV Work status: 60% → Not started

7 points by jmsflknr 7y ago

The bug itself: https://trac.videolan.org/vlc/ticket/22474 https://trac.videolan.org/vlc/ticket/22474 The response seems … snippy: > If you land on this ticket through a news article claiming a critical flaw in VLC, I suggest you to read the above comment first and reconsider your (fake) news sources. So, the code at hand is as follows: for (std::vector<matroska_stream_c*>::reverse_iterator i = streams.rbegin(); i != streams.rend(); ++i) { matroska_stream_c *p_s = *i; if( !p_s->isUsed() ) { std::advance(i, 1); streams.erase( i.base() ); delete p_s; } } My big assumption here is that we're going to want to remove the p_s that we delete; I think that's a safe bet given the function's name. The key lines, I think, are the advance and erase. They … do not make sense to me. The erase call, as a side effect, invalidates the iterator. Regardless, I would expect us to be deleting the original position of i from the vector, not the advanced position. That is, I'd expect something like, auto old_i = i; ++i; streams.erase(old_i.base()); In addition to an, else { ++i } and the removal of the ++i from the for loop's last part. You need to step the iterator prior to erasing, then erase the old position, and then not inadvertently step the iterator a second time in the for() call. Alternatively, call remove_if() followed by a range-erase; though here I think you'd need to supply remove_if with a function with side-effects (for the delete), which is a bit "ew" in the understandability/readability department. remove_if would probably be the way to go, and a lot clearer, if the vector were holding something that RAII'd/understood the ownership of its contents, instead of a raw pointer. (Edit: actually, now that I've thought about it, no, even with the side-effects in the predicate, it's worth it to not need to manually deal with iterators.) I haven't tried to reproduce it, but no crashes does not mean there isn't a bug, and the code looks wrong. Am I missing something?

dvtrn 7y ago
The response seems … snippy: Am I missing something? It does appear from the link you provided that the maintainers regressed back across the last three major releases and also couldn't reproduce the issue, and I suspect their snippiness is coming from Gizmodo's[1] haste to publish an article asserting that their product contained vulnerabilities when it so far cannot be proved to.Possibly even the assertion that "VideoLAN is also aware of the issue and is currently working on a patch, though right now" which has me wondering: Did anyone at Gizmodo actually read the issue tracker to see the testing done by JBK? I'd be a bit annoyed and prone to snippiness at this kind of reporting about my code in a major online publication as well, honestly. --- [1] https://gizmodo.com/you-might-want-to-uninstall-vlc-immediately-1836641101?fbclid=IwAR0ekZB3YuSBYezsxSvd1UjKEoU_8zvqRdxCipxms4f5ayWMJTyR1GtzeLk https://gizmodo.com/you-might-want-to-uninstall-vlc-immediat...
- deathanatos 7y ago
  That's fair; the article is probably taking its information from the CVE and the links to Symantec's "Security Focus" where apparently the research claims to have prepared an exploit. The bug itself (which perhaps is all the VLC people are looking at, aside from the news, now?) is pretty bare bones. I too have no idea where Gizmodo is getting the notion that it is being worked on. Perhaps they're getting that from the failed reproduction attempts.
  - dvtrn 7y ago
    Judging from the statement: VideoLAN is also aware of the issue and is currently working on a patch, though right now I'm going to guess someone quickly glanced at the issue tracker, saw a percentage of work done and presumed "vulnerability: valid" and pressed on with publication without bothering to try reaching JBK for comment. Both sites really ought to update their posts for clarity. Terrible form, IMO --in the interest of security disclosures. VideoLAN does not seem amused about it either: https://twitter.com/videolan/status/1153715138333220864 https://twitter.com/videolan/status/1153715138333220864
  - av3csr 7y ago
    They probably read the third comment, Component: Demuxers → Demuxers: MKV Work status: 60% → Not started