[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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



#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: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.)

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

 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.

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

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

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17776#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs