This issue tracker has been migrated to GitHub ,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2015年09月04日 23:09 by bhou, last changed 2022年04月11日 14:58 by admin. 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 |
| 2022年04月11日 14:58:20 | admin | set | github: 69193 |
| 2015年09月07日 05:38:05 | python-dev | set | nosy:
+ python-dev messages: + msg250070 |
| 2015年09月07日 05:34:44 | larry | set | status: open -> closed resolution: fixed stage: resolved |
| 2015年09月07日 05:12:20 | larry | set | messages: + msg250063 |
| 2015年09月07日 05:03:34 | steve.dower | set | messages: + msg250061 |
| 2015年09月07日 04:56:33 | steve.dower | set | nosy:
+ jbmilam messages: + msg250060 |
| 2015年09月07日 04:53:32 | steve.dower | set | messages: + msg250059 |
| 2015年09月07日 04:45:18 | larry | set | messages: + msg250058 |
| 2015年09月07日 04:42:29 | steve.dower | set | messages: + msg250057 |
| 2015年09月07日 04:26:28 | larry | set | messages: + msg250055 |
| 2015年09月07日 04:21:06 | steve.dower | set | messages: + msg250054 |
| 2015年09月07日 04:19:54 | steve.dower | set | messages: + msg250053 |
| 2015年09月07日 04:16:18 | steve.dower | set | messages: + msg250050 |
| 2015年09月07日 02:39:31 | larry | set | messages: + msg250036 |
| 2015年09月06日 20:22:01 | r.david.murray | set | messages: + msg250015 |
| 2015年09月06日 13:57:13 | steve.dower | set | messages: + msg249995 |
| 2015年09月06日 08:06:01 | larry | set | nosy:
+ larry messages: + msg249982 |
| 2015年09月06日 08:05:38 | larry | set | priority: release blocker -> deferred blocker |
| 2015年09月05日 19:02:19 | steve.dower | set | messages: + msg249918 |
| 2015年09月05日 18:58:27 | steve.dower | set | files:
+ 25005_2.patch messages: + msg249917 |
| 2015年09月05日 18:53:03 | steve.dower | set | files:
+ 25005_1.patch keywords: + patch messages: + msg249916 |
| 2015年09月05日 04:09:27 | steve.dower | set | messages: + msg249892 |
| 2015年09月05日 01:26:17 | r.david.murray | set | priority: normal -> release blocker versions: + Python 3.6 nosy: + r.david.murray messages: + msg249880 type: behavior -> security |
| 2015年09月04日 23:09:09 | bhou | create | |