git segfault in tag verify (patch included)

git segfault in tag verify (patch included)

[Steven Roberts]

Hi,

I believe I have found an off-by-one error in git.

Please see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2

Re: git segfault in tag verify (patch included)

[Junio C Hamano]

Steven Roberts <fenderq@gmail.com> writes:

> I believe I have found an off-by-one error in git.
>
> Please see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2

That is this thing.

        static void parse_gpg_output(struct signature_check *sigc)
        {
                const char *buf = sigc->gpg_status;
                const char *line, *next;
                int i, j;
                int seen_exclusive_status = 0;

                /* Iterate over all lines */
                for (line = buf; *line; line = strchrnul(line+1, '\n')) {
                        while (*line == '\n')
                                line++;
                        /* Skip lines that don't start with GNUPG status */
                        if (!skip_prefix(line, "[GNUPG:] ", &line))
                                continue;

If the GPG output ends with a trailing blank line, we skip and get
to the terminating NUL, then find that it does not begin with
the "[GNUPG:] " prefix, and hit the continue.  We try to scan and
look for LF (or stop at the end of the string) for the next round,
starting at one past where we are, which is already the terminating
NUL.  Ouch.

Good finding.

We need your sign-off (see Documentation/SubmittingPatches).

Thanks.


-- >8 --
From: Steven Roberts <fenderq@gmail.com>
Subject: gpg-interface: do not scan past the end of buffer

If the GPG output ends with trailing blank lines, after skipping
them over inside the loop to find the terminating NUL at the end,
the loop ends up looking for the next line, starting past the end.

---
 gpg-interface.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index 8ed274533f..eb55d46ea4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -116,6 +116,9 @@ static void parse_gpg_output(struct signature_check *sigc)
 	for (line = buf; *line; line = strchrnul(line+1, '\n')) {
 		while (*line == '\n')
 			line++;
+		if (!*line)
+			break;
+
 		/* Skip lines that don't start with GNUPG status */
 		if (!skip_prefix(line, "[GNUPG:] ", &line))
 			continue;

Re: git segfault in tag verify (patch included)

[Steven Roberts]

[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]

Thanks. I hope this works ok for you (see attached).

On Tue, Jul 16, 2019 at 11:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Steven Roberts <fenderq@gmail.com> writes:
>
> > I believe I have found an off-by-one error in git.
> >
> > Please see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2
>
> That is this thing.
>
>         static void parse_gpg_output(struct signature_check *sigc)
>         {
>                 const char *buf = sigc->gpg_status;
>                 const char *line, *next;
>                 int i, j;
>                 int seen_exclusive_status = 0;
>
>                 /* Iterate over all lines */
>                 for (line = buf; *line; line = strchrnul(line+1, '\n')) {
>                         while (*line == '\n')
>                                 line++;
>                         /* Skip lines that don't start with GNUPG status */
>                         if (!skip_prefix(line, "[GNUPG:] ", &line))
>                                 continue;
>
> If the GPG output ends with a trailing blank line, we skip and get
> to the terminating NUL, then find that it does not begin with
> the "[GNUPG:] " prefix, and hit the continue.  We try to scan and
> look for LF (or stop at the end of the string) for the next round,
> starting at one past where we are, which is already the terminating
> NUL.  Ouch.
>
> Good finding.
>
> We need your sign-off (see Documentation/SubmittingPatches).
>
> Thanks.
>
>
> -- >8 --
> From: Steven Roberts <fenderq@gmail.com>
> Subject: gpg-interface: do not scan past the end of buffer
>
> If the GPG output ends with trailing blank lines, after skipping
> them over inside the loop to find the terminating NUL at the end,
> the loop ends up looking for the next line, starting past the end.
>
> ---
>  gpg-interface.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8ed274533f..eb55d46ea4 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -116,6 +116,9 @@ static void parse_gpg_output(struct signature_check *sigc)
>         for (line = buf; *line; line = strchrnul(line+1, '\n')) {
>                 while (*line == '\n')
>                         line++;
> +               if (!*line)
> +                       break;
> +
>                 /* Skip lines that don't start with GNUPG status */
>                 if (!skip_prefix(line, "[GNUPG:] ", &line))
>                         continue;
>


-- 
Steven Roberts | https://www.fenderq.com/

[-- Attachment #2: 0001-gpg-interface-do-not-scan-past-the-end-of-buffer.patch --]
[-- Type: text/x-patch, Size: 837 bytes --]

From d48814273a50cf0b293148cc40a6a5cc7c13686e Mon Sep 17 00:00:00 2001
From: Steven Roberts <sroberts@fenderq.com>
Date: Tue, 16 Jul 2019 11:40:46 -0700
Subject: [PATCH] gpg-interface: do not scan past the end of buffer

Signed-off-by: Steven Roberts <sroberts@fenderq.com>
---
 gpg-interface.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index 8ed274533f..775475131d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -116,6 +116,11 @@ static void parse_gpg_output(struct signature_check *sigc)
 	for (line = buf; *line; line = strchrnul(line+1, '\n')) {
 		while (*line == '\n')
 			line++;
+
+		/* Break out of trailing '\n' */
+		if (!*line)
+			break;
+
 		/* Skip lines that don't start with GNUPG status */
 		if (!skip_prefix(line, "[GNUPG:] ", &line))
 			continue;
-- 
2.21.0

Re: git segfault in tag verify (patch included)

[Jeff King]

On Tue, Jul 16, 2019 at 11:20:48AM -0700, Junio C Hamano wrote:

> That is this thing.
> 
>         static void parse_gpg_output(struct signature_check *sigc)
>         {
>                 const char *buf = sigc->gpg_status;
>                 const char *line, *next;
>                 int i, j;
>                 int seen_exclusive_status = 0;
> 
>                 /* Iterate over all lines */
>                 for (line = buf; *line; line = strchrnul(line+1, '\n')) {
>                         while (*line == '\n')
>                                 line++;
>                         /* Skip lines that don't start with GNUPG status */
>                         if (!skip_prefix(line, "[GNUPG:] ", &line))
>                                 continue;
> 
> If the GPG output ends with a trailing blank line, we skip and get
> to the terminating NUL, then find that it does not begin with
> the "[GNUPG:] " prefix, and hit the continue.  We try to scan and
> look for LF (or stop at the end of the string) for the next round,
> starting at one past where we are, which is already the terminating
> NUL.  Ouch.
> 
> Good finding.

Yeah. The patch below looks fine, and I do not see any other
out-of-bounds issues in the code (there is a similar "next + 1" in the
inner loop, but it checks for an empty line right beforehand already).

I find these kind of "+1" pointer increments without constraint checking
are error-prone.  I suspect this whole loop could be a bit easier to
follow by finding the next line boundary at the start of the loop, and
jumping to it in the for-loop advancement. And then within the loop, we
know the boundaries of the line (right now, for example, we issue a
strchrnul() looking for a space that might unexpectedly cross line
boundaries).

Something like:

  for (line = buf; *line; line = next) {
	next = strchrnul(line, '\n');

	... do stuff ...
	/* find a space within the line */
	space = memchr(line, ' ', next - line);
  }

or even:

  for (line = buf; *line; line += len) {
	size_t len = strchrnul(line, '\n') - line;
	...
	space = memchr(line, ' ', linelen);
  }

but it may not be worth the churn. It's just as likely to introduce a
new bug. ;)

I've also found working with strbuf_getline() to be very pleasant for a
lot of these cases (in which you get a real per-line NUL-terminated
string), but of course it implies an extra copy.

-Peff

Re: git segfault in tag verify (patch included)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Something like:
>
>   for (line = buf; *line; line = next) {
> 	next = strchrnul(line, '\n');
>
> 	... do stuff ...
> 	/* find a space within the line */
> 	space = memchr(line, ' ', next - line);
>   }

I am not sure about the memchr() thing, but "prepare next that is
constant thru the iteration, and update line to it at the end of
each iteration" is a robust pattern to follow.  I like it.