[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