Shell injection via TIX_LIBRARY when using tkinter.tix

[f4cc16ea-e99c-4377-9719-f9dc2436b4cf]

BPO	29125
Nosy	@terryjreedy, @larryhastings, @tiran, @benjaminp, @ned-deily, @zware, @serhiy-storchaka
Files	tix_library_shell_injection.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-security', '3.8', '3.7', 'expert-tkinter', '3.9'] title = 'Shell injection via TIX_LIBRARY when using tkinter.tix' updated_at = user = 'https://bugs.python.org/symphorien' ``` bugs.python.org fields: ```python activity = actor = 'zach.ware' assignee = 'none' closed = False closed_date = None closer = None components = ['Tkinter'] creation = creator = 'symphorien' dependencies = [] files = ['46113'] hgrepos = [] issue_num = 29125 keywords = ['patch'] message_count = 14.0 messages = ['284408', '284485', '284486', '284487', '284488', '284851', '284860', '285187', '285462', '285472', '285479', '285534', '285540', '356848'] nosy_count = 8.0 nosy_names = ['terry.reedy', 'larry', 'christian.heimes', 'benjamin.peterson', 'ned.deily', 'zach.ware', 'serhiy.storchaka', 'symphorien'] pr_nums = [] priority = 'high' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'security' url = 'https://bugs.python.org/issue29125' versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9'] ```

[f4cc16ea-e99c-4377-9719-f9dc2436b4cf]

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

[larryhastings]

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.

[tiran]

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.

[serhiy-storchaka]

I agreed that this security issue has low severity. Only applications that use Tix are vulnerable, and this is very small number of applications.

[serhiy-storchaka]

Here is a fix.

[larryhastings]

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):

[serhiy-storchaka]

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

[larryhastings]

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.

[larryhastings]

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

[terryjreedy]

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.

[serhiy-storchaka]

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.

[larryhastings]

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

[larryhastings]

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.

[zware]

Nearly 3 years on, the patch looks fine to me (though I would also accept this issue as justification for removing Tix ;).

[terryjreedy]

Tix is gone in 3.13. Should we close as won't fix or fix in 3.12/11 and backport?