[MPlayer-dev-eng] [PATCH] Use unrar for open vobsubs if available

[Rich Felker]

On Sat, Nov 24, 2007 at 04:00:44PM +0800, Ulion wrote:
> > WTF, this is all utter nonsense. Path delimiter is the same on all
> > systems and it is /. Moreover, looking for unrar in . is a major vuln.
> > The PATH exists for a reason; use it.
> 
> I modified to search both '/' and '\'.
> looping for unrar in . is for safe reason.

It's still wrong. There is no extant platform where \ is needed. / has
been valid on DOS and all derivatives ever since 1.0.

> > > +    if (stat(cmd, &st) || !(st.st_mode&S_IXOTH)) {
> > > +#ifdef MACOSX_BUNDLE
> > > +        cmd = get_path(BIN_UNRAR_NAME);
> > > +        if (stat(cmd, &st) || !(st.st_mode&S_IXOTH)) {
> > > +#endif
> >
> > Looking in MPlayer config dirs is just bogus, too.
> 
> Check get_path.c, when MACOSX_BUNDLE defined, get_path will search the
> Resources directory of the bundle, that directory used for dynamic
> linked libraries and resources files, also can be external binary
> tools.

Bunding unrar with MPlayer as a single application bundle is highly
questionable and IMO GPL infringement.

> > > +    sprintf(cmdline, "'%s' p -inul %s '%s' '%s'", cmd, pwd, rarfile, filename);
> >
> > Vulnerable overflows and much worse. Consider for example if filename
> > or rarfile happened to be:
> > ....' & rm -rf ~ & '....
> 
> After a little test, I found only ' can break the '', so I for
> filename and rarfile I reject them if it contain '.
> for cmd, it's our defined for get from get_path, should be ok. And for
> the password, I change to use "" to quote it and escape \ and " by a
> prepending \ to make it safe.

You forgot to escape $ and ` and...
Anyway your whole approach is utterly brain-damaged from any
security-conscious perspective. There are ways to quote things safely
without excluding particular characters, and then there are idiotic
ways like you've done of blocking a few known-bad characters and
hoping everything else is safe. The very fact that you keep trying
fundamentally insecure approaches like this has me highly doubtful of
your qualifications as a developer...

> > popen is almost always a vuln if any part of the command line comes
> > from untrusted sources. You must either perform full shell quoting of
> > all arguments, or else use fork() and exec() manually.
> 
> That will be complex for me, so I select to make sure the cmd
> parameter is safe and escape them correctly.

"I don't know how to do the correct way so I write broken insecure
code" is not a valid justification.

Rich