Commit Graph

22210 Commits

Author SHA1 Message Date
Nick Mathewson
c3adbf755b Resolve some warnings from OSX clang. 2016-06-11 10:11:53 -04:00
Nick Mathewson
26e979b342 Add all the clang-only warnings that do not trigger now 2016-06-11 10:11:53 -04:00
Nick Mathewson
15533c8897 Set our autoconf-breaking options last, not before we check for others 2016-06-11 10:11:53 -04:00
Nick Mathewson
9bbd6502f0 Use autoconf, not gcc version, to decide which warnings we have
This gives more accurate results under Clang, which can only help us
detect more warnings in more places.

Fixes bug 19216; bugfix on 0.2.0.1-alpha
2016-06-11 10:11:53 -04:00
Nick Mathewson
ad16c55286 Use -Wstrict-overflow=2 on gcc5+. 2016-06-11 10:11:53 -04:00
Nick Mathewson
4caed2424a Enable -Woverlength-strings for GCC>=4.6 on MOST of the code.
IMO it's fine for us to make exceptions to this rule in the unit
tests, but not in the code at large.
2016-06-11 10:11:52 -04:00
Nick Mathewson
8f2d2933f9 Use -Wdouble-promotion in GCC >= 4.6
This warning triggers on silently promoting a float to a double.  In
our code, it's just a sign that somebody used a float by mistake,
since we always prefer double.
2016-06-11 10:11:52 -04:00
Nick Mathewson
493499a339 Add -Wfloat-conversion for GCC >= 4.9
This caught quite a few minor issues in our unit tests and elsewhere
in our code.
2016-06-11 10:11:52 -04:00
Nick Mathewson
2ff20c93a5 Add -Wunused-const-variable=2 on GCC >=6.1
This caused a trivial warning in curve25519-donna-64bit.h, which
had two unused constants.  I commented them out.
2016-06-11 10:11:52 -04:00
Nick Mathewson
12517c7303 Add -Wduplicated-cond on GCC 6 2016-06-11 10:11:52 -04:00
Nick Mathewson
b14c1f4082 Merge remote-tracking branch 'public/bug19203_027' into HEAD 2016-06-11 10:11:44 -04:00
Nick Mathewson
4f8086fb20 Enable -Wnull-dereference (GCC >=6.1), and fix the easy cases
This warning, IIUC, means that the compiler doesn't like it when it
sees a NULL check _after_ we've already dereferenced the
variable. In such cases, it considers itself free to eliminate the
NULL check.

There are a couple of tricky cases:

One was the case related to the fact that tor_addr_to_in6() can
return NULL if it gets a non-AF_INET6 address.  The fix was to
create a variant which asserts on the address type, and never
returns NULL.
2016-06-11 10:10:29 -04:00
Nick Mathewson
bdc59e33c1 Fix a warning on unnamed nodes in node_get_by_nickname().
There was a > that should have been an ==, and a missing !.  These
together prevented us from issuing a warning in the case that a
nickname matched an Unnamed node only.

Fixes bug 19203; bugfix on 0.2.3.1-alpha.
2016-05-30 12:03:03 -04:00
Nick Mathewson
55b5e0076f Add another 22 or so GCC warnings. None currently triggers for me. 2016-05-28 17:09:31 -04:00
Nick Mathewson
ce1dbbc4fd Enable the -Waggregate-return warning
Suppress it in the one spot in the code where we actually do want to
allow an aggregate return in order to call the mallinfo() API.
2016-05-27 11:26:14 -04:00
Nick Mathewson
0df2c5677a Use ENABLE_GCC_WARNING and DISABLE_GCC_WARNING in tortls.c
Previously we'd done this ad hoc.
2016-05-27 11:25:42 -04:00
Nick Mathewson
0279e48473 Add support for temporarily suppressing a warning
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.
2016-05-27 11:23:52 -04:00
Nick Mathewson
a0dd836098 Merge remote-tracking branch 'public/ticket19044' 2016-05-27 10:39:34 -04:00
Nick Mathewson
437cbb17c2 Merge remote-tracking branch 'asn/feature19036' 2016-05-27 10:37:11 -04:00
Nick Mathewson
476714e1a4 Merge remote-tracking branch 'arma/bug18840' 2016-05-27 10:35:55 -04:00
Nick Mathewson
3934e78bb9 Make format_changelog.py add links to bugs 2016-05-27 09:26:49 -04:00
Nick Mathewson
a93b6bbf17 Merge branch 'maint-0.2.8'
(This is an "ours" merge to avoid taking the version bump)
2016-05-26 21:09:22 -04:00
Nick Mathewson
f25806409d Bump to 0.2.8.3-alpha-dev 2016-05-26 21:09:01 -04:00
Nick Mathewson
02383ea7ea Forward-port the 0.2.8.3-alpha changelog 2016-05-26 21:06:42 -04:00
Nick Mathewson
a38dfc7f29 Merge branch 'maint-0.2.8'
(Ours merge, to avoid taking version bump)
2016-05-26 12:30:03 -04:00
Nick Mathewson
0a74346fe4 Bump to 0.2.8.3-alpha 2016-05-26 12:29:45 -04:00
Nick Mathewson
8c1c71aa2c Merge branch 'maint-0.2.8' 2016-05-26 12:12:54 -04:00
Nick Mathewson
a873ba8edd Fix two long lines 2016-05-26 12:11:57 -04:00
George Kadianakis
d875101e03 Functionify code that writes votes to disk. 2016-05-26 15:35:13 +03:00
Nick Mathewson
b7fac185a6 Merge branch 'maint-0.2.8' 2016-05-25 16:59:46 -04:00
Nick Mathewson
36b2b48308 Merge branch 'bug18668_028' into maint-0.2.8 2016-05-25 16:58:43 -04:00
Nick Mathewson
28cbcd033c Merge branch 'maint-0.2.8' 2016-05-25 16:40:51 -04:00
Nick Mathewson
f2d614c3d9 Merge branch 'bug19175_028_v2' into maint-0.2.8 2016-05-25 16:12:01 -04:00
Nick Mathewson
9cf6af76eb Fix a double-free bug in routerlist_reparse_old
I introduced this bug when I moved signing_key_cert into
signed_descriptor_t. Bug not in any released Tor.  Fixes bug 19175, and
another case of 19128.

Just like signed_descriptor_from_routerinfo(), routerlist_reparse_old()
copies the fields from one signed_descriptor_t to another, and then
clears the fields from the original that would have been double-freed by
freeing the original.  But when I fixed the s_d_f_r() bug [#19128] in
50cbf22099, I missed the fact that the code was duplicated in
r_p_o().

Duplicated code strikes again!

For a longer-term solution here, I am not only adding the missing fix to
r_p_o(): I am also extracting the duplicated code into a new function.

Many thanks to toralf for patiently sending me stack traces until
one made sense.
2016-05-25 16:11:35 -04:00
Nick Mathewson
44ea3dc331 Merge branch 'maint-0.2.8' 2016-05-25 10:21:15 -04:00
Nick Mathewson
6d375f17fc Merge branch 'bug19161_028_v2' into maint-0.2.8 2016-05-25 10:17:26 -04:00
Nick Mathewson
a3ec811c2e Merge branch 'maint-0.2.8' 2016-05-25 09:27:47 -04:00
Nick Mathewson
fdfc528f85 Merge branch 'bug19152_024_v2' into maint-0.2.8 2016-05-25 09:26:45 -04:00
Nick Mathewson
c4c4380a5e Fix a dangling pointer issue in our RSA keygen code
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.
2016-05-25 09:23:57 -04:00
Nick Mathewson
6abceca182 Merge branch 'memarea_overflow_027_squashed' into maint-0.2.8 2016-05-25 09:22:02 -04:00
Nick Mathewson
be2d37ad3c Fix a pointer arithmetic bug in memarea_alloc()
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.
2016-05-25 09:20:37 -04:00
Nick Mathewson
0ef36626ea Use calloc, not malloc(a*b), in ed25519 batch signature check fn
[Not a triggerable bug unless somebody is going to go checking
millions+ of signatures in a single go.]
2016-05-25 08:59:08 -04:00
Nick Mathewson
be3875cda2 Make sure that libscrypt_scrypt actually exists before using it.
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.
2016-05-24 10:31:02 -04:00
Nick Mathewson
771ca7c544 Stop recommending --enable-gcc-warnings in doc/HACKING 2016-05-23 14:40:27 -04:00
Nick Mathewson
2fa7a3af4c Make advisory-warnings on by default.
Add --enable-fatal-warnings to control -Werror.

Closes ticket 19044.
2016-05-23 14:39:56 -04:00
Nick Mathewson
5b7d4b0cea Merge branch 'maint-0.2.8' 2016-05-23 11:06:32 -04:00
cypherpunks
87134db57c Do not ignore files that are being tracked by git 2016-05-23 11:02:15 -04:00
Nick Mathewson
b53a2059c4 Expose crypto_digest_algorithm_get_length from crypto.c
Also, use it in routerparse.c
2016-05-23 10:58:27 -04:00
Nick Mathewson
2a884926c0 Merge remote-tracking branch 'dgoulet/bug19066_029_01' 2016-05-23 10:45:13 -04:00
Nick Mathewson
9c7edb0f3e Merge branch 'maint-0.2.8' 2016-05-20 10:46:50 -04:00