using --run broken/insecure when URL contains ampersand

[kovidgoyal]

urlchoose.py uses

Popen(self.run.format(url), shell=True).communicate()

this will not work if the url contains the ampersand character. It also allows arbitrary code execution with maliciously crafted URLs. You should be doing something like:

Popen(shlex.split(self.run.format(url))).communicate()

[firecat53]

Yep...it's not preferred, thus the note in the README:

Instead of opening a web browser, the selected URL can be passed as the argument to a command using --run "<command> {}". Note the use of {} in the command string to denote the selected URL. Alternatively, the URL can be piped to the command using --run <command> --pipe. Using --run with --pipe is preferred if the command supports it, as it is marginally more secure and tolerant of special characters in the URL.

The --pipe option is available as a more secure alternative. I intentionally used the shell=True format for --run to allow some people to use urlscan in some interesting ways. I think the chances of an arbitrary code execution attack via email knowing that person uses urlscan is pretty small!

If you have an alternative solution I'm open to it, but take a look though the closed issues and PRs so you understand some of the history.

Thanks!

[kovidgoyal]

It seems to me that making the default behavior unsafe to support a less common usecase is the wrong approach. The correct way would have been to have two options,

--run and --run-in-shell

since I am guessing you dont want to break backwards compat, perhaps add --run-safe which would be implemented as:

cmd = [q.format(url) for q in shlex.split(run_safe)]
subprocess.Popen(cmd)

[firecat53]

A wee bit late getting back to you on this :smile:

--run does seem to work with the ampersand:

$ echo -e "This is https://joe.joe.com&m=25&k=30\n\nand this is another url\nhttp://google.com&mk#45 is another one" | urlscan --run 'cat {}' -n
https://joe.joe.com&m=25&k=30
http://google.com&mk#45

And also works with --pipe:

$ echo -e "This is https://joe.joe.com&m=25&k=30\n\nand this is another url\nhttp://google.com&mk#45 is another one" | urlscan --run 'cat' --pipe -n
https://joe.joe.com&m=25&k=30
http://google.com&mk#45

However, your comments about security are well taken. I created the runsafe branch implementing --run-safe and --pipe --run-safe. If you are still following this, I'd really appreciate a code review, as you are a much more experienced developer!

Thanks! Scott

[firecat53]

Merged 104b09b to add a --run-safe implementation. Thanks again for the input.