Re: [PATCH v2 1/1] t6300: fix match with insecure memory

From: Christian Hesse <list@eworm.de>
Date: Aug 22, 2023, 09:04 UTC
Message-ID: <20230822110404.1c002dcf@leda.eworm.net>
In-Reply-To: <ZORpucPcjzm-dhjP@five231003>

Kousik Sanagavarapu <five231003@gmail.com> on Tue, 2023/08/22 13:24:

Show 21 quoted lines

> 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.

Show 39 quoted lines

> > ---
> >  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 ----

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.

-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}