classification
Title: Shell injection via TIX_LIBRARY when using tkinter.tix
Type: security Stage: patch review
Components: Tkinter Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, christian.heimes, larry, ned.deily, serhiy.storchaka, symphorien, terry.reedy, zach.ware
Priority: high Keywords: patch

Created on 2016-12-31 19:00 by symphorien, last changed 2017-01-16 07:41 by larry.

Files
File name Uploaded Description Edit
tix_library_shell_injection.patch serhiy.storchaka, 2017-01-02 16:13 review
Messages (13)
msg284408 - (view) Author: (symphorien) Date: 2016-12-31 19:00
The tkinter.tix module looks for a Tix installation in the directory specified by the TIX_LIBRARY environment variable, but blindly trusts that it is a path in the filesystem. This enables a shell injection :

TIX_LIBRARY='/dev/null}; exec gsimplecal;' python2 -c "from Tix import Tk; Tk()"

or 

TIX_LIBRARY='/dev/null}; exec gsimplecal;' python3 -c "from tkinter.tix import Tk; Tk()"

Python execs gsimplecal, waits on its completion and then raises a tkinter.TclError.

The offending code is here : https://github.com/python/cpython/blob/master/Lib/tkinter/tix.py#L204-L208
msg284485 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-02 15:43
This code hasn't changed in years.  So while I believe it's a security bug and should be fixed, I don't know if I agree it's a bad enough security bug to stop Python 3.5.3rc1, which is literally in the middle of the release process.

I'm guessing this is easily fixed (if not os.path.isfile(tixlib): return), so how about we release 3.5.3rc1 with this bug and I'll cherry-pick this fix for 3.5.3 final.
msg284486 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-01-02 15:59
yeah, sounds totally fine to me. It's a low risk change and the issue has a small attack surface. I set the priority to release blocker to draw your attention.
msg284487 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-02 16:01
I agreed that this security issue has low severity. Only applications that use Tix are vulnerable, and this is very small number of applications.
msg284488 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-02 16:13
Here is a fix.
msg284851 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-06 21:20
I don't understand the fix.  Does this really prevent the injection?

I would fix it this way:

    if tixlib is not None and os.path.exists(tixlib):
msg284860 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-06 21:54
Yes this prevents the injection.

The injection is possible because the patch is substituted in the string without any escaping. Your fix is not enough. The real path to a Tix installation can contain special characters: '\', '{' or '}'.

My patch first sets a path to a Tcl variable (there is no an injection, because special API is used instead of evaluating a generated script), and then use this variable in the script (unlike to Unix shell Tcl doesn't reparse the command after substituting variables).
msg285187 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-11 05:35
Well, clearly I'm not qualified to review the patch.  Could someone please review it?  I want to cherry-pick the fix for this issue for 3.5.3 final, which I tag in about four days.
msg285462 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-14 07:03
Could one of you recent tagees (Terry, Zach) review the patch?  Hoping to tag 3.5.3 final in less than 48 hours, and I want to cherry-pick the fix for this...!
msg285472 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-01-14 10:45
In the original code, python interpolates tixlib into the string sent to and executed by tcl exec.  With the patch, tcl exec does the interpolation.  Not knowing anything in particular about tcl's exec, I found a value for tixlib that appears to validate Serhiy's claim that tcl exec does not rescan.

C:\Users\Terry>py -3.5
Python 3.5.2 (v3.5.2:4def2a2901a5, Jun 25 2016, 22:18:55) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import tkinter as tk
>>> tka = tk.Tk().tk
>>> txlib =  '} python -c "print(999)"'
>>> tka.setvar('TIX_LIBRARY', txlib)
>>> tka.eval('global autopath; lappend auto_path {%s}' % txlib)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
_tkinter.TclError: extra characters after close-quote

>>> tka.eval('global autopath; lappend auto_path $TIX_LIBRARY')
'{C:\\Programs\\Python35\\tcl\\tcl8.6} C:/Programs/Python35/tcl C:/Programs/lib C:/Programs/Python35/tcl/tk8.6 C:/Programs/Python35/tcl/tk8.6/ttk \\}\\ python\\ -c\\ \\"print(999)\\"'

I don't understand exactly why (or when) TclError is raised. but it is only raised when python does the interpolation.  And for this string, only when '}' is present.  Without the '}', there is no exception and the interpolated string is simply appended, as with the new $TIX_LIBRARY code.

test_tix, such as it is, passes with the patch.  So unless I missed something the patch appears to be both safe and useful.
msg285479 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-14 12:57
TclError in Terry's example is raised because Tcl script has unpaired braces. You should add "{" at the end of TIX_LIBRARY.

Here is working exploit:

$ TIX_LIBRARY="/dev/null}; exec python3 -m this >spoiled; set x {"  python3 -c "from tkinter.tix import Tk; Tk()"

It creates the file "spoiled" in current directory containing The Zen of Python.
msg285534 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-16 02:08
I'll make you a deal.  If you check this in in the next 3 hours, I'll cherry-pick it for 3.5.3.  Otherwise I don't want to hold up the release.  To be honest I'm not sure why it's marked as "release blocker" if it's "low severity".
msg285540 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-16 07:41
If it "has a small attack surface" and affects "a very small number of applications", I don't think it's a release blocker.  Demoting to "high" priority, which will permit me to release 3.5.3.
History
Date User Action Args
2017-01-16 07:41:20larrysetpriority: release blocker -> high

messages: + msg285540
2017-01-16 02:08:22larrysetmessages: + msg285534
2017-01-14 12:57:45serhiy.storchakasetmessages: + msg285479
2017-01-14 10:45:14terry.reedysetmessages: + msg285472
2017-01-14 07:03:09larrysetmessages: + msg285462
2017-01-11 06:53:53serhiy.storchakasetnosy: + terry.reedy, zach.ware
2017-01-11 05:35:36larrysetmessages: + msg285187
2017-01-06 21:54:52serhiy.storchakasetmessages: + msg284860
2017-01-06 21:20:04larrysetmessages: + msg284851
2017-01-02 16:13:31serhiy.storchakasetstage: patch review
2017-01-02 16:13:17serhiy.storchakasetfiles: + tix_library_shell_injection.patch
keywords: + patch
messages: + msg284488
2017-01-02 16:01:10serhiy.storchakasetmessages: + msg284487
2017-01-02 15:59:30christian.heimessetmessages: + msg284486
2017-01-02 15:43:20larrysetmessages: + msg284485
2017-01-02 13:05:54christian.heimessetpriority: normal -> release blocker
nosy: + larry, christian.heimes, benjamin.peterson, ned.deily
2017-01-01 17:28:53serhiy.storchakasetnosy: + serhiy.storchaka

versions: + Python 3.5, Python 3.7
2016-12-31 19:00:30symphoriencreate