
Remote code execution vulnerability in VLC remains unpatched - jmsflknr
https://www.zdnet.com/article/remote-code-execution-vulnerability-in-vlc-remains-unpatched/
======
deathanatos
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
_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-
immediat...](https://gizmodo.com/you-might-want-to-uninstall-vlc-
immediately-1836641101?fbclid=IwAR0ekZB3YuSBYezsxSvd1UjKEoU_8zvqRdxCipxms4f5ayWMJTyR1GtzeLk)

~~~
deathanatos
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
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)

