User-input handling

[chrisidefix]

I believe there is an issue the way user-input is processed. Specifically, I am concerned about the passphrase. The input passphrase is passed along to the execution module Naked.toolshed.shell.execute() without any safety checks. The code either uses Naked.toolshed.shell.execute() or Naked.toolshed.shell.muterun() to run all gpg commands.

The problem with this is that execute()/muterun() apparently will trigger subprocess.call/subprocess.check_output with shell=True. This is a relevant security problem as the user could (accidentally or not) execute any shell command this way. This exact scenario is actually documented here: https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

I am not sure if this is all still true in Python 3 or if getpass does anything special (I don't think so, other than not displaying what the user types). This should probably be addressed somehow, though.

[chrissimpkins]

The passphrase is saved as a property in the Cryptor class and then used to construct the gpg command here

https://github.com/chrissimpkins/crypto/blob/master/lib/crypto/library/cryptor.py#L56

It is used as an argument to the gpg --passphrase flag and quoted in the construction of the command so that, even if a user/attacker were to enter a character that would allow an executable to run (e.g. entering a pipe character followed by a nefarious command) the shell will prevent it. It is treated as a string argument in the shell. Can you think of an example command that would thwart this mechanism and lead to problems?

[chrissimpkins]

On the decryption side it is used here:

https://github.com/chrissimpkins/crypto/blob/master/lib/crypto/decryptoapp.py#L146

and here:

https://github.com/chrissimpkins/crypto/blob/master/lib/crypto/decryptoapp.py#L158

[chrisidefix]

Yes, you are right - the passcode is always passed as a string to the commandline by using single quotation characters '. This way white-space characters can be ignored in the passphrase, which is neat.

There is a problem, though, if the user himself uses the single quotation character in the passcode - this way he can end the string (effectively closing the string input). Then the shell will execute whatever comes thereafter. Example:

I'm in folder /foo which contains the file bar and run:

crypto bar

as a passphrase I choose:

test' --no --dryrun; rm secret_file2.txt; #'

Note the two quotation characters (after test and after #), which allow me to add any code to the shell execution. I then add the gpg commands --no and --dryrun, which will shut down any queries gpg may present. Afterwards I end the command with ; and go on to delete a file.

The shell will now execute:

gpg [--some_options] --passphrase 'test' --no --dryrun; 
rm secret_file2.txt; # [--other_now_irrelevant_options]

Sounds like one way to fix this 'attack' is to check for single quotation marks inside the passcode and escape them properly (\' should work) or throw an error for these cases.

Python doc, however, discourages the use of shell=True all together for subprocess calls. Not sure if this Naked framework has implemented this, but instead of passing a full string to the shell, we would have to create a list with the exact commands and then call subprocess.call(['gpg','--passcode',pswd_string,...], ...) if this makes sense.

[chrissimpkins]

Yep, you are absolutely right. I have some comments about this but am on my phone. Will discuss with you this weekend.

[chrissimpkins]

Let's escape the single quotes in the passphrase. That should address this issue.

passphrase =  passphrase.replace("'", "\\'")

Will need to confirm that this maintains the same passphrase that the user entered. It should.

[chrissimpkins]

By the way, this was implemented in Python 3.3 (does not exist in Python 2.x):

shlex.quote()

Here is the Python source for the function:

import re

_find_unsafe = re.compile(r'[^\w@%+=:,./-]', re.ASCII).search

def quote(s):
    """Return a shell-escaped version of the string *s*."""
    if not s:
        return "''"

    if _find_unsafe(s) is None:
        return s

    # use single quotes, and put single quotes into double quotes
    # the string $'b is then quoted as '$'"'"'b'

    return "'" + s.replace("'", "'\"'\"'") + "'"

[chrisidefix]

yes, not quite sure how you would write a test case for this, but I found that there is a python 2.7 alternative: pipes.quote()

Deprecated since version 2.7: Prior to Python 2.7, this function was not publicly documented. It is finally exposed publicly in Python 3.3 as the quote function in the shlex module.

http://stackoverflow.com/questions/26790916/python-3-backward-compatability-shlex-quote-vs-pipes-quote

[chrissimpkins]

Here is the Python source for pipes.quote():

# Reliably quote a string as a single argument for /bin/sh

# Safe unquoted
_safechars = frozenset(string.ascii_letters + string.digits + '@%_-+=:,./')

def quote(file):
    """Return a shell-escaped version of the file string."""
    for c in file:
        if c not in _safechars:
            break
    else:
        if not file:
            return "''"
        return file

    # use single quotes, and put single quotes into double quotes
    # the string $'b is then quoted as '$'"'"'b'
    return "'" + file.replace("'", "'\"'\"'") + "'"

Pretty similar.

[chrissimpkins]

I backported the shlex.quote() function as shellescape and will push it to PyPI in the morning after I test a bit more. Works in Py 2 & all versions of 3.

We can import this into crypto and confirm that it works with passphrases that include single quotes to address this issue.

[chrissimpkins]

it's live on PyPI. interested in helping me test it in crypto?

[chrisidefix]

Absolutely - I'll try it out this weekend and submit a pull-request, unless you already added it into crypto.

[chrissimpkins]

I haven't yet. That sounds great. Look forward to it.

[chrissimpkins]

Any luck with this?

[chrisidefix]

Just started on it - our weekend was too nice to stay inside ;)

[chrissimpkins]

No worries! No rush. Just interested in whether the new module actually worked with passphrases.

[chrisidefix]

Alright, so I tested this and it seems to work fine with passphrases. A few general observations:

• shellescape shows me an error when running under python3

import shellescape
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/site-packages/shellescape/__init__.py", line 4, in <module>
    from main import quote
ImportError: No module named 'main'

• you may want to revise the test-case that I created for this (it's pretty much just a copy of the unicode-passphrase unit-test)
• a better test-case should probably call gpg manually, since an error in the quote function will be applied to encrypting as well as decrypting and thus will not be detected right now
• would it make sense to include shellescape quote into the Naked framework?

Naked already provides Naked.commandline and Naked.toolshed.shell - I thought quote could fit in nicely somewhere over there.

[chrissimpkins]

shellescape shows me an error when running under python3

Let me take a look at this. I think that I need to modify it to:

from .main import quote

you may want to revise the test-case that I created for this...

No problem. Since we make the claim that crypto encrypted files should be able to be decrypted with gpg, I completely agree that we need to test with both decrypto and gpg decryption. I will add the tests.

would it make sense to include shellescape quote into the Naked framework?

Yes! naked is one of my favorite (and unfortunately long neglected) projects. Here is the issue report where we can discuss it.

[chrissimpkins]

I fixed the shellescape import error and released on PyPI as v3.4.1. It works in Py3 now.

[chrissimpkins]

This patch was released in v.1.4.1 and is now live on PyPI.