[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:
     Type:  defect             |         Status:  needs_revision
 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:                     |
-------------------------------+------------------------------------

Comment (by 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.)


 > > > 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.

 > 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.

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