[tor-bugs] #17776 [Tor]: Buffer over-reads in directory and rendcache tests

[Tor Bug Tracker & Wiki]

#17776: Buffer over-reads in directory and rendcache tests
-------------------------------+------------------------------------
 Reporter:  cypherpunks        |          Owner:  cypherpunks
     Type:  defect             |         Status:  assigned
 Priority:  Medium             |      Milestone:  Tor: 0.2.8.x-final
Component:  Tor                |        Version:  Tor: 0.2.7.3-rc
 Severity:  Normal             |     Resolution:
 Keywords:  TorCoreTeam201512  |  Actual Points:
Parent ID:                     |         Points:
  Sponsor:                     |
-------------------------------+------------------------------------
Changes (by cypherpunks):

 * owner:   => cypherpunks
 * status:  needs_revision => assigned


Comment:

 Replying to [comment:7 teor]:
 > Replying to [comment:6 cypherpunks]:
 > > Replying to [comment:5 teor]:
 > > > Replying to [comment:4 cypherpunks]:
 > > > > Replying to [comment:2 teor]:
 > > > > > I want to check that the fingerprint strings are:
 > > > > > * not NULL, and
 > > > > > * don't contain a NULL character in the first DIGEST_LEN bytes?
 > > > > > before the functions read the strings?
 > > > > >
 > > > > > You can use code like:
 > > > > > {{{
 > > > > > tor_assert(fingerprint);
 > > > > > tor_assert(memchr(fingerprint, 0, DIGEST_LEN) == NULL);
 > > > > > }}}
 > > > > >
 > > > > > That would also require updating all the test data so it's
 really DIGEST_LEN characters long (and increasing the buffer lengths by 1
 to accommodate the terminating nul byte).
 > > > >
 > > > > The bugs were found using AddressSanitizer and i think the
 `memchr` solution will trigger similar warnings because the behavior is
 undefined if access occurs beyond the end of the array searched.
 > > >
 > > > memchr(fingerprint, 0, DIGEST_LEN) will never access any byte in
 fingerprint beyond a nul byte or DIGEST_LEN bytes. (Whichever comes
 first.)
 > > Unless the fingerprint isn't null-terminated and adjacent memory
 happens to contain random data with a null-byte exactly at DIGEST_LEN+1.
 This would both be undefined behavior because it over-reads and it would
 see the fingerprint as being of valid length which it isn't.
 >
 > This isn't how memchr works. It only ever reads "n" bytes, where "n" is
 its third argument. If we pass DIGEST_LEN for n, it will never read byte
 DIGEST_LEN + 1 under any circumstances. (But see my comments below for why
 this doesn't matter.)
 I believe my argument is still valid even when "n" is DIGEST_LEN (i did
 make a off-by-one error in my initial argument). Imagine if fingerprint
 has a length of 1 but that single character isn't a null-byte, `memchr`
 will then over-read until it finds a null-byte or has read DIGEST_LEN - 1
 bytes of adjacent memory. I'm not trying to beat a dead horse or be
 defensive, just trying to explain the issue with `memchr`.
 >
 >
 > > > > Replying to [comment:3 teor]:
 > > > > > Patches 1 & 2 are on unit tests introduced in 0.2.7.3-rc.
 > > > > > (The functions being tested are a lot older than that.)
 > > > >
 > > > > The commit messages of patches 1 and 2 include the commits which
 introduced the issues. According to `git describe --contains` both commits
 are not within in a tag. Is this correct?
 > > >
 > > > I don't know how this feature of git works.
 > > > I think we tag the commit that finalises a release. We don't tag all
 commits in a release.
 > > >
 > > > When I look for the release that contains a commit, I look for the
 first release commit after that commit in git log.
 > > After as in below it? Because that would mean the commit is not
 included in that release as `git log` usually goes back in time starting
 from the top. To quote the git manual on `git describe --contains`
 > > {{{
 > > --contains
 > >            Instead of finding the tag that predates the commit, find
 the tag
 > >            that comes after the commit, and thus contains it.
 > > }}}
 >
 > When I said "after", I meant "after in time", which is earlier in git
 log.
 Only with `git log --reverse` if you use the CLI implementation.
 >
 > > Finally, maybe i am overthinking the solution but i feel the proposed
 `memchr` assertion does not prevent all corner cases from occurring (as
 the first part of this reply explains). It introduces issues of its own.
 In search of other solutions i have looked at other parts of the Tor code
 base such as the `crypto_digest*` functions. These functions assume the
 digest buffer is large enough because they have no way of verifying it and
 all they do is assert the digest pointer isn't `NULL`. I know these are
 write operations on a buffer instead of read operations on a buffer, but
 the core question is the same; what if the buffer is too short? These
 functions do not care.
 > >
 > > With this knowledge i like to update the proposed changes to be
 > > * assert the pointer to the buffer isn't NULL (same as proposed by
 teor)
 > > * do not assert that the buffer is too short (there is no proper way
 to do this)
 > > * update the comments on the related functions that the buffer should
 be DIGEST_LEN long (as there is no proper way to enforce it otherwise)
 >
 > That sounds like a good idea. Let's do that.
 Ok, i'll assign the ticket to this account then and implement these
 changes.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17776#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online