<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/libgit2.git/src/transports/smart_pkt.c, branch ethomson/test_https</title>
<subtitle>github.com: libgit2/libgit2.git
</subtitle>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/'/>
<entry>
<title>str: introduce `git_str` for internal, `git_buf` is external</title>
<updated>2021-10-17T13:49:01+00:00</updated>
<author>
<name>Edward Thomson</name>
<email>ethomson@edwardthomson.com</email>
</author>
<published>2021-09-07T21:53:49+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=f0e693b18afbe1de37d7da5b5a8967b6c87d8e53'/>
<id>f0e693b18afbe1de37d7da5b5a8967b6c87d8e53</id>
<content type='text'>
libgit2 has two distinct requirements that were previously solved by
`git_buf`.  We require:

1. A general purpose string class that provides a number of utility APIs
   for manipulating data (eg, concatenating, truncating, etc).
2. A structure that we can use to return strings to callers that they
   can take ownership of.

By using a single class (`git_buf`) for both of these purposes, we have
confused the API to the point that refactorings are difficult and
reasoning about correctness is also difficult.

Move the utility class `git_buf` to be called `git_str`: this represents
its general purpose, as an internal string buffer class.  The name also
is an homage to Junio Hamano ("gitstr").

The public API remains `git_buf`, and has a much smaller footprint.  It
is generally only used as an "out" param with strict requirements that
follow the documentation.  (Exceptions exist for some legacy APIs to
avoid breaking callers unnecessarily.)

Utility functions exist to convert a user-specified `git_buf` to a
`git_str` so that we can call internal functions, then converting it
back again.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
libgit2 has two distinct requirements that were previously solved by
`git_buf`.  We require:

1. A general purpose string class that provides a number of utility APIs
   for manipulating data (eg, concatenating, truncating, etc).
2. A structure that we can use to return strings to callers that they
   can take ownership of.

By using a single class (`git_buf`) for both of these purposes, we have
confused the API to the point that refactorings are difficult and
reasoning about correctness is also difficult.

Move the utility class `git_buf` to be called `git_str`: this represents
its general purpose, as an internal string buffer class.  The name also
is an homage to Junio Hamano ("gitstr").

The public API remains `git_buf`, and has a much smaller footprint.  It
is generally only used as an "out" param with strict requirements that
follow the documentation.  (Exceptions exist for some legacy APIs to
avoid breaking callers unnecessarily.)

Utility functions exist to convert a user-specified `git_buf` to a
`git_str` so that we can call internal functions, then converting it
back again.
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: fix overflow resulting in OOB read/write of one byte</title>
<updated>2019-12-13T11:21:17+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2019-12-13T11:13:05+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=868526138e131601c69f0949c58976700501881f'/>
<id>868526138e131601c69f0949c58976700501881f</id>
<content type='text'>
When parsing OK packets, we copy any information after the initial "ok "
prefix into the resulting packet. As newlines act as packet boundaries,
we also strip the trailing newline if there is any. We do not check
whether there is any data left after the initial "ok " prefix though,
which leads to a pointer overflow in that case as `len == 0`:

	if (line[len - 1] == '\n')
		--len;

This out-of-bounds read is a rather useless gadget, as we can only
deduce whether at some offset there is a newline character. In case
there accidentally is one, we overflow `len` to `SIZE_MAX` and then
write a NUL byte into an array indexed by it:

	pkt-&gt;ref[len] = '\0';

Again, this doesn't seem like something that's possible to be exploited
in any meaningful way, but it may surely lead to inconsistencies or DoS.

Fix the issue by checking whether there is any trailing data after the
packet prefix.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
When parsing OK packets, we copy any information after the initial "ok "
prefix into the resulting packet. As newlines act as packet boundaries,
we also strip the trailing newline if there is any. We do not check
whether there is any data left after the initial "ok " prefix though,
which leads to a pointer overflow in that case as `len == 0`:

	if (line[len - 1] == '\n')
		--len;

This out-of-bounds read is a rather useless gadget, as we can only
deduce whether at some offset there is a newline character. In case
there accidentally is one, we overflow `len` to `SIZE_MAX` and then
write a NUL byte into an array indexed by it:

	pkt-&gt;ref[len] = '\0';

Again, this doesn't seem like something that's possible to be exploited
in any meaningful way, but it may surely lead to inconsistencies or DoS.

Fix the issue by checking whether there is any trailing data after the
packet prefix.
</pre>
</div>
</content>
</entry>
<entry>
<title>git_error: use new names in internal APIs and usage</title>
<updated>2019-01-22T22:30:35+00:00</updated>
<author>
<name>Edward Thomson</name>
<email>ethomson@edwardthomson.com</email>
</author>
<published>2018-12-27T19:47:34+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=f673e232afe22eb865cdc915e55a2df6493f0fbb'/>
<id>f673e232afe22eb865cdc915e55a2df6493f0fbb</id>
<content type='text'>
Move to the `git_error` name in the internal API for error-related
functions.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Move to the `git_error` name in the internal API for error-related
functions.
</pre>
</div>
</content>
</entry>
<entry>
<title>global: replace remaining use of `git__strtol32`</title>
<updated>2018-10-18T09:58:14+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-10-18T09:58:14+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=2613fbb26a3e1a34dda8a5d198c108626cfd6cc3'/>
<id>2613fbb26a3e1a34dda8a5d198c108626cfd6cc3</id>
<content type='text'>
Replace remaining uses of the `git__strtol32` function. While these uses
are all safe as the strings were either sanitized or from a trusted
source, we want to remove `git__strtol32` altogether to avoid future
misuse.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Replace remaining uses of the `git__strtol32` function. While these uses
are all safe as the strings were either sanitized or from a trusted
source, we want to remove `git__strtol32` altogether to avoid future
misuse.
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: do not accept callers passing in no line length</title>
<updated>2018-10-03T14:17:21+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-10-03T14:17:21+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=1bc5b05c614c7b10de021fa392943e8e6bd12c77'/>
<id>1bc5b05c614c7b10de021fa392943e8e6bd12c77</id>
<content type='text'>
Right now, we simply ignore the `linelen` parameter of
`git_pkt_parse_line` in case the caller passed in zero. But in fact, we
never want to assume anything about the provided buffer length and
always want the caller to pass in the available number of bytes.
And in fact, checking all the callers, one can see that the funciton is
never being called in case where the buffer length is zero, and thus we
are safe to remove this check.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Right now, we simply ignore the `linelen` parameter of
`git_pkt_parse_line` in case the caller passed in zero. But in fact, we
never want to assume anything about the provided buffer length and
always want the caller to pass in the available number of bytes.
And in fact, checking all the callers, one can see that the funciton is
never being called in case where the buffer length is zero, and thus we
are safe to remove this check.
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: return parsed length via out-parameter</title>
<updated>2018-10-03T14:09:38+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-08-09T09:16:15+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=c05790a8a8dd4351e61fc06c0a06c6a6fb6134dc'/>
<id>c05790a8a8dd4351e61fc06c0a06c6a6fb6134dc</id>
<content type='text'>
The `parse_len` function currently directly returns the parsed length of
a packet line or an error code in case there was an error. Instead,
convert this to our usual style of using the return value as error code
only and returning the actual value via an out-parameter. Thus, we can
now convert the output parameter to an unsigned type, as the size of a
packet cannot ever be negative.

While at it, we also move the check whether the input buffer is long
enough into `parse_len` itself. We don't really want to pass around
potentially non-NUL-terminated buffers to functions without also passing
along the length, as this is dangerous in the unlikely case where other
callers for that function get added. Note that we need to make sure
though to not mess with `GIT_EBUFS` error codes, as these indicate not
an error to the caller but that he needs to fetch more data.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The `parse_len` function currently directly returns the parsed length of
a packet line or an error code in case there was an error. Instead,
convert this to our usual style of using the return value as error code
only and returning the actual value via an out-parameter. Thus, we can
now convert the output parameter to an unsigned type, as the size of a
packet cannot ever be negative.

While at it, we also move the check whether the input buffer is long
enough into `parse_len` itself. We don't really want to pass around
potentially non-NUL-terminated buffers to functions without also passing
along the length, as this is dangerous in the unlikely case where other
callers for that function get added. Note that we need to make sure
though to not mess with `GIT_EBUFS` error codes, as these indicate not
an error to the caller but that he needs to fetch more data.
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: reorder and rename parameters of `git_pkt_parse_line`</title>
<updated>2018-10-03T14:09:38+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-08-09T09:13:59+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=0b3dfbf425d689101663341beb94237614f1b5c2'/>
<id>0b3dfbf425d689101663341beb94237614f1b5c2</id>
<content type='text'>
The parameters of the `git_pkt_parse_line` function are quite confusing.
First, there is no real indicator what the `out` parameter is actually
all about, and it's not really clear what the `bufflen` parameter refers
to. Reorder and rename the parameters to make this more obvious.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The parameters of the `git_pkt_parse_line` function are quite confusing.
First, there is no real indicator what the `out` parameter is actually
all about, and it's not really clear what the `bufflen` parameter refers
to. Reorder and rename the parameters to make this more obvious.
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: fix buffer overflow when parsing "unpack" packets</title>
<updated>2018-10-03T14:09:38+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-08-09T09:04:42+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=5fabaca801e1f5e7a1054be612e8fabec7cd6a7f'/>
<id>5fabaca801e1f5e7a1054be612e8fabec7cd6a7f</id>
<content type='text'>
When checking whether an "unpack" packet returned the "ok" status or
not, we use a call to `git__prefixcmp`. In case where the passed line
isn't properly NUL terminated, though, this may overrun the line buffer.
Fix this by using `git__prefixncmp` instead.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
When checking whether an "unpack" packet returned the "ok" status or
not, we use a call to `git__prefixcmp`. In case where the passed line
isn't properly NUL terminated, though, this may overrun the line buffer.
Fix this by using `git__prefixncmp` instead.
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: fix "ng" parser accepting non-space character</title>
<updated>2018-10-03T14:09:38+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-08-09T09:03:37+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=b5ba7af2d30c958b090dcf135749d9afe89ec703'/>
<id>b5ba7af2d30c958b090dcf135749d9afe89ec703</id>
<content type='text'>
When parsing "ng" packets, we blindly assume that the character
immediately following the "ng" prefix is a space and skip it. As the
calling function doesn't make sure that this is the case, we can thus
end up blindly accepting an invalid packet line.

Fix the issue by using `git__prefixncmp`, checking whether the line
starts with "ng ".
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
When parsing "ng" packets, we blindly assume that the character
immediately following the "ng" prefix is a space and skip it. As the
calling function doesn't make sure that this is the case, we can thus
end up blindly accepting an invalid packet line.

Fix the issue by using `git__prefixncmp`, checking whether the line
starts with "ng ".
</pre>
</div>
</content>
</entry>
<entry>
<title>smart_pkt: fix buffer overflow when parsing "ok" packets</title>
<updated>2018-10-03T14:09:02+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-08-09T09:01:00+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=a9f1ca09178af0640963e069a2142d5ced53f0b4'/>
<id>a9f1ca09178af0640963e069a2142d5ced53f0b4</id>
<content type='text'>
There are two different buffer overflows present when parsing "ok"
packets. First, we never verify whether the line already ends after
"ok", but directly go ahead and also try to skip the expected space
after "ok". Second, we then go ahead and use `strchr` to scan for the
terminating newline character. But in case where the line isn't
terminated correctly, this can overflow the line buffer.

Fix the issues by using `git__prefixncmp` to check for the "ok " prefix
and only checking for a trailing '\n' instead of using `memchr`. This
also fixes the issue of us always requiring a trailing '\n'.

Reported by oss-fuzz, issue 9749:

Crash Type: Heap-buffer-overflow READ {*}
Crash Address: 0x6310000389c0
Crash State:
  ok_pkt
  git_pkt_parse_line
  git_smart__store_refs

Sanitizer: address (ASAN)
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
There are two different buffer overflows present when parsing "ok"
packets. First, we never verify whether the line already ends after
"ok", but directly go ahead and also try to skip the expected space
after "ok". Second, we then go ahead and use `strchr` to scan for the
terminating newline character. But in case where the line isn't
terminated correctly, this can overflow the line buffer.

Fix the issues by using `git__prefixncmp` to check for the "ok " prefix
and only checking for a trailing '\n' instead of using `memchr`. This
also fixes the issue of us always requiring a trailing '\n'.

Reported by oss-fuzz, issue 9749:

Crash Type: Heap-buffer-overflow READ {*}
Crash Address: 0x6310000389c0
Crash State:
  ok_pkt
  git_pkt_parse_line
  git_smart__store_refs

Sanitizer: address (ASAN)
</pre>
</div>
</content>
</entry>
</feed>
