[PATCH] Bug: race condition in reencrypt_path

[Đoàn Trần Công Danh]

On 2022-12-06 18:26:16+0100, Michael J Gruber <git at grubix.eu> wrote:
> I guess this whole discussion shows the importance of a git commit
> message which addresses the "why" (what race condition) and "how"
> (does the patch solve this), since (only) the "what" (is changed) can
> be read from the diff.
> 
> BTW: Are we supposed to top-post or bottom-post here?

I think we're supposed to interleaved but Timm didn't.

> 
> Michael
> 
> Am Di., 6. Dez. 2022 um 16:15 Uhr schrieb Đoàn Trần Công Danh
> <congdanhqx at gmail.com>:
> >
> > On 2022-12-06 15:44:37+0100, t.fitschen at indiscale.com wrote:
> > > Sorry, Louis, what is tmpdir? It's not a bash built-in, is it? Did I
> > > miss something?
> >
> > tmpdir is a function in password-store.
> > It will create temporary directories in /dev/shm if available which is
> > guaranteed to be in devtmpfs on Linux. It also falls back to "mktemp"
> > on system that lacks /dev/shm and shred the file later.
> >
> > > I would suggest using `mktemp` which is as secure as gets when you are
> > > writing unencrypted content on the disk, I suppose. And yes, I think it
> > > is not ideal to do that.
> > >
> > > Apart from that, I don't know if fixing a gpg bug here is the way to go.
> > >
> > > While I didn't run into this bug yet, I think it is good that you
> > > found a work-around anyways, because I know that there have been some
> > > issues with the locking (https://dev.gnupg.org/T5884), so thank you.
> > >
> > > Regards
> > > Timm
> > >
> > > On Mon, Dec 05, 2022 at 11:16:24PM +0100, Louis Bettens wrote:
> > > > ---
> > > >  src/password-store.sh | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/password-store.sh b/src/password-store.sh
> > > > index 22e818f..549848e 100755
> > > > --- a/src/password-store.sh
> > > > +++ b/src/password-store.sh
> > > > @@ -110,6 +110,7 @@ set_gpg_recipients() {
> > > >  reencrypt_path() {
> > > >     local prev_gpg_recipients="" gpg_keys="" current_keys="" index passfile
> > > >     local groups="$($GPG $PASSWORD_STORE_GPG_OPTS --list-config --with-colons | grep "^cfg:group:.*")"
> > > > +   [[ -d "$SECURE_TMPDIR" ]] || die "Error: secure temporary directory not found"
> > > >     while read -r -d "" passfile; do
> > > >             [[ -L $passfile ]] && continue
> > > >             local passfile_dir="${passfile%/*}"
> > > > @@ -117,7 +118,7 @@ reencrypt_path() {
> > > >             passfile_dir="${passfile_dir#/}"
> > > >             local passfile_display="${passfile#$PREFIX/}"
> > > >             passfile_display="${passfile_display%.gpg}"
> > > > -           local passfile_temp="${passfile}.tmp.${RANDOM}.${RANDOM}.${RANDOM}.${RANDOM}.--"
> > > > +           local passfile_temp="${SECURE_TMPDIR}/passfile.tmp.${RANDOM}.${RANDOM}.${RANDOM}.${RANDOM}.--"
> > > >
> > > >             set_gpg_recipients "$passfile_dir"
> > > >             if [[ $prev_gpg_recipients != "${GPG_RECIPIENTS[*]}" ]]; then
> > > > @@ -133,8 +134,9 @@ reencrypt_path() {
> > > >
> > > >             if [[ $gpg_keys != "$current_keys" ]]; then
> > > >                     echo "$passfile_display: reencrypting to ${gpg_keys//$'\n'/ }"
> > > > -                   $GPG -d "${GPG_OPTS[@]}" "$passfile" | $GPG -e "${GPG_RECIPIENT_ARGS[@]}" -o "$passfile_temp" "${GPG_OPTS[@]}" &&
> > > > -                   mv "$passfile_temp" "$passfile" || rm -f "$passfile_temp"
> > > > +                   $GPG -d "${GPG_OPTS[@]}" -o "$passfile_temp" "${GPG_OPTS[@]}" "$passfile" &&
> > > > +                   $GPG -e "${GPG_RECIPIENT_ARGS[@]}" -o "$passfile" "${GPG_OPTS[@]}" "$passfile_temp" ||
> > > > +                   shred "$passfile_temp"
> > > >             fi
> > > >             prev_gpg_recipients="${GPG_RECIPIENTS[*]}"
> > > >     done < <(find "$1" -path '*/.git' -prune -o -path '*/.extensions' -prune -o -iname '*.gpg' -print0)
> > > > @@ -335,6 +337,8 @@ cmd_init() {
> > > >     local gpg_id="$PREFIX/$id_path/.gpg-id"
> > > >     set_git "$gpg_id"
> > > >
> > > > +   tmpdir #Defines $SECURE_TMPDIR, required for reencrypt_path
> > > > +
> > > >     if [[ $# -eq 1 && -z $1 ]]; then
> > > >             [[ ! -f "$gpg_id" ]] && die "Error: $gpg_id does not exist and so cannot be removed."
> > > >             rm -v -f "$gpg_id" || exit 1
> > > > @@ -624,6 +628,8 @@ cmd_copy_move() {
> > > >     local interactive="-i"
> > > >     [[ ! -t 0 || $force -eq 1 ]] && interactive="-f"
> > > >
> > > > +   tmpdir #Defines $SECURE_TMPDIR, required for reencrypt_path
> > > > +
> > > >     set_git "$new_path"
> > > >     if [[ $move -eq 1 ]]; then
> > > >             mv $interactive -v "$old_path" "$new_path" || exit 1
> > > > --
> > > > 2.38.1
> > > >
> > >
> > > --
> > > Herr/Mr Timm Fitschen
> > > (er/he)
> > > Development
> > >
> > > T: +49 551 288 76 48-3
> > > E: t.fitschen at indiscale.com
> > > I: indiscale.com
> > >
> > > IndiScale - Wir machen individuelles Datenmanagement skalierbar.
> > >
> > > IndiScale GmbH
> > > Lotzestraße 22a
> > > 37083 Göttingen
> > >
> > > Amtsgericht Göttingen • HRB 205721
> > > Geschäftsführung Henrik tom Wörden
> >
> >
> >
> > --
> > Danh

-- 
Danh