Description: A simple command-line password manager that keeps passwords inside a gpg encrypted tgz archive. The content of the archive is a directory tree with a file for each password entry. The first line of the file is the password, and the rest can optionally be additional or related info. It provides commands for manipulating the passwords, allowing the user to add, remove, edit, generate passwords etc. Repository: https://github.com/dashohoxha/pw Documentation: http://dashohoxha.github.io/pw/man/ This program started by forking 'pass': http://www.passwordstore.org/ I sugessted a few changes to it, which were not accepted, so I forked it and made further changes and improvements, until it became a completely different program. See: - https://lists.zx2c4.com/pipermail/password-store/2016-January/001887.html - https://lists.zx2c4.com/pipermail/password-store/2016-January/001902.html - https://lists.zx2c4.com/pipermail/password-store/2016-January/001928.html
I have to say that I'm pretty unconvinced of this code. * The archive is temporarily stored unencrypted on disk: encrypt and decrypt do an in-place operation with gpg, which is done wherever the encrypted archive lives. So tar stores onto disk into the target path, then the result is encrypted with gpg and the original is erased using rm -rf (also ignoring errors in case the file fails to delete). The inverse happens for decryption. * Symmetric and asymmetric encryption are not actually exclusive as the author makes it sound on the mailing list thread as gpg can wrap the session key with both symmetric and asymmetric keys. * Error handling in the script is wonky. I wonder if we could end up with an actual "rm -rf /" in case mktemp for WORKDIR fails. Errors on untar and tar are suppressed... * Comments like [0] aren't exactly inspiring either. The quoting in the script is "interesting". Sure, maybe you're asking for trouble anyway if your home directory contains a space, but this script will break in interesting ways. :) I did not look at the original code of pass, but I don't find this code handling secrets confidence inspiring, to be honest. Kind regards Philipp Kern [0] https://lists.zx2c4.com/pipermail/password-store/2016-January/001932.html
All your assertions/assumptions are wrong. Either you did not look close enough to the code, or you are not an expert on bash scripting (bash is a bit cryptic and difficult to understand even for experts). I did not look at the original code of pass, but I don't find this code Instead of basing your judgment on general opinions, why don't you try to find any particular situation that will break the script in some interesting way ;) This is called proof by counter-example. If you cannot do this, and if nobody else can do this, then you cannot claim that it is not safe to use this script. Best regards, Dashamir Hoxha
This is not a valid argument. Nobody can yet prove (by example) that it is not safe to go near a black hole. But it is not safe. The non existence of a counter-example does not mean that the theory is valid. Nonetheless, I don't have any opinion about pw. I use pass, and I think that forks are welcomed. Adrien
You are right to say that shell scripting (for bash or for Bourne shells
in general) is cryptic and difficult to understand. I've found that many
of the people who understand shell well enough to write correct shell
scripts prefer to avoid writing non-trivial shell scripts, because they
know they will be more productive in a language with fewer sharp edges.
In my experience, writing a robust shell script requires a lot of
defensive programming (this is not just my personal opinion, Debian
Policy also touches on this [1]).
You can use proof by counter-example to demonstrate that a particular
shell script is not correct: if your assertion is "this shell script
behaves as documented", finding an input that will make the script
behave not-as-documented disproves that assertion. The converse is
not true. If nobody finds an input that breaks the shell script, that
does not mean that no such input exists: it could equally well mean
that nobody has looked for long enough yet, for instance because they
consider auditing a shell script that doesn't start with set -e [1]
to be an inefficient use of their time.
smcv
[1] https://www.debian.org/doc/debian-policy/#scripts
Please review and sponsor it. Adrien
please *dont* sponsor this until Dashamir has addressed the concerns pointed out in https://lists.debian.org/msgid-search/aa2d4d3d-41d2-5399-225b-f492be2d2c1c@t-online.de
Hi Holger, (I've added a link to this thread on mentors.debian.net) Regards,
I just found this message in Spam, so I did not ignore it intentionally. I feel more productive with Bash scripts, maybe because I have used them for a long time, for non-trivial things. I always do defensive programming, not always with bash scripting. While `set -e` could be the right thing for trivial administration scripts (like installation, or running a bunch of commands, etc.), it would be the wrong thing for a non-trivial application, like 'pw', because it can leave the application in an inconsistent state. It is a poor substitution for defensive programming. Whoever stops auditing this Bash application because it does not start with `set -e`, does not have the right skills and experience for reviewing/auditing it.
$ sudo apt install shellcheck
$ curl -s https://raw.githubusercontent.com/dashohoxha/pw/master/src/pw.sh | shellcheck -
In - line 26:
$GPG --symmetric $opts --cipher-algo=AES256 \
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 31:
$GPG --encrypt $opts --use-agent --no-encrypt-to \
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 32:
$recipients "$archive"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 39:
$GPG $opts --passphrase-fd 0 "$archive.gpg" <<< "$PASSPHRASE"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 41:
$GPG --decrypt $opts --use-agent --output="$archive" "$archive.gpg"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 71:
make_workdir
^-- SC2119: Use make_workdir "$@" if function's $1 should mean script's $1.
In - line 91:
make_workdir
^-- SC2119: Use make_workdir "$@" if function's $1 should mean script's $1.
In - line 144:
local before="$(xclip -o -selection "$X_SELECTION" 2>/dev/null | base64)"
^-- SC2155: Declare and assign separately to avoid masking return values.
In - line 149:
local now="$(xclip -o -selection "$X_SELECTION" | base64)"
^-- SC2155: Declare and assign separately to avoid masking return values.
In - line 166:
make_workdir() {
^-- SC2120: make_workdir references arguments, but none are ever passed.
In - line 189:
find "$WORKDIR" -type f -exec $SHRED {} +
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 200:
[[ -f "$platform_file" ]] && source "$platform_file"
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
In - line 346:
local pass="$(cat "$WORKDIR/$path" | head -n 1)"
^-- SC2155: Declare and assign separately to avoid masking return values.
^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
In - line 569:
GPG_KEYS="$@"
^-- SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
In - line 584:
$GPG $GPG_OPTS --gen-key
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 606:
| while read pwfile
^-- SC2162: read without -r will mangle backslashes.
In - line 624:
[[ -f "$customize_file" ]] && source "$customize_file"
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
In - line 647:
*) try_ext_cmd $cmd "$@" ;;
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 659:
read -e -p 'pw> ' command options
^-- SC2162: read without -r will mangle backslashes.
In - line 665:
*) run_cmd $command $options ;;
^-- SC2086: Double quote to prevent globbing and word splitting.
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 684:
sleep $TIMEOUT
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 698:
source "$PW_DIR/cmd_$cmd.sh"
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
In - line 699:
debug running: cmd_$cmd "$@"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 700:
cmd_$cmd "$@"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 707:
source "$LIBDIR/ext/$PLATFORM/cmd_$cmd.sh"
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
In - line 708:
debug running: cmd_$cmd "$@"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 709:
cmd_$cmd "$@"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 716:
source "$LIBDIR/ext/cmd_$cmd.sh"
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
In - line 717:
debug running: cmd_$cmd "$@"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 718:
cmd_$cmd "$@"
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 723:
cmd_get $cmd
^-- SC2086: Double quote to prevent globbing and word splitting.
In - line 753:
source "$config_file"
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
In - line 781:
[[ -f "$ARCHIVE.gpg.keys" ]] && source "$ARCHIVE.gpg.keys" # get GPG_KEYS
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.
Hi, rather than trying to appeal to authority like Marc - I could have been wrong -, I will point out that my first point was not actually addressed at all: ++ mktemp -d /dev/shm/pw.sh.XXXXXXXXXXXXX + WORKDIR=/dev/shm/pw.sh.JHasAYH9zwYz1 [...] + decrypt /home/pkern/.pw/pw.tgz + local archive=/home/pkern/.pw/pw.tgz + local 'opts=--quiet --yes --batch ' + [[ -z '' ]] + gpg2 --quiet --yes --batch --passphrase-fd 0 /home/pkern/.pw/pw.tgz.gpg + local err=0 + [[ 0 -ne 0 ]] + tar -xzf /home/pkern/.pw/pw.tgz -C /dev/shm/pw.sh.JHasAYH9zwYz1 + rm -f /home/pkern/.pw/pw.tgz This clearly writes the unencrypted tarball out to disk. Now you might say "I will fix that". But I think the behavior in this thread was sufficient evidence to not package this in Debian. I know it is hard to deal with criticism and I could have phrased my mail differently, but this is clearly not the way. What if there is another contentious bug? Will you also insult your users and your fellow developers (no matter how senior)? I could not have phrased it better than Simon with "let's not try to audit scripts without `set -e`" and his other point of not using shell scripts for non-trivial matters. Yes, it's a valid language. Yes, people can write good scripts in it. But it is very hard and this thread has not given me evidence that the creation process of this script mastered it. Especially how hard it is to reason about a script that can fail silently in all places. Especially if it's something that handles a user's secrets. for> reviewing/auditing it. And these ad hominems are just unhelpful. Kind regards Philipp Kern
On Mon, Jul 16, 2018 at 12:34 PM Holger Levsen <holger@layer-acht.org> wrote: The email pointed out by the link has not arrived me at all. It is not in my inbox, it is not in my Spam, it is not in my Trash. I don't know what has happened, but I am not trying to avoid or ignore legitimate questions or concerns. I just figured out that I am not subscribed to debian-devel and Carsten has not included me on the Cc: I just tried it and most of the warnings are about using double quotes (when I know that they are not needed) and about not being able to follow included files. There is only 1 potentially useful suggestion in all of them. I have the same answer that I gave to Philipp. He has not looked close enough to the code, and has not tried to follow its logic. For example, error *messages* of `tar` are suppressed, not the errors themselves. The result of the command is checked afterwards. Etc. we can discuss them later. It all depends on the skills and experience of the programmer. Bash may be cryptic, ugly, difficult, etc. but nonetheless it is also powerful. Everybody claimed the same for javascript 20 years ago, but now it rules the world. Regards, Dashamir
Chris (and Holger), You could have forwarded that message to me, instead of trying to block my application. Dashamir
I have already answered this. Only one of the suggestions might be useful. If everything was clean, according to shellcheck, this wouldn't mean at all that the program is safe and secure and takes care of all the cases. I know what is going on in my program better than the mindless shellcheck.
As much as I would have liked to not reply, but alas, another ad hominem. The result of tar is not checked, no. The result of gpg is checked. I think the case I'm worried about is a race on ~/.pw/pw.tgz where between archive_unlock and archive_lock pw.tgz is set - say - 0400 and tar fails to write. That said, because you are so much into proofs: But as soon as tar writes incomplete output (which it totally can, it's a Tape ARchiver) you have silent corruption. Kind regards Philipp Kern
So, you have not looked at the code trying to follow the logic. You have just tried to debug it. This way you cannot get the full picture. But nevertheless it is useful for finding ways to break the script. By the way, you may notice that *there is* error checking there. This clearly writes the unencrypted tarball out to disk. It writes to `/dev/shm` which is not disk. It writes to a random temporary directory, so that it cannot be guessed. It removes the unencrypted content as soon as the operation is performed. All this happens almost instantly, it never stays unencrypted for a long time. It is almost the same thing as using a pipe (|). What is wrong here? I have been using it for 2-3 years and never had a problem. Now you might say "I will fix that". But I think the behavior in this You comments showed that you did not study it carefully, it is not hard to deal with criticism. For example you claimed there is no error handling, which is not true. I have almost 20 years programming experience, and have worked many years with bash scripting, using it in unconventional ways as well (which make you shy away from it). Maybe you have more experience than me but you have to show it on your comments. I could not have phrased it better than Simon with "let's not try to Let's agree that we don't agree about the usefulness of `set -e` and about the usefulness of Bash for serious matters. Regards, Dashamir
Yes, but this is because `gpg` will fail if `tar` fails. This is not a realistic example. You corrupt the `tar` command and then expect the program to work well. You might as well delete manually the archive file and then expect the program to work well. But as soon as tar writes incomplete output (which it totally can, it's It may happen, but the chances are so small. I have never heard of `tar` command (or any command) failing randomly on their own, without any reason. Anyway, it doesn't hurt to try and make the operations more transactional. Regards, Dashamir
That is not a valid assumption. You have no way of knowing the device behind /dev/shm. Unless the operation is atomic there is a possibility it can be interrupted. Ibid. It is not the same thing and it is based on several invalid/flawed assumptions. That doesn't make it correct code. I spend most of my day in code bases authored by other people. I consistently find bugs that have been in production, unreported, for 10 or more years. A bug is still a bug when it is found and identified, even if it has never manifested itself in the real world. If you doubt that, please review the recent news surrounding the SPECTRE and MELTDOWN vulnerabilities. Regards,
I've been following this thread and it is very difficult for me to understand why constructive criticism from others is so difficult for you to accept. In general, the community of Debian Developers is very concerned with producing a high quality distribution and also with supporting free software development. The fact that some have taken the time and interest to critique your work is very positive. Yet, you choose to perceive their critiques as an attack and then launch your own counter-attack. I don't mean to lecture, but your responses to several of the messages in this thread indicate that you are likely a younger/junior developer. That is not intended to be disparraging, but rather I am trying to understand the reason for the way in which you have responded in this thread. In my own case, I know that my attitude in response to critique was much like yours, when I was still a young developer who thought he knew it all. Over the years, though, I have come to understand that I know far less than I thought I knew when I was younger. That is, the world of programming knowledge far larger than I originally understood it to be. Even now, as a very experienced and senior developer, I frequently seek the advice and review of colleagues whenever I make significant changes to existing code, write new code, etc. I can tell you that I am a far better and more productive developer as a result. Another thing which seems to indicate that you are not particularly mature as a developer is the manner in which you quickly dismiss the results of static analysis. Certainly, there are instances where the tools do not fully understand the meaning of your code and provide false alarms. However, I have come to realize that static analysis is right for more than it is wrong. So, I have adopted the position that unless I can clearly articulate a good reason why the static analysis is wrong and my approach is better (and defend that reason to other programmers more senior than myself), I defer to the tool and fix the code. I do this in several programming languages. Additionally, the argument that you make, "If everything was clean, according to shellcheck, this wouldn't mean at all that the program is safe and secure and takes care of all the cases," is totally invalid. The fact that the tool fails to catch everything is not justification to automatically reject the things that it does catch. If the tool is consistently wrong, contact the developer of the tool with a sample of your code that you think the tool is incorrectly flagging, and convince the tool developer (using a technical and supported argument) why the tool should be updated. Your discussion with the tool developer might reveal to you that there is a defect in your own code that you did not understand. I encourage you, for your own benefit to accept the criticism from myself and others in the spirit in which it was intended: to help you produce a better free software tool and to improve as a developer. Regards,
So /home/pkern/.pw/pw.tgz is not "the unencrypted tarball"? This is just wrong.
Of course I did. Can we stop with the ad hominems and implying that the other party is stupid, please? No, it doesn't. /home/pkern/.pw/pw.tgz is not on /dev/shm. If it were a pipe, there wouldn't be a problem. But alas, there isn't one and it totally isn't the same as using a pipe. Kind regards Philipp Kern
Now I see. You are right.
You are right. Now I see the problem. I revoke the package request. I also ask your pardon for any unkind words. But I still think that this is not a problem of Bash, and no other language could have done it better. It is my mistake. Best regards, Dashamir
Can we assume you didn't look at the code trying to follow the logic and you don't have the full picture?
Yes you can. I just looked at the example provided, but did not look carefully enough.
Can we please stop here? It takes grace to take back the request. Kind regards and thanks Philipp Kern
Sure, but this email was sent before receiving that email.
On Mon, 16 Jul 2018 15:56:04 +0200 Dashamir Hoxha <dashohoxha@gmail.com > wrote: (...) Then lets just close the bugs.
All else that's been said aside, this idea is also dangerously incorrect in a typical configuration: the tmpfs backend will write to swap under memory pressure. (This is also true of the memory used by the process; if it's actually important to keep data from being written to persistent storage, it should be set unswappable using mlock. I have no idea how one would do this effectively in a shell script.) Mike Stone
It is possible, but ugly as hell in shell script. I did it in the following old script using foregrounded memlockd invocation, but that was written in shell script only really as an exercise in boredom/masochism. https://github.com/rowanthorpe/safe-key-setup/blob/master/safe-key-setup.sh#L131