1. 25 Jan, 2019 1 commit
    • strntol: fix out-of-bounds read when skipping leading spaces · af9692ba
      The `git__strntol` family of functions accepts leading spaces and will
      simply skip them. The skipping will not honor the provided buffer's
      length, though, which may lead it to read outside of the provided
      buffer's bounds if it is not a simple NUL-terminated string.
      Furthermore, if leading space is trimmed, the function will further
      advance the pointer but not update the number of remaining bytes, which
      may also lead to out-of-bounds reads.
      
      Fix the issue by properly paying attention to the buffer length and
      updating it when stripping leading whitespace characters. Add a test
      that verifies that we won't read past the provided buffer length.
      Patrick Steinhardt committed
  2. 18 Jan, 2019 15 commits
    • signature: fix out-of-bounds read when parsing timezone offset · 6daeb4fb
      When parsing a signature's timezone offset, we first check whether there
      is a timezone at all by verifying that there are still bytes left to
      read following the time itself. The check thus looks like `time_end + 1
      < buffer_end`, which is actually correct in this case. After setting the
      timezone's start pointer to that location, we compute the remaining
      bytes by using the formula `buffer_end - tz_start + 1`, re-using the
      previous `time_end + 1`. But this is in fact missing the braces around
      `(tz_start + 1)`, thus leading to an overestimation of the remaining
      bytes by a length of two. In case of a non-NUL terminated buffer, this
      will result in an overflow.
      
      The function `git_signature__parse` is only used in two locations. First
      is `git_signature_from_buffer`, which only accepts a string without a
      length. The string thus necessarily has to be NUL terminated and cannot
      trigger the issue.
      
      The other function is `git_commit__parse_raw`, which can in fact trigger
      the error as it may receive non-NUL terminated commit data. But as
      objects read from the ODB are always NUL-terminated by us as a
      cautionary measure, it cannot trigger the issue either.
      
      In other words, this error does not have any impact on security.
      Patrick Steinhardt committed
    • tests: address two null argument instances · 98a6d9d5
      Handle two null argument cases that occur in the unit tests.
      One is in library code, the other is in test code.
      
      Detected by running unit tests with undefined behavior sanitizer:
      ```bash
       # build
      mkdir build && cd build
      cmake -DBUILD_CLAR=ON -DCMAKE_C_FLAGS="-fsanitize=address \
      -fsanitize=undefined -fstack-usage -static-libasan" ..
      cmake --build .
      
       # run with asan
      ASAN_OPTIONS="allocator_may_return_null=1" ./libgit2_clar
      ...
      ............../libgit2/src/apply.c:316:3: runtime error: null pointer \
      passed as argument 1, which is declared to never be null
      ...................../libgit2/tests/apply/fromfile.c:46:3: runtime \
      error: null pointer passed as argument 1, which is declared to never be null
      ```
      Noah Pendleton committed
    • commit: fix out-of-bound reads when parsing truncated author fields · 7529124b
      While commit objects usually should have only one author field, our commit
      parser actually handles the case where a commit has multiple author fields
      because some tools that exist in the wild actually write them. Detection of
      those additional author fields is done by using a simple `git__prefixcmp`,
      checking whether the current line starts with the string "author ". In case
      where we are handed a non-NUL-terminated string that ends directly after the
      space, though, we may have an out-of-bounds read of one byte when trying to
      compare the expected final NUL byte.
      
      Fix the issue by using `git__prefixncmp` instead of `git_prefixcmp`.
      Unfortunately, a test cannot be easily written to catch this case. While we
      could test the last error message and verify that it didn't in fact fail parsing
      a signature (because that would indicate that it has in fact tried to parse the
      additional "author " field, which it shouldn't be able to detect in the first
      place), this doesn't work as the next line needs to be the "committer" field,
      which would error out with the same error message even if we hadn't done an
      out-of-bounds read.
      
      As objects read from the object database are always NUL terminated, this issue
      cannot be triggered in normal code and thus it's not security critical.
      Patrick Steinhardt committed
    • config: fix adding files if their parent directory is a file · d04c1aa0
      When we try to add a configuration file with `git_config_add_file_ondisk`, we
      treat nonexisting files as empty. We do this by performing a stat call, ignoring
      ENOENT errors. This works just fine in case the file or any of its parents
      simply does not exist, but there is also the case where any of the parent
      directories is not a directory, but a file. So e.g. trying to add a
      configuration file "/dev/null/.gitconfig" will fail, as `errno` will be ENOTDIR
      instead of ENOENT.
      
      Catch ENOTDIR in addition to ENOENT to fix the issue. Add a test that verifies
      we are able to add configuration files with such an invalid path file just fine.
      Patrick Steinhardt committed
    • Typesetting conventions · ac5879de
      Joe Rabinoff committed
    • Removed one null check · a6ffb1ce
      Joe Rabinoff committed
    • Fix segfault in loose_backend__readstream · 82209544
      If the routine exits with error before stream or hash_ctx is initialized, the
      program will segfault when trying to free them.
      Joe Rabinoff committed
    • make proxy_stream_close close target stream even on errors · c3b2053d
      When git_filter_apply_fn callback returns a error while smudging proxy_stream_close
      ends up returning without closing the stream. This is turn makes blob_content_to_file
      crash as it asserts the stream being closed whether there are errors or not.
      
      Closing the target stream on error fixes this problem.
      Anders Borum committed
    • annotated_commit: peel to commit instead of assuming we have one · 8ce7272d
      We want to allow the creation of annotated commits out of annotated tags and for
      that we have to peel the reference all the way to the commit instead of stopping
      at the first id it provides.
      Carlos Martín Nieto committed
    • refs: constify git_reference_peel · 0573e78d
      We have no need to take a non-const reference. This does involve some other work
      to make sure we don't mix const and non-const variables, but by splitting what
      we want each variable to do we can also simplify the logic for when we do want
      to free a new reference we might have allocated.
      Carlos Martín Nieto committed
    • config: assert that our parameters are valid · 42c4c624
      CID 1395011
      Etienne Samson committed
    • diff: assert that we're passed a valid git_diff object · c7469503
      CID 1386176, 1386177, 1388219
      Etienne Samson committed
    • submodule: grab the error while loading from config · a4b332e8
      Previously, an error in `git_config_next` would be mistaken as a
      successful load, because the previous call would have succeeded.
      Coverity saw the subsequent check for a completed iteration as dead, so
      let's make it useful again.
      
      CID 1391374
      Etienne Samson committed
  3. 19 Dec, 2018 2 commits
    • Merge pull request #4916 from libgit2/ethomson/backport_0278 · 313440c3
      smart transport: only clear url on hard reset
      Edward Thomson committed
    • smart transport: only clear url on hard reset · 8bc913a2
      After creating a transport for a server, we expect to be able to call
      `connect`, then invoke subsequent `action` calls.  We provide the URL to
      these `action` calls, although our built-in transports happen to ignore
      it since they've already parsed it into an internal format that they
      intend to use (`gitno_connection_data`).
      
      In ca2eb460, we began clearing the URL
      field after a connection, meaning that subsequent calls to transport
      `action` callbacks would get a NULL URL, which went undetected since the
      builtin transports ignore the URL when they're already connected
      (instead of re-parsing it into an internal format).
      
      Downstream custom transport implementations (eg, LibGit2Sharp) did
      notice this change, however.
      
      Since `reset_stream` is called even when we're not closing the
      subtransport, update to only clear the URL when we're closing the
      subtransport.  This ensures that `action` calls will get the correct URL
      information even after a connection.
      Edward Thomson committed
  4. 26 Oct, 2018 22 commits