[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 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.
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 solution IMO would be to require string length parameters (like the
modern C string functions).
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?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17776#comment:4>
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