[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 cypherpunks):

 Replying to [comment:2 teor]:
 > Replying to [comment:1 cypherpunks]:
 > > The attached patches fixes the issues mentioned in the ticket
 description. I hope the commit messages speak for themselves. Rewriting
 them into the ticket seemed redundant.
 >
 > Code Review: Patches 1 & 2
 >
 > I agree that these buffer overruns need to be fixed.
 >
 > But what I'd like to do is change the functions that overrun the buffers
 so they don't overrun buffers if a short string is passed to them. That
 way, we fix the problem at the source.
 >
 > 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.

 The assertion implies that the digests are required to be null-terminated
 strings. This moves away from the requirement that they need to be
 `DIGEST_LEN` long and is something which could be ignored in a similar
 fashion.

 The solution IMO would be to require string length parameters (like the
 modern C string functions).

 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?

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