So, back long ago, XXX012 meant, "before Tor 0.1.2 is released, we
had better revisit this comment and fix it!"
But we have a huge pile of such comments accumulated for a large
number of released versions! Not cool.
So, here's what I tried to do:
* 0.2.9 and 0.2.8 are retained, since those are not yet released.
* XXX+ or XXX++ or XXX++++ or whatever means, "This one looks
quite important!"
* The others, after one-by-one examination, are downgraded to
plain old XXX. Which doesn't mean they aren't a problem -- just
that they cannot possibly be a release-blocking problem.
There are a few places where we want to disable a warning: for
example, when it's impossible to call a legacy API without
triggering it, or when it's impossible to include an external header
without triggering it.
This pile of macros uses GCC's c99 _Pragma support, plus the usual
macro trickery, to enable and disable warnings.
If OpenSSL fails to generate an RSA key, do not retain a dangling
pointer to the previous (uninitialized) key value. The impact here
should be limited to a difficult-to-trigger crash, if OpenSSL is
running an engine that makes key generation failures possible, or if
OpenSSL runs out of memory. Fixes bug 19152; bugfix on
0.2.1.10-alpha. Found by Yuan Jochen Kang, Suman Jana, and Baishakhi
Ray.
This is potentially scary stuff, so let me walk through my analysis.
I think this is a bug, and a backport candidate, but not remotely
triggerable in any useful way.
Observation 1a:
Looking over the OpenSSL code here, the only way we can really fail in
the non-engine case is if malloc() fails. But if malloc() is failing,
then tor_malloc() calls should be tor_asserting -- the only way that an
attacker could do an exploit here would be to figure out some way to
make malloc() fail when openssl does it, but work whenever Tor does it.
(Also ordinary malloc() doesn't fail on platforms like Linux that
overcommit.)
Observation 1b:
Although engines are _allowed_ to fail in extra ways, I can't find much
evidence online that they actually _do_ fail in practice. More evidence
would be nice, though.
Observation 2:
We don't call crypto_pk_generate*() all that often, and we don't do it
in response to external inputs. The only way to get it to happen
remotely would be by causing a hidden service to build new introduction
points.
Observation 3a:
So, let's assume that both of the above observations are wrong, and the
attacker can make us generate a crypto_pk_env_t with a dangling pointer
in its 'key' field, and not immediately crash.
This dangling pointer will point to what used to be an RSA structure,
with the fields all set to NULL. Actually using this RSA structure,
before the memory is reused for anything else, will cause a crash.
In nearly every function where we call crypto_pk_generate*(), we quickly
use the RSA key pointer -- either to sign something, or to encode the
key, or to free the key. The only exception is when we generate an
intro key in rend_consider_services_intro_points(). In that case, we
don't actually use the key until the intro circuit is opened -- at which
point we encode it, and use it to sign an introduction request.
So in order to exploit this bug to do anything besides crash Tor, the
attacker needs to make sure that by the time the introduction circuit
completes, either:
* the e, d, and n BNs look valid, and at least one of the other BNs is
still NULL.
OR
* all 8 of the BNs must look valid.
To look like a valid BN, *they* all need to have their 'top' index plus
their 'd' pointer indicate an addressable region in memory.
So actually getting useful data of of this, rather than a crash, is
going to be pretty damn hard. You'd have to force an introduction point
to be created (or wait for one to be created), and force that particular
crypto_pk_generate*() to fail, and then arrange for the memory that the
RSA points to to in turn point to 3...8 valid BNs, all by the time the
introduction circuit completes.
Naturally, the signature won't check as valid [*], so the intro point
will reject the ESTABLISH_INTRO cell. So you need to _be_ the
introduction point, or you don't actually see this information.
[*] Okay, so if you could somehow make the 'rsa' pointer point to a
different valid RSA key, then you'd get a valid signature of an
ESTABLISH_INTRO cell using a key that was supposed to be used for
something else ... but nothing else looks like that, so you can't use
that signature elsewhere.
Observation 3b:
Your best bet as an attacker would be to make the dangling RSA pointer
actually contain a fake method, with a fake RSA_private_encrypt
function that actually pointed to code you wanted to execute. You'd
still need to transit 3 or 4 pointers deep though in order to make that
work.
Conclusion:
By 1, you probably can't trigger this without Tor crashing from OOM.
By 2, you probably can't trigger this reliably.
By 3, even if I'm wrong about 1 and 2, you have to jump through a pretty
big array of hoops in order to get any kind of data leak or code
execution.
So I'm calling it a bug, but not a security hole. Still worth
patching.
Fortunately, the arithmetic cannot actually overflow, so long as we
*always* check for the size of potentially hostile input before
copying it. I think we do, though. We do check each line against
MAX_LINE_LENGTH, and each object name or object against
MAX_UNPARSED_OBJECT_SIZE, both of which are 128k. So to get this
overflow, we need to have our memarea allocated way way too high up
in RAM, which most allocators won't actually do.
Bugfix on 0.2.1.1-alpha, where memarea was introduced.
Found by Guido Vranken.
Previously, if the header was present, we'd proceed even if the
function wasn't there.
Easy fix for bug 19161. A better fix would involve trying harder to
find libscrypt_scrypt.
We use a pretty specific pair of autoconf tests here to make sure
that we only add this code when:
a) a 64-bit signed multiply fails to link,
AND
b) the same 64-bit signed multiply DOES link correctly when
__mulodi4 is defined.
Closes ticket 19079.
We know there are overflows in curve25519-donna-c32, so we'll have
to have that one be fwrapv.
Only apply the asan, ubsan, and trapv options to the code that does
not need to run in constant time. Those options introduce branches
to the code they instrument.
(These introduced branches should never actually be taken, so it
might _still_ be constant time after all, but branch predictors are
complicated enough that I'm not really confident here. Let's aim for
safety.)
Closes 17983.
The goal here is to provide a way to decouple pieces of the code
that want to learn "when something happens" from those that realize
that it has happened.
The implementation here consists of a generic backend, plus a set of
macros to define and implement a set of type-safe frontends.
Apparently somewhere along the line we decided that MIN might be
missing.
But we already defined it (if it was missing) in compat.h, which
everybody includes.
Closes ticket 18889.
Unlike tor_assert(), these macros don't abort the process. They're
good for checking conditions we want to warn about, but which don't
warrant a full crash.
This commit also changes the default implementation for
tor_fragile_assert() to tor_assert_nonfatal_unreached_once().
Closes ticket 18613.
This marks some lines as unreachable by the unit tests, and as
therefore excluded from test coverage.
(Note: This convention is only for lines that are absolutely
unreachable. Don't use it anywhere you wouldn't add a
tor_fragile_assert().)
Otherwise coverity complains that we're checking an whether an int64 is
less than INT64_MIN, which of course it isn't.
Fixes CID 1357176. Not in any released Tor.
The fd would leak when the User wasn't recogniezed by
getpwnam(). Since we'd then go on to exit, this wasn't a terribad
leak, but it's still not as nice as no leak at all.
CID 1355640; bugfix on no released Tor.
I didn't want to grant blanket permissions for chmod() and chown(),
so here's what I had to do:
* Grant open() on all parent directories of a unix socket
* Write code to allow chmod() and chown() on a given file only.
* Grant chmod() and chown() on the unix socket.
On windows, you cannot open() a directory. So for Windows we should
just take our previous stat-based approach.
Closes bug 18392; bug not in any released Tor.
This is in accordance with our usual policy against freelists,
now that working allocators are everywhere.
It should also make memarea.c's coverage higher.
I also doubt that this code ever helped performance.
Short version: clang asan hates the glibc strcmp macro in
bits/string2.h if you are passing it a constant string argument of
length two or less. (I could be off by one here, but that's the
basic idea.)
Closes issue 14821.
Did you know that crypto_digest_all is a substring of
crypto_digest_alloc_bytes()? Hence the mysterious emergence of
"crypto_common_digestsoc_bytes".
Next time I should use the \b assertion in my regexen.
Spotted by Mike.
They are no longer "all" digests, but only the "common" digests.
Part of 17795.
This is an automated patch I made with a couple of perl one-liners:
perl -i -pe 's/crypto_digest_all/crypto_common_digests/g;' src/*/*.[ch]
perl -i -pe 's/\bdigests_t\b/common_digests_t/g;' src/*/*.[ch]
Closes ticket 18242.
The rationale here is that I like having coverage on by default in my
own working directory, but I always want assertions turned on unless
I'm doing branch coverage specifically.
We use sensible parameters taken from common sources, and no longer
have dynamic DH groups as an option, but it feels prudent to have
OpenSSL validate p and g at initialization time.
We've never actually tested this support, and we should probably assume
it's broken.
To the best of my knowledge, only OpenVMS has this, and even on
OpenVMS it's a compile-time option to disable it. And I don't think
we build on openvms anyway. (Everybody else seems to be working
around the 2038 problem by using a 64-bit time_t, which won't expire
for roughly 292 billion years.)
Closes ticket 18184.
node_get_all_orports and router_get_all_orports incorrectly used or_port
with IPv6 addresses. They now use ipv6_orport.
Also refactor and remove duplicated code.
This closes bug 18162; bugfix on a45b131590, which fixed a related
issue long ago.
In addition to the #18162 issues, this fixes a signed integer overflow
in smarltist_add_all(), which is probably not so great either.
Avoid using a pronoun where it makes comments unclear.
Avoid using gender for things that don't have it.
Avoid assigning gender to people unnecessarily.
