We found this in #40076, after we started using buf_move_all() in
more places.  Fixes bug #40076; bugfix on 0.3.3.1-alpha.  As far as
I know, the crash only affects master, but I think this warrants a
backport, "just in case".

The failing case is #if'd out for now, but will be fixed in the next
commit.

Testing for a fix for #40076.

Fixes bug 31036; bugfix on 0.2.1.8-alpha when we moved the logging
system to use posix fds.

They are about to be shutdown in September.
Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored Jul 23, 2020 and Nick Mathewson committed Jul 23, 2020

Closes #40061
Signed-off-by: David Goulet <dgoulet@torproject.org>

This warning only affects platforms (like win32) with 32-bit time_t.

Fixes bug 40028; bugfix on 0.3.2.8-rc.

Alexander Færøy authored May 16, 2020 and Nick Mathewson committed Jul 06, 2020

This patch changes our bits-to-bytes conversion logic in the NSS
implementation of `tor_tls_cert_matches_key()` from using (x >> 3) to
((x + 7) >> 3) since DER bit-strings are allowed to contain a number of
bits that is not a multiple of 8.

Additionally, we add a comment on why we cannot use the
`DER_ConvertBitString()` macro from NSS, as we would potentially apply
the bits-to-bytes conversion logic twice, which would lead to an
insignificant amount of bytes being compared in
`SECITEM_ItemsAreEqual()` and thus turn the logic into being a
prefix match instead of a full match.

The `DER_ConvertBitString()` macro is defined in NSS as:

    /*
    ** Macro to convert der decoded bit string into a decoded octet
    ** string. All it needs to do is fiddle with the length code.
    */
    #define DER_ConvertBitString(item)            \
        {                                         \
            (item)->len = ((item)->len + 7) >> 3; \
        }

Thanks to Taylor Yu for spotting this problem.

This patch is part of the fix for TROVE-2020-001.

See: https://bugs.torproject.org/33119

Alexander Færøy authored May 16, 2020 and Nick Mathewson committed Jul 06, 2020

We add constness to `peer_info_orig_len` and `cert_info_orig_len` in
`tor_tls_cert_matches_key` to ensure that we don't accidentally alter
the variables.

This patch is part of the fix for TROVE-2020-001.

See: https://bugs.torproject.org/33119

Alexander Færøy authored Mar 31, 2020 and Nick Mathewson committed Jul 06, 2020

This patch fixes an out-of-bound memory read in
`tor_tls_cert_matches_key()` when Tor is compiled to use Mozilla's NSS
instead of OpenSSL.

The NSS library stores some length fields in bits instead of bytes, but
the comparison function found in `SECITEM_ItemsAreEqual()` needs the
length to be encoded in bytes. This means that for a 140-byte,
DER-encoded, SubjectPublicKeyInfo struct (with a 1024-bit RSA public key
in it), we would ask `SECITEM_ItemsAreEqual()` to compare the first 1120
bytes instead of 140 (140bytes * 8bits = 1120bits).

This patch fixes the issue by converting from bits to bytes before
calling `SECITEM_ItemsAreEqual()` and convert the `len`-fields back to
bits before we leave the function.

This patch is part of the fix for TROVE-2020-001.

See: https://bugs.torproject.org/33119

Alexander Færøy authored Mar 31, 2020 and Nick Mathewson committed Jul 06, 2020

This patch lifts the `tor_tls_cert_matches_key()` tests out of the
OpenSSL specific TLS test suite and moves it into the generic TLS test
suite that is executed for both OpenSSL and NSS.

This patch is largely a code movement, but we had to rewrite parts of
the test to avoid using OpenSSL specific data-types (such as `X509 *`)
and replace it with the generic Tor abstraction type
(`tor_x509_cert_impl_t *`).

This patch is part of the fix for TROVE-2020-001.

See: https://bugs.torproject.org/33119

This is not actually a bug!  It can happen for a bunch of reasons,
which all boil down to "trying to add an extrainfo for which we no
longer have the corresponding routerinfo".

Fixes #16016; bugfix on 0.2.6.3-alpha.

Resume being willing to use preemptively-built circuits when
UseEntryGuards is set to 0. We accidentally disabled this feature with
that config setting (in our fix for #24469), leading to slower load times.

Fixes bug 34303; bugfix on 0.3.3.2-alpha.

GCC added an implicit-fallthrough warning a while back, where it
would complain if you had a nontrivial "case:" block that didn't end
with break, return, or something like that.  Clang recently added
the same thing.

GCC, however, would let you annotate a fall-through as intended by
any of various magic "/* fall through */" comments.  Clang, however,
only seems to like "__attribute__((fallthrough))".  Fortunately, GCC
accepts that too.

A previous commit in this branch defined a FALLTHROUGH macro to do
the right thing if GNUC is defined; here we replace all of our "fall
through" comments with uses of that macro.

This is an automated commit, made with the following perl one-liner:

  #!/usr/bin/perl -i -p
  s#/\* *falls? ?thr.*?\*/#FALLTHROUGH;#i;

This macro defers to __attribute__((fallthrough)) on GCC (and
clang).  Previously we had been using GCC's magic /* fallthrough */
comments, but clang very sensibly doesn't accept those.

Since not all compiler recognize it, we only define it when our
configure script detects that it works.

Part of a fix for 34078.

For example, "TOR_SKIP_TESTCASES=crypto/.. ./src/test/test" will run
the tests and suppress all the "crypto/" tests.  You could get the
same effect by running "./src/test/test :crypto/..", but that can be
harder to arrange from CI.

Part of a fix/workaround for 33643.

George Kadianakis authored Feb 11, 2020 and Nick Mathewson committed Mar 17, 2020

Given that ed25519 public key validity checks are usually not needed
and (so far) they are only necessary for onion addesses in the Tor
protocol, we decided to fix this specific bug instance without
modifying the rest of the codebase (see below for other fix
approaches).

In our minimal fix we check that the pubkey in
hs_service_add_ephemeral() is valid and error out otherwise.

In the openssl that I have, it should be safe to only check the size
of n.  But if I'm wrong, or if other openssls work differently, we
should check whether any of the fields are too large.

Issue spotted by Teor.

(Deep, deep thanks to Taylor for reminding me to test this!)

Although OpenSSL before 1.1.1 is no longer supported, it's possible
that somebody is still using it with 0.3.5, so we probably shouldn't
break it with this fix.

David Goulet authored Feb 18, 2020 and Nick Mathewson committed Mar 12, 2020

Closes #33361
Signed-off-by: David Goulet <dgoulet@torproject.org>

Fixes bug 33032; bugfix on 0.3.5.1-alpha when we introduced our own
PEM decoder.

For a bridge configured with a pluggable transport, the transport name is
used, with the IP address, for the GeoIP client cache entry.

However, the DoS subsystem was not aware of it and always passing NULL when
doing a lookup into the GeoIP cache.

This resulted in bridges with a PT are never able to apply DoS defenses for
newly created connections.

Fixes #33491
Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored Feb 04, 2020 and teor committed Feb 12, 2020

Fixes #28992
Signed-off-by: David Goulet <dgoulet@torproject.org>