OT egpg evaluation

Robert J. Hansen rjh at sixdemonbag.org
Sun May 8 09:56:56 CEST 2016


> Or is it totally a worthless idea.

I think this code, in its current form, is genuinely dangerous, and
should not be recommended to anyone.

> Are you sure you used the right branch for testing?

I didn't test it.  What I saw while reading your code gave me such the
heebie-jeebies I *won't* test it -- that's how big some of the bugs are.

Let's start with the very first function I inspected.  I chose this
function because it's short, simple, and hard to screw up:


=====
gpg_send_keys() {
    is_true $SHARE || return
    gnupghome_setup
    gpg --keyserver "$KEYSERVER" --send-keys "$@"
    gnupghome_reset
}
=====


Okay, so to know if we can send keys successfully we have to know how
gnupghome_setup works.  Let's look at that (in auxiliary.sh).  The first
few lines are:


=====
gnupghome_setup() {
    # ...
    workdir_make
    # ...
}
=====


So now we need to know if workdir_make (in platform.sh) can fail.

I found workdir_make in platform.sh.  Let's look at that one.


=====
workdir_make() {
    [[ -z "$WORKDIR" ]] || return
    # ...
    trap workdir_clear INT TERM EXIT
}
=====


Whereupon my brow began to arch and I began to get a bad, bad feeling.
You test if WORKDIR is set, but what if the user has already set WORKDIR
for something else?  What if the user has specified "export
WORKDIR=/path/to/my/doctoral/thesis" in their startup file?

This is not auspicious, but it might be understandable.  Does the
manpage warn the user about this?  Searching through the manpage, there
is no mention of WORKDIR.

Moving on: the trap line means that when the script receives INT, TERM,
or EXIT, workdir_clear gets called.  So let's look at that.


=====
workdir_clear() {
    [[ -n "$WORKDIR" ]] || return
    [[ -d "$WORKDIR" ]] && find "$WORKDIR" -type f -exec shred {} +
    [[ -d "$WORKDIR" ]] && rm -rf "$WORKDIR"
    unset WORKDIR
}
=====


And at that point I decided that I *will not* test this code.  If
WORKDIR is set in the user's environment before they start egpg, egpg
will shred and rm -rf $WORKDIR.  This could have terrifying consequences
for my doctoral thesis, and even worse if someone has WORKDIR set to
something like /.

I found a potentially *system-destroying bug* in literally the *very
first function I inspected*.  I've been very circumspect in my
criticisms until now, Dashamir, because I really want to encourage
people to hack on things.  But it took me under seven minutes to
discover a bug that will destroy a user's hard drive, and that is not
the sort of thing which inspires trust in your code.

Now do you understand why so many people here are getting upset about
you recommending this package for inclusion in live Debian images,
recommending it to new users, etc.?



More information about the Gnupg-users mailing list