Skip to content

strntol: fix out-of-bounds reads when parsing numbers with leading sign#4886

Merged
ethomson merged 2 commits intolibgit2:masterfrom
pks-t:pks/strntol-truncate-leading-sign
Nov 14, 2018
Merged

strntol: fix out-of-bounds reads when parsing numbers with leading sign#4886
ethomson merged 2 commits intolibgit2:masterfrom
pks-t:pks/strntol-truncate-leading-sign

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Nov 14, 2018

When parsing a number, we accept a leading plus or minus sign to return
a positive or negative number. When the parsed string has such a leading
sign, we set up a flag indicating that the number is negative and
advance the pointer to the next character in that string. This misses
updating the number of bytes in the string, though, which is why the
parser may later on do an out-of-bounds read.

Fix the issue by correctly updating both the pointer and the number of
remaining bytes. Furthermore, we need to check whether we actually have
any bytes left after having advanced the pointer, as otherwise the
auto-detection of the base may do an out-of-bonuds access. Add a test
that detects the out-of-bound read.

Note that this is not actually security critical. While there are a lot
of places where the function is called, all of these places are guarded
or irrelevant:

  • commit list: this operates on objects from the ODB, which are always
    NUL terminated any may thus not trigger the off-by-one OOB read.

  • config: the configuration is NUL terminated.

  • curl stream: user input is being parsed that is always NUL terminated

  • index: the index is read via git_futils_readbuffer, which always NUL
    terminates it.

  • loose objects: used to parse the length from the object's header. As
    we check previously that the buffer contains a NUL byte, this is safe.

  • rebase: this parses numbers from the rebase instruction sheet. As the
    rebase code uses git_futils_readbuffer, the buffer is always NUL
    terminated.

  • revparse: this parses a user provided buffer that is NUL terminated.

  • signature: this parser the header information of objects. As objects
    read from the ODB are always NUL terminated, this is a non-issue. The
    constructor git_signature_from_buffer does not accept a length
    parameter for the buffer, so the buffer needs to be NUL terminated, as
    well.

  • smart transport: the buffer that is parsed is NUL terminated

  • tree cache: this parses the tree cache from the index extension. The
    index itself is read via git_futils_readbuffer, which always NUL
    terminates it.

  • winhttp transport: user input is being parsed that is always NUL
    terminated

When parsing a number, we accept a leading plus or minus sign to return
a positive or negative number. When the parsed string has such a leading
sign, we set up a flag indicating that the number is negative and
advance the pointer to the next character in that string. This misses
updating the number of bytes in the string, though, which is why the
parser may later on do an out-of-bounds read.

Fix the issue by correctly updating both the pointer and the number of
remaining bytes. Furthermore, we need to check whether we actually have
any bytes left after having advanced the pointer, as otherwise the
auto-detection of the base may do an out-of-bonuds access. Add a test
that detects the out-of-bound read.

Note that this is not actually security critical. While there are a lot
of places where the function is called, all of these places are guarded
or irrelevant:

- commit list: this operates on objects from the ODB, which are always
  NUL terminated any may thus not trigger the off-by-one OOB read.

- config: the configuration is NUL terminated.

- curl stream: user input is being parsed that is always NUL terminated

- index: the index is read via `git_futils_readbuffer`, which always NUL
  terminates it.

- loose objects: used to parse the length from the object's header. As
  we check previously that the buffer contains a NUL byte, this is safe.

- rebase: this parses numbers from the rebase instruction sheet. As the
  rebase code uses `git_futils_readbuffer`, the buffer is always NUL
  terminated.

- revparse: this parses a user provided buffer that is NUL terminated.

- signature: this parser the header information of objects. As objects
  read from the ODB are always NUL terminated, this is a non-issue. The
  constructor `git_signature_from_buffer` does not accept a length
  parameter for the buffer, so the buffer needs to be NUL terminated, as
  well.

- smart transport: the buffer that is parsed is NUL terminated

- tree cache: this parses the tree cache from the index extension. The
  index itself is read via `git_futils_readbuffer`, which always NUL
  terminates it.

- winhttp transport: user input is being parsed that is always NUL
  terminated
The function `parse_number` was replaced by `git_parse_advance_digit`
which is provided by the parser interface in commit 252f2ee (parse:
implement and use `git_parse_advance_digit`, 2017-07-14). As there are
no remaining callers, remove it.
@ethomson
Copy link
Copy Markdown
Member

Appreciate the detailed analysis. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants