| Message ID | 20230821202606.49067-1-list@eworm.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] t6300: fix match with insecure memory | expand |
Christian Hesse <list@eworm.de> wrote: > From: Christian Hesse <mail@eworm.de> > > Running the tests in a build environment makes gnupg print a warning: > > gpg: Warning: using insecure memory! > > This warning breaks the match, as `head` misses one line. Let's strip > the line, make `head` return what is expected and fix the match. > > Signed-off-by: Christian Hesse <mail@eworm.de> I think a bit of an explanation about why this warning is showing up in the commit message would be good. "man gpg" gives me On older systems this program should be installed as setuid(root). This is necessary to lock memory pages. Locking memory pages prevents the operating system from writing memory pages (which may contain passphrases or other sensitive material) to disk. If you get no warning message about insecure memory your operating system supports locking without being root. The program drops root privileges as soon as locked memory is allocated. Note also that some systems (especially laptops) have the ability to ``suspend to disk'' (also known as ``safe sleep'' or ``hibernate''). This writes all memory to disk before going into a low power or even powered off mode. Unless measures are taken in the operating system to protect the saved memory, passphrases or other sensitive material may be recoverable from it later. So it seems that this warning will pop up if gpg is writing memory pages to disk which is bad because as stated above we don't want these pages written to disk which is a security risk. > --- > t/t6300-for-each-ref.sh | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 5b434ab451..0f9981798e 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -1764,12 +1764,13 @@ test_expect_success GPGSSH 'setup for signature atom using ssh' ' > > test_expect_success GPG2 'bare signature atom' ' > git verify-commit first-signed 2>out.raw && > - grep -Ev "checking the trustdb|PGP trust model" out.raw >out && > + grep -Ev "checking the trustdb|PGP trust model|using insecure memory" out.raw >out && > head -3 out >expect && > tail -1 out >>expect && > echo >>expect && > git for-each-ref refs/tags/first-signed \ > - --format="%(signature)" >actual && > + --format="%(signature)" >out.raw && > + grep -Ev "using insecure memory" out.raw >actual && > test_cmp expect actual > ' > > -- > 2.41.0 We skip "checking the trustdb" and "PGP trust model" lines (which are not warnings) here because we don't really need those from the output that GPG produces here but skipping a warning too seems kind of a question mark. It also seems that one could use "--no-secmem-warning" to suppress such a warning. So a better place to make a change would not be in t/t6300 but in t/lib-gpg from where the prereq GPG2 comes from. Although I'm against this, because we don't really want to suppress any warnings. I think it is a good thing this test is breaking because it informs us about the security risk. I have Cc'ed people who might have a thought on this. So it's better to wait for their response. Thanks
Kousik Sanagavarapu <five231003@gmail.com> on Tue, 2023/08/22 13:24: > Christian Hesse <list@eworm.de> wrote: > > > From: Christian Hesse <mail@eworm.de> > > > > Running the tests in a build environment makes gnupg print a warning: > > > > gpg: Warning: using insecure memory! > > > > This warning breaks the match, as `head` misses one line. Let's strip > > the line, make `head` return what is expected and fix the match. > > > > Signed-off-by: Christian Hesse <mail@eworm.de> > > I think a bit of an explanation about why this warning is showing up in the > commit message would be good. > > "man gpg" gives me <stripped> > > So it seems that this warning will pop up if gpg is writing memory pages to > disk which is bad because as stated above we don't want these pages written > to disk which is a security risk. The Arch Linux packages are built inside a clean container, started via systemd-nspawn. Within the container the system call @memlock is not allowed by default, for security reasons. There's an upstream systemd issue on this topic: https://github.com/systemd/systemd/issues/9414 Note this is only true at build time. If the packages are installed on the actual system the @memlock system call is available and things work as expected without issues. > > --- > > t/t6300-for-each-ref.sh | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > > index 5b434ab451..0f9981798e 100755 > > --- a/t/t6300-for-each-ref.sh > > +++ b/t/t6300-for-each-ref.sh > > @@ -1764,12 +1764,13 @@ test_expect_success GPGSSH 'setup for signature > > atom using ssh' ' > > test_expect_success GPG2 'bare signature atom' ' > > git verify-commit first-signed 2>out.raw && > > - grep -Ev "checking the trustdb|PGP trust model" out.raw >out && > > + grep -Ev "checking the trustdb|PGP trust model|using insecure > > memory" out.raw >out && head -3 out >expect && > > tail -1 out >>expect && > > echo >>expect && > > git for-each-ref refs/tags/first-signed \ > > - --format="%(signature)" >actual && > > + --format="%(signature)" >out.raw && > > + grep -Ev "using insecure memory" out.raw >actual && > > test_cmp expect actual > > ' > > > > -- > > 2.41.0 > > We skip "checking the trustdb" and "PGP trust model" lines (which are not > warnings) here because we don't really need those from the output that GPG > produces here but skipping a warning too seems kind of a question mark. > > It also seems that one could use "--no-secmem-warning" to suppress such a > warning. So a better place to make a change would not be in t/t6300 but in > t/lib-gpg from where the prereq GPG2 comes from. Although I'm against this, > because we don't really want to suppress any warnings. > > I think it is a good thing this test is breaking because it informs us about > the security risk. I have Cc'ed people who might have a thought on this. So > it's better to wait for their response. Well, after all I just want to change the tests to succeed with our build environment, let's take a detailed look at the issue. All command below are inside the build environment, so including the warning about insecure memory. The output of `git verify-commit first-signed` is: ---- >8 ---- gpg: Warning: using insecure memory! gpg: Signature made Tue Aug 22 08:46:43 2023 UTC gpg: using DSA key 73D758744BE721698EC54E8713B6F51ECDDE430D gpg: issuer "committer@example.com" gpg: checking the trustdb gpg: marginals needed: 3 completes needed: 1 trust model: pgp gpg: depth: 0 valid: 1 signed: 0 trust: 0-, 0q, 0n, 0m, 0f, 1u gpg: Good signature from "C O Mitter <committer@example.com>" [ultimate] ---- >8 ---- Whereas `git for-each-ref refs/tags/first-signed --format="%(signature)"` gives: ---- >8 ---- gpg: Warning: using insecure memory! gpg: Signature made Tue Aug 22 08:46:43 2023 UTC gpg: using DSA key 73D758744BE721698EC54E8713B6F51ECDDE430D gpg: issuer "committer@example.com" gpg: Good signature from "C O Mitter <committer@example.com>" [ultimate] ---- >8 ---- Running `head -3` on first output causes the warning to be included, but the issuer line to be removed. That is what finally differs between `expect` and `actual`. Just changing the number of lines brings other issues I guess... As far as I known the output on issuer was added recently with a gnupg release. So we need a set of commands to bring the output of both command in line, with or without warning on insecure memory.
Christian Hesse <list@eworm.de> on Tue, 2023/08/22 11:04: > So we need a set of commands to bring the output of both command in line, > with or without warning on insecure memory. I think I found a clean solution... Running a trustdb update earlier makes the extra lines go away, and we do not need to filter them. See the follow up...
Christian Hesse <list@eworm.de> writes: > Kousik Sanagavarapu <five231003@gmail.com> on Tue, 2023/08/22 13:24: >> Christian Hesse <list@eworm.de> wrote: >> >> > From: Christian Hesse <mail@eworm.de> >> > >> > Running the tests in a build environment makes gnupg print a warning: >> > >> > gpg: Warning: using insecure memory! >> > >> > This warning breaks the match, as `head` misses one line. Let's strip >> > the line, make `head` return what is expected and fix the match. >> > >> > Signed-off-by: Christian Hesse <mail@eworm.de> >> >> I think a bit of an explanation about why this warning is showing up in the >> commit message would be good. >> >> "man gpg" gives me <stripped> >> >> So it seems that this warning will pop up if gpg is writing memory pages to >> disk which is bad because as stated above we don't want these pages written >> to disk which is a security risk. > > The Arch Linux packages are built inside a clean container, started via > systemd-nspawn. Within the container the system call @memlock is not allowed > by default, for security reasons. Thanks for Kousik and Christian for discussing this. The phrase "in a build environment" in the proposed log message puzzled me, as the program does not seem to print such warning in my build environment. And environments where memlock is disabled are probably not limited to containers used to build Arch's packages. "in a build environment" -> "in an enviornment where memlock is disabled" would have avoided puzzling readers.
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 5b434ab451..0f9981798e 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1764,12 +1764,13 @@ test_expect_success GPGSSH 'setup for signature atom using ssh' ' test_expect_success GPG2 'bare signature atom' ' git verify-commit first-signed 2>out.raw && - grep -Ev "checking the trustdb|PGP trust model" out.raw >out && + grep -Ev "checking the trustdb|PGP trust model|using insecure memory" out.raw >out && head -3 out >expect && tail -1 out >>expect && echo >>expect && git for-each-ref refs/tags/first-signed \ - --format="%(signature)" >actual && + --format="%(signature)" >out.raw && + grep -Ev "using insecure memory" out.raw >actual && test_cmp expect actual '