Command injection in IPython

[mschwager]

Hi IPython,

First off, I'd like to say how much I enjoy using IPython - I use it every day. Recently I've begun work on a static analysis tool for Python code. I ran a subset of its rules against the IPython code base and received some interesting results. In particular, Dlint's checks for shell=True subprocess calls:

$ python3 -m flake8 --select=DUO116 ipython/IPython/
ipython/IPython/core/interactiveshell.py:2482:22: DUO116 use of "shell=True" is insecure in "subprocess" module
ipython/IPython/core/hooks.py:80:12: DUO116 use of "shell=True" is insecure in "subprocess" module
ipython/IPython/core/page.py:214:24: DUO116 use of "shell=True" is insecure in "subprocess" module
ipython/IPython/lib/editorhooks.py:55:16: DUO116 use of "shell=True" is insecure in "subprocess" module
ipython/IPython/utils/_process_common.py:79:9: DUO116 use of "shell=True" is insecure in "subprocess" module
ipython/IPython/utils/sysinfo.py:58:12: DUO116 use of "shell=True" is insecure in "subprocess" module

I manually investigated each finding with the following results:

• interactiveshell.py: Unclear if vulnerable, unclear if shell=True is necessary
• hooks.py: Vulnerable via $EDITOR, shell=True unnecessary
• page.py: Vulnerable via $PAGER, shell=True unnecessary
• editorhooks.py: Unclear if vulnerable, unclear if shell=True is necessary
• _process_commend.py: Unclear if vulnerable, seems shell=True is necessary
• sysinfo.py: Not vulnerable, shell=True unnecessary

The following demonstrates the vulnerabilities in hooks and page:

$ PAGER='echo "pager" > /tmp/pager' ipython -c "open??"
$ cat /tmp/pager
pager

$ EDITOR='echo "editor" > /tmp/editor' ipython -c "%edit"
$ cat /tmp/editor
editor

This issue is highlighted in the Python subprocess docs: Subprocess Security Considerations. This issue falls under CWE-77: Improper Neutralization of Special Elements used in a Command ('Command Injection'). I would recommend avoiding shell=True whenever possible and investigating the other findings and ensuring they do not have the same issue. It appears that most of the calls do not need shell functionality anyway.

Let me know if you need any additional information!

[Carreau]

Note: This conversation already happen on the security mailing list and is posted in public for transparency.

---

@Carreau replied

Hi Matt,

Many thanks for the kind words, and thanks for the report, and for
pointing us to dlint..

A few questions and discussion though.
As far as I understand, "Command injection" usually refer to
application that are not meant to run arbitrary command. But do so
under adversarial user input.

1. The goal of IPython is to run arbitrary commands; If you run
   can run IPython, or enter commands into it you don't need to modify
   $PAGER or $EDITOR to execute arbitrary commands. I'm also unsure how
   an attacker would modify those env variable or provide adversarial
   input.
2. I believe the shell=True is necessary (at least ini $EDITOR) in
   both the case you outline we do want to explicitly run arbitrary
   commands. For example $EDITOR='atom -w' to have a blocking GUI editor
   after calling %edit. Shell=False will say no such command 'atom -w' – I guess those can be handled by shlex.split(), but other like
   ipython -c '%sx echo "onpurpose" > /tmp/onpurpose' really must use
   shell (sx stands for shell execute).

So I'd like to better understand what your threat model is, or would
be for such attacks.
Does "Command injection" really make sens when the goal of the project
is arbitrary code execution, or if I misunderstood some of the issues
behind command injections.

I understand that some of the above would be cleaner w/o shell=True,
at least to show good practice.

Let us know what you think, and wether there is a need to keep this
private; in which case i'm open to have this public on the IPython
repo and/or do a security advisory.

---

@takluyver replied

$PAGER can also include options, e.g. "less -R". So we can't easily switch away from shell=True there.

The threat model here would have to be an attacker that can set environment variables for a context where IPython will be used, but not give IPython commands directly or write anywhere on the filesystem (because if you can do that, you can point one of these environment variables to a script you've written, which gets run even with 'shell=False'). I imagine you can come up with some scenario like that, but given how IPython is designed to be used, I suspect those scenarios are pretty unlikely. If you figure out potential improvements in that area, I'd discuss them in the open rather than through the private security channels.

---

@mschwager replied

Hi,

Thanks for the clarifications. Based on your inputs it seems that there isn't much that can be done. Though I'd say avoiding shell=True is still a good practice :)

Since IPython is mostly a CLI/development-focused tool, it's unlikely that it's used in such a way where these concerns are exploitable. However, there are probably places where it's being used in an unusual, hard to envision scenario. Further, the attacker would have to be somewhat constrained. As already mentioned, the attacker would have to be able to set env variables but not files on the filesystem. But even in this situation, defensive programming (such as shlex or no shell) wouldn't stop something like: EDITOR='python -c ""' ipython ...

If we consider some of the previously mentioned concerns:
Requiring arguments to script, e.g. 'atom -w': we could avoid shell=True here by using shlex.split. Although this still falls into trouble with the arbitrary code example above.
Handling shell execute functionality, e.g. '%sx': there's not much to be done here, this is the intended behavior :)
Thinking about shell=True a bit more... I believe this is most dangerous when user input is being concatenated with an existing string. For example, consider the following:

cmd = 'ls -l {}'
cmd = cmd.format(cmd, user_input_directory)
subprocess.Popen(cmd, shell=True)

The dangerous part here is that user input could be 'foo; rm -rf /'. Adding shell=True enables multiple commands with ';'. Although the IPython codebase doesn't contain this behavior (or at least doesn't where code execution isn't intended).

All in all I'd say that there's no security issues here. Although I would say shlex instead of shell=True would generally be better practice.

I appreciate all your feedback here and quick responses. I can use this to improve Dlint's analysis capabilities.

[Carreau]

I've done #12024 to remove the one obvious with a static string.
I think we can try page.py and hook.py with the shlex.split() independently.

[takluyver]

We should remember to check anything we want to change on Windows, unless it's something that would never run on Windows. I vaguely recall some cases where things mysteriously failed on Windows and setting shell=True was the fix (/workaround).