tinc devel - Feb 2018 - tinc 1.1: freeaddrinfo(NULL) crash on windows

On Mon, 22 Jan 2018 18:08:22 +0100, Guus Sliepen wrote:
> Thanks for reporting this bug! Indeed, that code is wrong. It should
> free cache->ai instead of cache->aip. Even if it didn't crash on
Linux,
> it was a memory leak. A fix is now in the git repository.
The fix leads to a use after free bug which is visible on macOS.
It doesn't seem to be much of a problem on most platforms since a
copy of freed memory is made almost immediately but on macOS the
memory appears to be cleared much of the time.  This results in an
attempt to connect to an AF_UNSPEC socket with no address, which
of course fails.

A potential fix, modeled on the "recently seen addresses not in our
cache" part of get_recent_address() is:

    https://github.com/gsliepen/tinc/pull/177

Another approach would be to pass the sockaddr_t to get_recent_address()
and fill it in there.  I can write a diff for that if you'd like.

I also noticed a problem with the recently seen but uncached code
which was hidden by a cast as well as a buffer overflow (read, not
write).  The PR includes fixes for these two as well.

 - todd

On Fri, Feb 16, 2018 at 02:26:36PM -0700, Todd C. Miller wrote:
> > Thanks for reporting this bug! Indeed, that code is wrong. It should
> > free cache->ai instead of cache->aip. Even if it didn't
crash on Linux,
> > it was a memory leak. A fix is now in the git repository.
> 
> The fix leads to a use after free bug which is visible on macOS.
> It doesn't seem to be much of a problem on most platforms since a
> copy of freed memory is made almost immediately but on macOS the
> memory appears to be cleared much of the time.  This results in an
> attempt to connect to an AF_UNSPEC socket with no address, which
> of course fails.
Reading freed memory is always a bug.
> A potential fix, modeled on the "recently seen addresses not in our
> cache" part of get_recent_address() is:
> 
>     https://github.com/gsliepen/tinc/pull/177
Thanks again for providing a patch!
> Another approach would be to pass the sockaddr_t to get_recent_address()
> and fill it in there.  I can write a diff for that if you'd like.
The fix in the pull request looks fine, I don't think another approach
is better.

I'll have to invest some time in improving the test suite, and using
code coverage checking to ensure the test suite covers all everything
(if that's possible). Then the test suite can be run with
valgrind/asan/ubsan/etc. to prevent bugs like this from being committed
in the first place.

-- 
Met vriendelijke groet / with kind regards,
     Guus Sliepen <guus at tinc-vpn.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20180217/5ebb5dde/attachment.sig>

Unfortunately, my fix exposed a problem in reset_address_cache()
could call free_known_addresses() on a struct addrinfo * that was
returned by getaddrinfo().  This results in heap corruption on
Windows (and a crash).

It seems safest to just make a copy of the addresses returned by
getaddrinfo() so it can always use free_known_addresses() instead
of trying to determine whether or not we need to use freeaddrinfo().

https://github.com/gsliepen/tinc/pull/178

 - todd