[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: 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: |
-------------------------------+------------------------------------
Changes (by cypherpunks):
* owner: => cypherpunks
* status: needs_revision => assigned
Comment:
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`.
>
>
> > > > 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.
Only with `git log --reverse` if you use the CLI implementation.
>
> > 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.
Ok, i'll assign the ticket to this account then and implement these
changes.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17776#comment:8>
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