gh-68966: Fix CVE-2015-20107 in mailcap

[arhadthedev]

Fixes#68966. See #68966 (comment) for a detailed report.

Likely neeeds a backport up to 3.9.

[gpshead]

Is it plausible to create a regression test for this?

[JelleZijlstra]

Suggested change

	test=subst(e['test'], filename, plist)
	test=subst(e['test'], MIMEtype, filename, plist)

Don't know that we should fix this, but this is actually broken if test has a %s in it.

[JelleZijlstra]

This is still vulnerable with a sufficiently buggy test command. Example:

In [111]: c
Out[111]: {'x/y': [{'test': 'eval %s', 'view': 'x'}]}
In [112]: mailcap.findmatch(c, "x/y", filename="exit 1", plist='"echo hello"')
hello
Out[112]: ('x', {'test': 'eval %s', 'view': 'x'})

(had to set plist to work around the bug in #91542 (comment))

[JelleZijlstra]

Also, this approach will break some legitimate uses. On my Ubuntu system, mailcap has a lot of entries like 'test': 'test -n "$DISPLAY"',. If we don't run that command through a shell, the environment variable won't be expanded.

Maybe mailcap is just vulnerable by design. We can just put a big red banner in the docs and a warning on import, and be done with it when we remove the module in 3.13.

[vstinner]

Maybe mailcap is just vulnerable by design.

I confirm. For me, it's the responsibility of the caller (users of the mailcap API) to sanitize arguments. Maybe the Python mailcap module documentation can suggest to use shlex.quote()

Does it work to pass shlex.quote(filename) rather than filename to mailcap functions?

I also read somewhere that it's possible to use a temporary file to have a safe filename. But I don't know how to do that.

[encukou]

Maybe not a temporary file, but use an environment variable and pass "$PYTHON_MAILCAP_FILENAME" to the shell?

[gpshead]

Certainly a documentation update is a good idea (make a separate PR for that - we should get that in if nothing else). If we cannot actually fix all possible problems with this because of mailcap's fundamental design is it worth even a partial "fix"?

[vstinner]

I confirm. For me, it's the responsibility of the caller (users of the mailcap API) to sanitize arguments. Maybe the Python mailcap module documentation can suggest to use shlex.quote()

I'm talking about this:

@gpshead:

FWIW the only third party package we've found using mailcap already does create a safe filename before calling mailcap:
https://github.com/mitmproxy/mitmproxy/blob/main/mitmproxy/tools/console/master.py#L153

There is already a way to avoid mailcap vulnerabilities without fixing the code. It can be the responsibility of the caller to handle security.

Python stdlib has many very dangerous modules like os, shutil, ctypes or pickle. The pickle module clearly documents that it must not used to deserialize untrusted data. mailcap is not an exception here.

[hugovk]

Note that mailcap will be deprecated in Python 3.11 and removed in 3.13:

• PEP 594 – Removing dead batteries from the standard library
• Deprecate modules listed in PEP 594 #91217

[encukou]

Sorry for the misguided comment above. findcap returns shell command, of course it can't set environment variables.

Does it work to pass shlex.quote(filename) rather than filename to mailcap functions?

No, because we can't be sure of the context. This was mentioned in the issue in 2015: if the mailcap line already includes quotes, as in text/plain; less '%s'; needsterminal, extra quotes from shlex.quote would un-quote that and open the vulnerability again.

[arhadthedev]

Closing in favor of #91993.