[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:5 teor]:
 > Replying to [comment:4 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.
 >
 > 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.
 >
 > > 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 assertion is intended to catch code which passes strings shorter
 than DIGEST_LEN to the function. (We will need to comment it to make this
 clear.)
 I agree the comments on the related functions need to be clearer regarding
 the expected buffer length.
 >
 > Buffers passed to the function should never contain a nul byte in the
 first DIGEST_LEN bytes. If they do, it's because someone passed a short
 string to the function. (As you've noticed, this is easy to do.)
 >
 > Since there's no way to find the length of the memory pointed to by a
 pointer in C, our best option is to look for evidence that someone passed
 in a short buffer.
 I agree with this, but i would like to do it in a way that doesn't leave
 the door open for other issues.
 >
 > > The solution IMO would be to require string length parameters (like
 the modern C string functions).
 >
 > The buffer is a set-length non-nul terminated block of memory.
 > Any string length parameter would always be DIGEST_LEN, so it's
 redundant. (And it would imply that the buffers need to be nul-terminated,
 which they don't.)
 I agree that it is redundant so maybe it is not a good solution, but it
 does not imply the buffers are null-terminated. On the contrary, it
 implies the buffer are not null-terminated hence why we need the length as
 a separate parameter.
 >
 > > 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.
 }}}

 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)

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