Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#18089 closed defect (fixed)

Tor compiled with --enable-expensive-hardening leads to runtime errors (null pointer passing)

Reported by: gk Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.25
Severity: Critical Keywords: 027-backport, 026-backport
Cc: isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

../src/common/crypto.c:2596:3: runtime error: null pointer passed as argument 1, which is declared to never be null

Immediately following I get

/usr/include/x86_64-linux-gnu/bits/string3.h:90:10: runtime error: null pointer passed as argument 1, which is declared to never be null

Line 90 in my string3.h is:

return __builtin___memset_chk (__dest, __ch, __len, __bos0 (__dest));

Steps to reproduce:
1) Get the latest hardened Tor Browser (the same issue happens with a stable Tor Browser + a self-compiled hardened Tor)
2) Start it with ./start-tor-browser.desktop --debug --log and don't connect directly but use a pluggable transport; just using the default one is enough, there is no need to enter bridges manually
3) You should see the above messages in your terminal/tor-browser.log file

FWIW connecting directly does not trigger this issue.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by isis

Cc: isis added

comment:2 Changed 3 years ago by bugzilla

Severity: NormalCritical

Null pointer dereferencing leads to unspecified behavior. Usually, there are zeroes at 0 addr, so everything "seems" to be fine, but this is a common bug in C.

comment:3 Changed 3 years ago by teor

Keywords: 027-backport 026-backport added
Milestone: Tor: 0.2.8.x-final
Status: newneeds_review
Version: Tor: 0.2.7.6Tor: 0.2.3.25

memwipe() calls OPENSSL_cleanse() and memset() on a NULL pointer. OPENSSL_cleanse() calls memchr() on a NULL pointer.

The impact of this bug depends on the exact semantics of the two system calls when passed a NULL pointer, and likely differs on different platforms. You'd think it wouldn't have any impact if the size argument was zero, but as far as I can determine, tor never passes a 0 size to memwipe().

However, plentry of Tor functions pass a NULL pointer to memwipe(). The unit tests are full of them.

Please see my branch bug18089-v2 at https://github.com/teor2345/tor.git
It's based on maint-0.2.6.

It changes memwipe to do nothing if passed a NULL pointer, and asserts that size is non-zero and not a likely underflow.

comment:4 Changed 3 years ago by nickm

I'm okay with all of this, except that memwiping() with sz 0 should IMO not be a bug; it's perfectly reasonable behavior to clear an thing whose length might be 0.

comment:5 Changed 3 years ago by teor

I wondered about that, fixed in bug18089-v3 to ignore NULL pointers or zero size.

comment:6 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.6.x-final
Resolution: fixed
Status: needs_reviewclosed

Applied to 0.2.6 and later.

comment:7 Changed 3 years ago by teor

(For the record, bug18089-v4 is actually based on maint-0.2.6 without any extra commits.)

comment:8 Changed 3 years ago by bugzilla

Googling brought this: www.viva64.com/en/b/0178/
Did you see it?
On topic: it is not good at all to pass NULL argument to function that doesn't expect it.
sz=0 is allowed but useless.

comment:9 in reply to:  8 Changed 3 years ago by teor

Replying to bugzilla:

Googling brought this: www.viva64.com/en/b/0178/
Did you see it?

It's from 2012, published on the same day Tor introduced memwipe() to address the issues described in that post.

On topic: it is not good at all to pass NULL argument to function that doesn't expect it.
sz=0 is allowed but useless.

As I said, the impact of this depends on unspecified semantics of those functions when passed a NULL pointer. This may vary by C library, compiler, and/or platform.

The patch above fixes these issues, and will be included in the next Tor 0.2.6 and 0.2.7 releases (and the master branch).

comment:10 Changed 3 years ago by nickm

teor: I've tweaked the patch a little so that we assert on ptr == NULL when sz != 0.

comment:11 Changed 3 years ago by cypherpunks

What code was involved here? Any bt?
It could be minor case or not.

comment:12 in reply to:  10 Changed 3 years ago by teor

Replying to nickm:

teor: I've tweaked the patch a little so that we assert on ptr == NULL when sz != 0.

This is going to cause crashes everywhere, we memwipe(NULL, sizeof(object)) in a lot of places in the tor codebase.

It's triggered by the ed25519 unit tests in master, and by something to do with pluggable transports in 0.2.7.6.

Please don't backport this change!

comment:13 Changed 3 years ago by nickm

I already made that change in 0.2.6 and later; master unit tests don't crash for me.

comment:14 Changed 3 years ago by teor

I ran make check and make test-network[-all] on 0.2.6, 0.2.7, and master and they all worked.

Also, I compiled a hardened version of tor on OS X and used it in Tor Browser with the default obfs3 pluggable transports, everything worked.

(I was sure I'd seen code passing a NULL pointer and struct size to memwipe, perhaps I was mistaken or the size was zero.)

comment:15 Changed 2 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.4.x-final

Backported to 0.2.4

Note: See TracTickets for help on using tickets.