Open pawamoy opened 6 years ago
Hi Pawamoy, thanks for this result. Is indeed useful to pin down the problem. Zsh manual indicates the existance of two interesting flags:
-r
Ignore the escape conventions of echo.
-R
Emulate the BSD echo command, which does not process escape sequences unless the -e flag is given. The -n flag suppresses the trailing newline. Only the -e and -n flags are recognized after -R; all other arguments and options are printed.
I wonder if -r
or most likely -R
would help here if used preceeding -n
in the option_value
implementation at line 688. Can you test it? seems you found a pretty easy way to replicate.
In fact print
is innocent. The next suspect is zparseopts
: if I add a _verbose "${OPTS[--tomb-pwd]}"
right after the options parsing, I see that the %A
part of the password is already missing.
But what is printed might not correspond to what the actual values are. I see that the _msg
function uses the -P
flag for print
:
If the PROMPT_PERCENT option is set, certain escape sequences that start with '%' are expanded. Many escapes are followed by a single character, although some of these take an optional integer argument that should appear between the '%' and the next character of the sequence. More complicated escape sequences are available to provide conditional expansion.
(from https://linux.die.net/man/1/zshmisc, section "Expansion Of Prompt Sequences")
Continuing investigation...
So it seems that zparseopts
is innocent as well. Removing the -P
flag from the _msg
function shows that the password variable is correctly set. So I'm left with no clue for why I get the tomb [D] gpg: decryption failed: bad key
error :cry:. I tried the same command with another tomb (and other key+password) and it works perfectly.
So I wrote a script to check different passwords to see what characters will produce the issue:
#!/bin/bash
# create the tombs: password will be copied in clipboard,
# so just fill the pinentry input by clicking middle-mouse button
i=1
n=$(wc -l < passwords.txt)
while read -r password; do
echo "Tomb $i/$n, password $password"
let i++
echo -n "Creating... "
tomb dig -s 10 tomb_313.tomb &>/dev/null
echo -n "$password" | xclip
tomb forge tomb_313.key &>/dev/null
tomb lock tomb_313.tomb -k tomb_313.key &>/dev/null
echo -n "Opening... "
if tomb -D --unsafe --tomb-pwd "$password" open tomb_313.tomb -k tomb_313.key 2>&1 | grep -q 'bad key'; then
echo -e "\033[;31;1mNOT OK\033[0m"
else
echo -e "\033[;32;1mOK\033[0m"
fi
echo -n "Closing... "
tomb close tomb_313 &>/dev/null
echo "Removing... "
rm tomb_313.tomb
rm tomb_313.key
echo
done < passwords.txt
The file passwords.txt
contains the following passwords:
control_password_normal
control_password_special_/\|<>\t\n(){}[]---?!:;,.$+=`'""'`~&
with_%A
with_%A_hello
with_%s
%w
%%
%
\%
\\%
The result is:
Tomb 1/10, password control_password_normal
Creating... Opening... OK
Closing... Removing...
Tomb 2/10, password control_password_special_/\|<>\t\n(){}[]---?!:;,.$+=`'""'`~&
Creating... Opening... OK
Closing... Removing...
Tomb 3/10, password with_%A
Creating... Opening... NOT OK
Closing... Removing...
Tomb 4/10, password with_%A_hello
Creating... Opening... NOT OK
Closing... Removing...
Tomb 5/10, password with_%s
Creating... Opening... NOT OK
Closing... Removing...
Tomb 6/10, password %w
Creating... Opening... NOT OK
Closing... Removing...
Tomb 7/10, password %%
Creating... Opening... NOT OK
Closing... Removing...
Tomb 8/10, password %
Creating... Opening... NOT OK
Closing... Removing...
Tomb 9/10, password \%
Creating... Opening... NOT OK
Closing... Removing...
Tomb 10/10, password \\%
Creating... Opening... NOT OK
Closing... Removing...
As we can see, it really seems that a single %
in the password will always produce the issue, even if escaped (one or two backslashes), and only this character. #304 is another issue (\n
appended for TOMBKEY in the code, etc.).
So, I looked back again at the code to see how it unfolds:
And I fell onto these three lines (yes suspecting print again haha):
https://github.com/dyne/Tomb/blob/68a9589925cca68e5368eea1754c1a5c789c355b/tomb#L980
https://github.com/dyne/Tomb/blob/68a9589925cca68e5368eea1754c1a5c789c355b/tomb#L1011-L1012
I'm gonna try the -r
and -R
options there (maybe the -n
as well), to see if it fixes the issue or not.
Finally, I think I know where the issue comes from. If I print the contents of the output
variable from ask_password
with echo -E "$output"
, it seems that pinentry
is actually changing %
signs into %25
.
# Tomb 1/1, password %A__%%__%s__\\%
# tomb forge / lock / open, ask_password output:
OK Pleased to meet you
OK
OK
OK
OK
OK
D %25A__%25%25__%25s__\\%25
OK
So when creating the key, locking the tomb, or opening it, if we use pinentry, the password is transformed each time the same way, and it works. But then if we use --tomb-pwd
, the actual, original password is used, but it is now wrong.
To replicate the pinentry issue:
$ cat <<EOF | pinentry-gtk-2 # password typed: %s__\\%
OPTION ttyname=$(tty)
SETTITLE hello
SETDESC hello
SETPROMPT Password:
GETPIN
EOF
OK Pleased to meet you
OK
OK
OK
OK
D %25s__\\%25
OK
(I was able to replicate on two different machines, but with the same OS - Debian Jessie, as well as in a ubuntu:latest Docker container.)
%
has been transformedThanks again Pawamoy for this well done analysis. I am very surprised by pinentry. Wondering if there is a way we can fix this without impacting users, perhaps making tomb always do a transformation that tries twice the escaped and unescaped password. Its dirty, but then tomb is a wrapper whose goal is to make user's life easier not harder... and we can wrap this problem. What do you think? I will read your PR now.
Found relevant documentation about the behavior in ASSUAN's dev manual here at page 5 https://gnupg.org/documentation/manuals/assuan.pdf
D <raw data>
Raw data returned to client. There must be exactly one space after the ’D’.
The values for ’%’, CR and LF must be percent escaped; these are encoded
as %25, %0D and %0A, respectively. Only uppercase letters should be used in
the hexadecimal representation. Other characters may be percent escaped for
easier debugging. All Data lines are considered one data stream up to the OK
or ERR response. Status and Inquiry Responses may be mixed with the Data
lines.
Nice catch! So this is not a bug :slightly_smiling_face: I did take a look at the call-pinentry.c
from GnuPG and saw the unescape
function but was unable to say if the %25
was voluntary or not.
For your above comment, yes, maybe we could proceed in two steps:
%
is present and user mixes pinentry and --tomb-pwd
option), so everything will work as usual, transparently for the user. A note should also be added to inform users about this issue, and tell them that they will, at some point, need to update their passwords, because:%25
, %0D
and %0A
), requiring the users to update their passwords containing %
.Doing it in two steps would let the impacted users know how to deal with this issue, without forcing them to immediatly update both passwords and Tomb version.
Or, a more direct option would be to release a patch that always unescape the percent sequences, and tell users that if they want to upgrade Tomb they will need to fix their passwords containing %
(with an explanation on how to do so). It should not be difficult, but could require some time depending on the number of impacted passwords (but I guess people usually don't have 1K tombs with %
in passwords :astonished:).
Yea. I wish we'd have catched this earlier, but now is done. The manual is dated 2017 and I wonder if we could have known this by RTFM. However for now I definitely want to go for the two step and eventually leaving compatibility for it until the next major version.
Do you want me to give it a try? If so, do you have any hints or directions (places in code where we need to check escaped and unescaped password)? I don't particularly want to put my hands in the code and would feel better if you or another author/maintainer would take this on :sweat_smile:, but I still can try if nobody is willing to do so :slightly_smiling_face:.
I also checked that other pinentry alternatives print %25
as well, just to make sure, and they do. I tested gtk-2, gnome3, qt, qt4, x2go, tty, x11 and curses.
I checked pinentry-gtk-2's output for \n
and \r
, and as expected it returns %0A
and %0D
. I think we can assume that all the alternatives do the same. Here is a one-liner to test it: echo -ne '1 \n 2 \r 3' | xclip; echo GETPIN | pinentry-gtk-2
(then paste and click OK).
Thanks. I need to find some time more urgently now to review a pending PR and this issue. Don't worry if you don't feel like going forward, we got close to the issue and your contribution has been very useful for it. I'm not surprised all pinentry implementations do the same, since they all use libassuan where the parser really is.
I analysed a bit the complexity of the two-step fixes (supported escaped and unescaped % in a transition period) and noticed:
ask_password
function printing it at stdout. There is no space for intervention there IMHO, it will always return assuan escaped % symbolsask_key_password
handles most cases in which a password is needed to interact with the LUKS keystore and is the best candidate for interventionget_lukskey
function and ask_password
functions are used directly by exhume / bury functionalities, where an intervention should be crafted ad-hocPlease anyone correct me if I'm missing something.
So, the plan is to
Or
Anyway, I don't know the code enough but I think you are right about ask_password
. It needs to stay the same as long as we need to support the two versions. Therefore manual retries are needed elsewhere.
I'm leaving this as-is for now, too delicate to intervene. Best is to assume the ASSUAN's escaping is well meaning and that it is inherited in the way tomb 2.x encodes passwords.
I think this issue is related to #304, because it's about handling passwords containing backslahes or percent signs.
This issue however only happens when using the
--tomb-pwd
(and therefore the--unsafe
) option: my password contains%A
, and when trying to open the tomb from a script with--unsafe
and--tomb-pwd
options, the%A
simply disappears. I suspect the culprits here to be:https://github.com/dyne/Tomb/blob/68a9589925cca68e5368eea1754c1a5c789c355b/tomb#L1063-L1064
and
https://github.com/dyne/Tomb/blob/68a9589925cca68e5368eea1754c1a5c789c355b/tomb#L687-L689
Relevant part of the debug output: