Hi, I noticed that for one entry in my pass store `show -c` had stopped working. Apparently I went a bit overboard with supplemental information in that entry and now `show -c` was reporting a SIGPIPE since earlier commands in some internal pipeline were still trying to write while later commands had already extracted the password line and closed the read end.
Attached are two patches, one that adds some tests to detect this situation, and one that fixes the actual problem. Although I saw the problem initially with `show -c`, the tests are written for `show -q` since that seems easier to do portably. Note: tested on Linux only since that's all I have access to. Thanks for your work, Jö. --
From 2ccf90bed8606eac3094f596e3a10d4696496925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6=20Fahlke?= <[email protected]> Date: Sun, 11 Jun 2023 20:26:50 +0200 Subject: [PATCH 1/2] Test show -q with large secrets This breaks at the moment for large enough secrets. If a single line is extracted for `-q` (and `-c`), this happens with a pipe that feeds to `head -n 1`. This will simply close the pipe once the line has been read. Subsequent writes to the pipe will then result in a SIGPIPE to the writer, which will then report an exit code > 0 and no qrcode being displayed. --- tests/mock/qrencode | 3 +++ tests/setup.sh | 6 ++++++ tests/t0020-show-tests.sh | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100755 tests/mock/qrencode diff --git a/tests/mock/qrencode b/tests/mock/qrencode new file mode 100755 index 0000000..2f0e1de --- /dev/null +++ b/tests/mock/qrencode @@ -0,0 +1,3 @@ +#!/bin/sh + +cat >qrcode_data diff --git a/tests/setup.sh b/tests/setup.sh index 058ce0a..923d60f 100644 --- a/tests/setup.sh +++ b/tests/setup.sh @@ -62,3 +62,9 @@ KEY2="70BD448330ACF0653645B8F2B4DDBFF0D774A374" # pass test key 2 KEY3="62EBE74BE834C2EC71E6414595C4B715EB7D54A8" # pass test key 3 KEY4="9378267629F989A0E96677B7976DD3D6E4691410" # pass test key 4 KEY5="4D2AFBDE67C60F5999D143AFA6E073D439E5020C" # pass test key 5 + +# setup qrencode mock +export PATH="$TEST_HOME/mock:$PATH" +unset DISPLAY +unset WAYLAND_DISPLAY + diff --git a/tests/t0020-show-tests.sh b/tests/t0020-show-tests.sh index 81b87b4..193986a 100755 --- a/tests/t0020-show-tests.sh +++ b/tests/t0020-show-tests.sh @@ -24,4 +24,38 @@ test_expect_success 'Test "show" of nonexistant password' ' test_must_fail "$PASS" show cred2 ' +test_expect_success 'Create long multiline entry for later tests' ' + echo long_secret >long_credential && + # 1MiB should be enough to fill pipe buffers + yes | head -c "$((1024 * 1024))" >>long_credential && + echo >>long_credential && + "$PASS" insert -m long_cred <long_credential +' + +test_expect_success 'Sanity-check long multiline entry fills pipe buffers' ' + # This is important so later tests are not wrongly ok. If this sanity + # check fails, increase the size of the multiline entry until it + # succeeds + cat long_credential > >(head -n 1 >/dev/null) + result=$? + [[ $result -gt 128 ]] && [[ $(kill -l $result) == PIPE ]] +' + +test_expect_success 'Test "show" command with long multiline entry' ' + # We intentionally do not care about the exit status of pass or head, + # the actual test is whether we retreive the correct secret + [[ "$("$PASS" show long_cred | head -n 1)" == long_secret ]] +' + +test_expect_success 'Test "show -q" command' ' + "$PASS" insert -e cred3<<<"secret3" && + "$PASS" show -q cred3 && + echo -n secret3 | cmp qrcode_data - +' + +test_expect_failure 'Test "show -q" command with long multiline entry' ' + "$PASS" show -q long_cred && + echo -n long_secret | cmp qrcode_data - +' + test_done -- 2.30.2
From 42513d0ae8110d12e9f169e5f6ed8f45a1532ab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6=20Fahlke?= <[email protected]> Date: Sun, 11 Jun 2023 20:43:30 +0200 Subject: [PATCH 2/2] Fix show -q/-c for long secrets Commit 8446a40fbca8b092d28952edba163ed7df0be2a2 started checking the error of the pipeline extracting the password line. However, if an entry is too large to fit into the pipelines buffers, earlier commands in the pipeline will receive SIGPIPE, resulting in gpg exiting with an error. To avoid this, read the GPG output completely and report any errors from it before extracting the password line from the output. --- src/password-store.sh | 4 ++-- tests/t0020-show-tests.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/password-store.sh b/src/password-store.sh index 22e818f..defb4a9 100755 --- a/src/password-store.sh +++ b/src/password-store.sh @@ -383,12 +383,12 @@ cmd_show() { local passfile="$PREFIX/$path.gpg" check_sneaky_paths "$path" if [[ -f $passfile ]]; then + pass="$($GPG -d "${GPG_OPTS[@]}" "$passfile" | $BASE64)" || exit $? if [[ $clip -eq 0 && $qrcode -eq 0 ]]; then - pass="$($GPG -d "${GPG_OPTS[@]}" "$passfile" | $BASE64)" || exit $? echo "$pass" | $BASE64 -d else [[ $selected_line =~ ^[0-9]+$ ]] || die "Clip location '$selected_line' is not a number." - pass="$($GPG -d "${GPG_OPTS[@]}" "$passfile" | tail -n +${selected_line} | head -n 1)" || exit $? + pass="$($BASE64 -d <<<"$pass" | tail -n +${selected_line} | head -n 1)" [[ -n $pass ]] || die "There is no password to put on the clipboard at line ${selected_line}." if [[ $clip -eq 1 ]]; then clip "$pass" "$path" diff --git a/tests/t0020-show-tests.sh b/tests/t0020-show-tests.sh index 193986a..9fed41e 100755 --- a/tests/t0020-show-tests.sh +++ b/tests/t0020-show-tests.sh @@ -53,7 +53,7 @@ test_expect_success 'Test "show -q" command' ' echo -n secret3 | cmp qrcode_data - ' -test_expect_failure 'Test "show -q" command with long multiline entry' ' +test_expect_success 'Test "show -q" command with long multiline entry' ' "$PASS" show -q long_cred && echo -n long_secret | cmp qrcode_data - ' -- 2.30.2
signature.asc
Description: PGP signature
