classification
Title: webbrowser breaks on query strings with multiple fields on Windows
Type: security Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: bhou, jbmilam, larry, paul.moore, python-dev, r.david.murray, steve.dower, tim.golden, zach.ware
Priority: deferred blocker Keywords: patch

Created on 2015-09-04 23:09 by bhou, last changed 2015-09-07 05:38 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
25005_1.patch steve.dower, 2015-09-05 18:53 review
25005_2.patch steve.dower, 2015-09-05 18:58
Messages (21)
msg249858 - (view) Author: Brian Hou (bhou) Date: 2015-09-04 23:09
With Python 3.5.0rc2 (tested with both Git BASH and Cmder on Windows 8):

$ python3
>>> import webbrowser
>>> webbrowser.open_new('http://example.com/?a=1&b=2')
'b' is not recognized as an internal or external command,
operable program or batch file.
True

The opened page is http://example.com/?a=1 rather than http://example.com/?a=1&b=2.

Trying to open a URL with only one field works as expected.
msg249880 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-09-05 01:26
Thank you for reporting this.

I see that the Windows browser class uses shell=True, and that is wrong from a security standpoint.

This appears to be a regression from 3.4, introduced by issue 8232.  Since this is a security regression there either needs to be a fix or that changeset should be backed out.
msg249892 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-05 04:09
It'll have to be backed out. There may be a fix to salvage some of the functionality, but it's certainly too significant at this stage.
msg249916 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-05 18:53
Here's an alternative to backing out the change, and it's simpler than I expected when I said it would be too much for 3.5.0.

We add an 'arguments' parameter to os.startfile and use that instead of subprocess.call(shell=True). The underlying ShellExecute call does not do any argument processing, so you can pass through any arguments you like.

In the attached patch, I only added the argument for when Unicode strings are used, since byte strings are deprecated, but it's fairly trivial to add it to both. I'll add a backout patch next so they can be compared.
msg249917 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-05 18:58
Patch for backing out #8232's changes.
msg249918 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-05 19:02
Patch 1 also requires a minor update to Doc\library\os.rst:

-.. function:: startfile(path[, operation])
+.. function:: startfile(path[, operation[, arguments]])
...
+*arguments* is passed to the underlying :c:func:`ShellExecute`
+call. Its format is determined by the target file and operation.
msg249982 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-09-06 08:06
Marking this as deferred, as I'm not convinced I should ship either of those patches in 3.5.0rc3.
msg249995 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-06 13:57
Does that mean not shipping either of them in 3.5.0 at all? Do you need convincing or a simpler patch?
msg250015 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-09-06 20:22
You have to ship one of them by ("one of them" being either the fix or the backout) in 3.5.0, Larry, otherwise you are introducing a security vulnerability into 3.5 that doesn't exist in 3.4.  If you don't ship it in rc3, then there's no chance that 3.5.0 will be unchanged from rc3, which is the ideal we should be aiming for (that's why it's called a "release *candidate*").
msg250036 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-09-07 02:39
I want to ship something, but I don't think it'll be either of those patches in their current form.

Maybe I'm dense, but I don't feel like I understand these patches.  They have very different approaches.

The first one attempts to rehabilitate the patch by running the browser directly instead of using "start"?  Do the features added in issue 8232 still work?

The second one just backs out the feature completely?  The features added in issue 8232 are gone?

And both patches switch from using subprocess.call(shell=True) to os.startfile(), which is the security hole David is talking about, which isn't actually what this bug is about.

Do both patches fix the erroneous behavior observed by the original bug report, with "&" being interpreted as a shell command separator?
msg250050 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 04:16
subprocess with shell=True turns it into a "cmd.exe /C "start chrome.exe ..."" type command, which means the arguments will use shell parsing (e.g. > for redirection, & for multiple commands, etc.)

"start" in cmd.exe behaves the same as os.startfile, but can also accept arguments. Patch 1 lets startfile accept arguments, so we don't have to go via cmd.exe to launch the browser (we're still resolving it via the shell, so the behaviour is the same, but the arguments are never interpreted using shell rules). All the functionality of #8232 is still there and still works.

Patch 2 is a rollback of #8232, so we ship the same functionality as in Python 3.4. Because this only supports the default browser and does not support selecting between new tab/window, it can go back to using os.startfile without arguments.

A third option would be to find some other way to discover the full path to each browser and launch it without using start/os.startfile. (This is what I was thinking originally when I said it would be too big a change.)
msg250053 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 04:19
To be more specific, with patch 1 applied:

subprocess.call("start file a&b>x", shell=True)

is equivalent to typing the following at a command prompt:

start file a & b > x

That is, "start file a" and then do "b", redirecting the output from "b" to a file named "x".

With the change to os.startfile, we can write it as this:

os.startfile("file", "open", "a&b>x")

Which matches what we intended above. (startfile implies "start <first argument>", and passes the third argument to the launched program without modification.)
msg250054 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 04:21
Oh, and the "start" is necessary because, while the Windows kernel can only resolve "chrome.exe" if it appears on PATH, the Windows shell has some other ways to resolve it.

By using ShellExecute (via 'start' or startfile), we can let the OS find it rather than having to find it ourselves.
msg250055 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-09-07 04:26
So, whatever the security hole is that subprocess.call(shell=True) leaves open, os.startfile() doesn't have?  os.startfile() doesn't use a shell?  (How does it find the full path to the executable?)
msg250057 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 04:42
Correct.

os.startfile uses ShellExecute (https://msdn.microsoft.com/en-us/library/windows/desktop/bb762153.aspx), which is the same API that the shell uses for the 'start' command. So by using os.startfile we get the same behaviour, but we're calling in after the terminal would have done its own parsing (for piping/redirection/etc.).
msg250058 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-09-07 04:45
Well, so, what do you recommend I do here?
msg250059 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 04:53
Rollback.

I'm not 100% confident in patch 1 (too many things I can't predict) and with only a week it probably won't get enough testing to flush out other surprises.
msg250060 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 04:56
I guess now I've been that definitive I'll go make you a PR :)

If someone (perhaps Brandon?) is willing to thoroughly validate patch 1 we might be able to consider it for 3.5.1 (the only API change is to startfile() - the webbrowser API is already there, it just doesn't report much). Otherwise, it'll have to be 3.6. (But will it get any more tested there? Not so sure...)
msg250061 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-07 05:03
PR is: https://bitbucket.org/larry/cpython350/pull-requests/20/issue-25005-backout-fix-for-8232-because/diff
msg250063 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-09-07 05:12
Backout pull request merged, please forward-merge, thanks!
msg250070 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-07 05:38
New changeset aa60b34d5200 by Steve Dower in branch '3.5':
Issue #25005: Backout fix for #8232 because of use of unsafe subprocess.call(shell=True)
https://hg.python.org/cpython/rev/aa60b34d5200

New changeset 7d320c3bf9c6 by Larry Hastings in branch '3.5':
Merged in stevedower/cpython350 (pull request #20)
https://hg.python.org/cpython/rev/7d320c3bf9c6
History
Date User Action Args
2015-09-07 05:38:05python-devsetnosy: + python-dev
messages: + msg250070
2015-09-07 05:34:44larrysetstatus: open -> closed
resolution: fixed
stage: resolved
2015-09-07 05:12:20larrysetmessages: + msg250063
2015-09-07 05:03:34steve.dowersetmessages: + msg250061
2015-09-07 04:56:33steve.dowersetnosy: + jbmilam
messages: + msg250060
2015-09-07 04:53:32steve.dowersetmessages: + msg250059
2015-09-07 04:45:18larrysetmessages: + msg250058
2015-09-07 04:42:29steve.dowersetmessages: + msg250057
2015-09-07 04:26:28larrysetmessages: + msg250055
2015-09-07 04:21:06steve.dowersetmessages: + msg250054
2015-09-07 04:19:54steve.dowersetmessages: + msg250053
2015-09-07 04:16:18steve.dowersetmessages: + msg250050
2015-09-07 02:39:31larrysetmessages: + msg250036
2015-09-06 20:22:01r.david.murraysetmessages: + msg250015
2015-09-06 13:57:13steve.dowersetmessages: + msg249995
2015-09-06 08:06:01larrysetnosy: + larry
messages: + msg249982
2015-09-06 08:05:38larrysetpriority: release blocker -> deferred blocker
2015-09-05 19:02:19steve.dowersetmessages: + msg249918
2015-09-05 18:58:27steve.dowersetfiles: + 25005_2.patch

messages: + msg249917
2015-09-05 18:53:03steve.dowersetfiles: + 25005_1.patch
keywords: + patch
messages: + msg249916
2015-09-05 04:09:27steve.dowersetmessages: + msg249892
2015-09-05 01:26:17r.david.murraysetpriority: normal -> release blocker
versions: + Python 3.6
nosy: + r.david.murray

messages: + msg249880

type: behavior -> security
2015-09-04 23:09:09bhoucreate