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

[Ulion]

2007/11/24, Rich Felker <dalias at aerifal.cx>:
> On Sat, Nov 24, 2007 at 02:50:59PM +0800, Ulion wrote:
> > +#include <sys/stat.h>
> > +#include "mp_msg.h"
> > +#ifndef _WIN32
> > +#include "config.h"
> > +#define BIN_UNRAR_NAME "./unrar"
> > +#ifdef MACOSX
> > +#include "get_path.h"
> > +#endif /* MACOSX */
> > +#define PATHDELIMSTR "/"
> > +#else /* _WIN32 */
> > +#define BIN_UNRAR_NAME ".\\unrar.exe"
> > +#define PATHDELIMSTR "\\"
> > +#endif /* _WIN32 */
>
> 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.

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

>
> > +    if (libpassword && strlen(libpassword))
> > +        sprintf(pwd, "'-p%s'", libpassword);
> > +    else
> > +        pwd[0] = '\0';
> > +
> > +    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.

Now, if you still think there is any chance have security problem,
please give me a note.

>
> > +    fp = popen(cmdline, "r");
>
> 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.

Here's the updated patch.

-- 
Ulion
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unrarlib_use_bin_unrar2.diff
Type: text/x-diff
Size: 4503 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071124/d5838f1d/attachment.diff>