1. 26 Oct, 2018 20 commits
    • commit: fix reading out of bounds when parsing encoding · 4f0e5f70
      The commit message encoding is currently being parsed by the
      `git__prefixcmp` function. As this function does not accept a buffer
      length, it will happily skip over a buffer's end if it is not `NUL`
      terminated.
      
      Fix the issue by using `git__prefixncmp` instead. Add a test that
      verifies that we are unable to parse the encoding field if it's cut off
      by the supplied buffer length.
      
      (cherry picked from commit 7655b2d8)
      Patrick Steinhardt committed
    • tag: fix out of bounds read when searching for tag message · 6e40bb3a
      When parsing tags, we skip all unknown fields that appear before the tag
      message. This skipping is done by using a plain `strstr(buffer, "\n\n")`
      to search for the two newlines that separate tag fields from tag
      message. As it is not possible to supply a buffer length to `strstr`,
      this call may skip over the buffer's end and thus result in an out of
      bounds read. As `strstr` may return a pointer that is out of bounds, the
      following computation of `buffer_end - buffer` will overflow and result
      in an allocation of an invalid length.
      
      Fix the issue by using `git__memmem` instead. Add a test that verifies
      parsing the tag fails not due to the allocation failure but due to the
      tag having no message.
      
      (cherry picked from commit ee11d47e)
      Patrick Steinhardt committed
    • util: provide `git__memmem` function · ab0d9fb2
      Unfortunately, neither the `memmem` nor the `strnstr` functions are part
      of any C standard but are merely extensions of C that are implemented by
      e.g. glibc. Thus, there is no standardized way to search for a string in
      a block of memory with a limited size, and using `strstr` is to be
      considered unsafe in case where the buffer has not been sanitized. In
      fact, there are some uses of `strstr` in exactly that unsafe way in our
      codebase.
      
      Provide a new function `git__memmem` that implements the `memmem`
      semantics. That is in a given haystack of `n` bytes, search for the
      occurrence of a byte sequence of `m` bytes and return a pointer to the
      first occurrence. The implementation chosen is the "Not So Naive"
      algorithm from [1]. It was chosen as the implementation is comparably
      simple while still being reasonably efficient in most cases.
      Preprocessing happens in constant time and space, searching has a time
      complexity of O(n*m) with a slightly sub-linear average case.
      
      [1]: http://www-igm.univ-mlv.fr/~lecroq/string/
      
      (cherry picked from commit 83e8a6b3)
      Patrick Steinhardt committed
    • util: fix out of bounds read in error message · c8e94f84
      When an integer that is parsed with `git__strntol32` is too big to fit
      into an int32, we will generate an error message that includes the
      actual string that failed to parse. This does not acknowledge the fact
      that the string may either not be NUL terminated or alternative include
      additional characters after the number that is to be parsed. We may thus
      end up printing characters into the buffer that aren't the number or,
      worse, read out of bounds.
      
      Fix the issue by utilizing the `endptr` that was set by
      `git__strntol64`. This pointer is guaranteed to be set to the first
      character following the number, and we can thus use it to compute the
      width of the number that shall be printed. Create a test to verify that
      we correctly truncate the number.
      
      (cherry picked from commit ea19efc1)
      Patrick Steinhardt committed
    • util: avoid signed integer overflows in `git__strntol64` · 0f7663a1
      While `git__strntol64` tries to detect integer overflows when doing the
      necessary arithmetics to come up with the final result, it does the
      detection only after the fact. This check thus relies on undefined
      behavior of signed integer overflows. Fix this by instead checking
      up-front whether the multiplications or additions will overflow.
      
      Note that a detected overflow will not cause us to abort parsing the
      current sequence of digits. In the case of an overflow, previous
      behavior was to still set up the end pointer correctly to point to the
      first character immediately after the currently parsed number. We do not
      want to change this now as code may rely on the end pointer being set up
      correctly even if the parsed number is too big to be represented as
      64 bit integer.
      
      (cherry picked from commit b09c1c7b)
      Patrick Steinhardt committed
    • tests: core::strtol: test for some more edge-cases · 55f4f235
      Some edge cases were currently completely untested, e.g. parsing numbers
      greater than INT64_{MIN,MAX}, truncating buffers by length and invalid
      characters. Add tests to verify that the system under test performs as
      expected.
      
      (cherry picked from commit 39087ab8)
      Patrick Steinhardt committed
    • util: remove `git__strtol32` · e2e7a9cb
      The function `git__strtol32` can easily be misused when untrusted data
      is passed to it that may not have been sanitized with trailing `NUL`
      bytes. As all usages of this function have now been removed, we can
      remove this function altogether to avoid future misuse of it.
      
      (cherry picked from commit 8d7fa88a)
      Patrick Steinhardt committed
    • global: replace remaining use of `git__strtol32` · 2052a3cd
      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.
      
      (cherry picked from commit 2613fbb2)
      Patrick Steinhardt committed
    • tree-cache: avoid out-of-bound reads when parsing trees · 61165dd4
      We use the `git__strtol32` function to parse the child and entry count
      of treecaches from the index, which do not accept a buffer length. As
      the buffer that is being passed in is untrusted data and may thus be
      malformed and may not contain a terminating `NUL` byte, we can overrun
      the buffer and thus perform an out-of-bounds read.
      
      Fix the issue by uzing `git__strntol32` instead.
      
      (cherry picked from commit 21652ee9)
      Patrick Steinhardt committed
    • util: remove unsafe `git__strtol64` function · 6b2b63e5
      The function `git__strtol64` does not take a maximum buffer length as
      parameter. This has led to some unsafe usages of this function, and as
      such we may consider it as being unsafe to use. As we have now
      eradicated all usages of this function, let's remove it completely to
      avoid future misuse.
      
      (cherry picked from commit 68deb2cc)
      Patrick Steinhardt committed
    • config: remove last instance of `git__strntol64` · 334e6c69
      When parsing integers from configuration values, we use `git__strtol64`.
      This is fine to do, as we always sanitize values and can thus be sure
      that they'll have a terminating `NUL` byte. But as this is the last
      call-site of `git__strtol64`, let's just pass in the length explicitly
      by calling `strlen` on the value to be able to remove `git__strtol64`
      altogether.
      
      (cherry picked from commit 1a2efd10)
      Patrick Steinhardt committed
    • signature: avoid out-of-bounds reads when parsing signature dates · 5ce26b18
      We use `git__strtol64` and `git__strtol32` to parse the trailing commit
      or author date and timezone of signatures. As signatures are usually
      part of a commit or tag object and thus essentially untrusted data, the
      buffer may be misformatted and may not be `NUL` terminated. This may
      lead to an out-of-bounds read.
      
      Fix the issue by using `git__strntol64` and `git__strntol32` instead.
      
      (cherry picked from commit 3db9aa6f)
      Patrick Steinhardt committed
    • index: avoid out-of-bounds read when reading reuc entry stage · 76e57b4e
      We use `git__strtol64` to parse file modes of the index entries, which
      does not limit the parsed buffer length. As the index can be essentially
      treated as "untrusted" in that the data stems from the file system, it
      may be misformatted and may not contain terminating `NUL` bytes. This
      may lead to out-of-bounds reads when trying to parse index entries with
      such malformatted modes.
      
      Fix the issue by using `git__strntol64` instead.
      
      (cherry picked from commit 600ceadd)
      Patrick Steinhardt committed
    • commit_list: avoid use of strtol64 without length limit · 4dddcafb
      When quick-parsing a commit, we use `git__strtol64` to parse the
      commit's time. The buffer that's passed to `commit_quick_parse` is the
      raw data of an ODB object, though, whose data may not be properly
      formatted and also does not have to be `NUL` terminated. This may lead
      to out-of-bound reads.
      
      Use `git__strntol64` to avoid this problem.
      
      (cherry picked from commit 1a3fa1f5)
      Patrick Steinhardt committed
    • online::clone: free url and username before resetting · 54c02f26
      Before resetting the url and username, ensure that we free them in case
      they were set by environment variables.
      
      (cherry picked from commit e84914fd)
      Edward Thomson committed
    • push tests: deeply free the specs · 27469ed3
      Don't just free the spec vector, also free the specs themselves.
      
      (cherry picked from commit d285de73)
      Edward Thomson committed
    • push tests: deeply free the push status · c8826499
      Don't just free the push status structure, actually free the strings that were
      strdup'd into the struct as well.
      
      (cherry picked from commit dad99881)
      Edward Thomson committed
    • smart subtransport: free url when resetting stream · 1c02b896
      Free the url field when resetting the stream to avoid leaking it.
      
      (cherry picked from commit ca2eb460)
      Edward Thomson committed
    • ci: fail on test failures · a20b8c21
      PowerShell can _read_ top-level variables in functions, but cannot _update_
      top-level variables in functions unless they're explicitly prefixed with
      `$global`.
      
      (cherry picked from commit 0e26717a)
      Edward Thomson committed
  2. 12 Oct, 2018 20 commits