-
-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
User-input handling #8
Comments
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 |
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 |
Yes, you are right - the passcode is always passed as a string to the commandline by using single quotation characters 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
as a passphrase I choose:
Note the two quotation characters (after The shell will now execute:
Sounds like one way to fix this 'attack' is to check for single quotation marks inside the passcode and escape them properly ( Python doc, however, discourages the use of |
Yep, you are absolutely right. I have some comments about this but am on my phone. Will discuss with you this weekend. |
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. |
By the way, this was implemented in Python 3.3 (does not exist in Python 2.x):
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("'", "'\"'\"'") + "'" |
yes, not quite sure how you would write a test case for this, but I found that there is a python 2.7 alternative:
|
Here is the Python source for # 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. |
I backported the We can import this into crypto and confirm that it works with passphrases that include single quotes to address this issue. |
it's live on PyPI. interested in helping me test it in crypto? |
Absolutely - I'll try it out this weekend and submit a pull-request, unless you already added it into |
I haven't yet. That sounds great. Look forward to it. |
Any luck with this? |
Just started on it - our weekend was too nice to stay inside ;) |
No worries! No rush. Just interested in whether the new module actually worked with passphrases. |
Alright, so I tested this and it seems to work fine with passphrases. A few general observations:
|
Let me take a look at this. I think that I need to modify it to: from .main import quote
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.
Yes! naked is one of my favorite (and unfortunately long neglected) projects. Here is the issue report where we can discuss it. |
I fixed the shellescape import error and released on PyPI as v3.4.1. It works in Py3 now. |
This patch was released in v.1.4.1 and is now live on PyPI. |
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 usesNaked.toolshed.shell.execute()
orNaked.toolshed.shell.muterun()
to run all gpg commands.The problem with this is that
execute()
/muterun()
apparently will triggersubprocess.call
/subprocess.check_output
withshell=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.
The text was updated successfully, but these errors were encountered: