[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:                     |
-------------------------------+------------------------------------

Comment (by teor):

 Replying to [comment:8 cypherpunks]:
 > 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`.

 I think we agree about how memchr works, and that it isn't a perfect
 solution.

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