[tor-bugs] #17776 [Tor]: Buffer over-reads in directory and rendcache tests
Tor Bug Tracker & Wiki
blackhole at torproject.org
Wed Dec 9 10:58:09 UTC 2015
#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
More information about the tor-bugs
mailing list