Opened 2 years ago

Last modified 2 months ago

#22926 assigned defect

The Tor compression code can call functions that are NULL

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: review-group-32, 033-triage-20180320, 033-removed-20180320
Cc: mtigas, alex_y_xu@…, ahf Actual Points:
Parent ID: Points: 1
Reviewer: mikeperry Sponsor:

Description

The new Tor compression code in 0.3.1 assumes that all the compression functions are bound at runtime.

For example, tor_lzma_method_supported() returns 1 when HAVE_LZMA is defined, but that doesn't mean that lzma_version_string() has actually been bound to a non-NULL address in the binary.

This is more likely to happens when tor is used as a shared library rather than linked as an executable (shadow, iOS), and when using weak, lazy symbol binding.

This might not be an issue we can solve unless we check for all the symbols being NULL at runtime. Maybe the responsibility for proper linking is on people who are compiling tor with weak, lazy symbol binding.

This bug was discovered by Rob Jansen when running shadow.

Child Tickets

Change History (31)

comment:1 Changed 2 years ago by nickm

Owner: set to ahf
Status: newassigned

comment:2 Changed 2 years ago by teor

Can we make configure check that linking results in non-NULL symbols, as well as checking that the headers are present?

comment:3 Changed 2 years ago by ahf

This is going to be an issue for all external dependencies that we use via pkg-config I think. The old TOR_SEARCH_LIBRARY() macro is probably unaffected since it tries to build a small test file.

I will try to do the same as we do in TOR_SEARCH_LIBRARY() when the library is found to ensure that we can link against the .so and that the headers are present.

comment:4 Changed 2 years ago by nickm

If this is a hard issue to fix, we might just say "don't do that then" -- we don't build Tor with weak/lazy symbol binding ourselves, and while we don't tell people not to do it, we could just say that it is incumbent on people who change our build system to keep it working with their changes.

Thoughts?

comment:5 in reply to:  4 Changed 2 years ago by teor

Replying to nickm:

If this is a hard issue to fix, we might just say "don't do that then" -- we don't build Tor with weak/lazy symbol binding ourselves, and while we don't tell people not to do it, we could just say that it is incumbent on people who change our build system to keep it working with their changes.

Thoughts?

Documenting this issue seems like a sensible solution, but we should check if this affects iOS and Tor.framework before deciding. If it does affect iOS, and we want to support iOS as a first-class platform, maybe linking would be a good check to add.

I'd also phrase it as "pass unusual [linker] options to our build system", as "change our build system" could be unclear.

comment:6 Changed 22 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.3.x-final

comment:7 Changed 22 months ago by ahf

Cc: mtigas added

Hey Mike,

You mentioned during the mobile session in Montreal that you were going to look into adding support for LZMA and Zstandard for OnionBrowser.

Do you think this is going to be an issue for you on iOS?

comment:8 in reply to:  7 Changed 22 months ago by mtigas

Replying to ahf:

You mentioned during the mobile session in Montreal that you were going to look into adding support for LZMA and Zstandard for OnionBrowser.

Do you think this is going to be an issue for you on iOS?

Actually, my current guess is that this isn't an issue for iOS at all because the dependencies (including tor) get built as static libraries and then they're all linked into the app's single executable anyway.

I'm hoping to take a stab at building with liblzma (in Tor.framework / Onion Browser) over the next week or so. So maybe I'll have more specific feedback for y'all soon.

comment:9 Changed 19 months ago by nickm

Have we advanced our thinking any here? It still seems to me that "don't do that then" is the right solution.

comment:10 in reply to:  9 Changed 19 months ago by teor

Replying to nickm:

Have we advanced our thinking any here? It still seems to me that "don't do that then" is the right solution.

We have decided to support running Tor as a library.
Therefore, we will see more linking issues in future.

I suggest we:

  • document this weak linking issue
  • check for NULL pointers to functions in optional libraries at configure time
  • check for NULL pointers to functions in optional libraries at runtime

Otherwise, people will be surprised the first time their tor tries to use the compression code.

comment:11 Changed 19 months ago by ahf

Status: assignedneeds_review

There's a potential patch for parts of this issue in https://gitlab.com/ahf/tor/merge_requests/22

Note that the patch does the NULL function checks for the zlib backend: we might want to consider adding a tor_assert(0) in the pathological case there since zlib is still required to be able to use Tor.

I'm marking this needs_review, but these patches does not fix everything Teor mentioned above. We still need to add checks to configure and document the weak linking issue.

comment:12 Changed 19 months ago by teor

Maybe we don't need to check at configure time?
I think a warning is enough.

The documentation could form part of the tor_api header or docs.

comment:13 Changed 19 months ago by ahf

One thing to keep in mind, I'm not fully sure the patch I've proposed work under the given condition. I assume that doing &function yields NULL if the function is unavailable.

comment:14 Changed 19 months ago by nickm

Looks okay at first glance, but: do we really want to go down this road? Will we wind up doing this for every shared library we use in Tor, including libevent and openssl?

Also, zlib is mandatory: we can't keep going if zlib isn't going to work.

comment:15 Changed 19 months ago by ahf

I'm conflicted about this.

I don't think doing this is a particularly good solution. I added the changes to the zlib code in case we move away from Tor's TOR_SEARCH_LIBRARY() where this issue isn't detected.

The other option is that we add the configure checks that TOR_SEARCH_LIBRARY() adds to configure to ensure that our zstd/lzma code works in the same way.

This is of course still an issue for OpenSSL, Libevent, and other external dependencies we may have.

Would you prefer a configure check to this or should we do something entirely different?

comment:16 Changed 19 months ago by teor

I think Nick is right, checking for NULL functions could rapidly get out of control.
(I don't think we should check for mandatory libraries. But then I asked myself: why bother to check at all?)

I suggest we check that the zstd and lzma library functions are non-NULL at configure time. And fail if they are NULL.
That way, builders get instant feedback if their linker flags or install are broken.

People who design custom build processes will have to make sure they work themselves.

comment:17 Changed 19 months ago by Hello71

how exactly does one "enable weak symbols"? as far as I can tell, on both Mac and Linux, this process involves inserting __attribute__((weak)) or #pragma weak in the actual source files that reference the symbol in question. even in cases of shared libraries, AIUI, if executable a NEEDS library b.so, and a has a weak symbol f with no default, and b.so has an undefined symbol f, then a will fail to start.

comment:18 Changed 19 months ago by Hello71

Cc: alex_y_xu@… added

also, the following also fails to link (on x86-64 Linux):

a.c:

void a() __attribute__((weak));
void b();

int main() {
    a();
    b();
    return 0;
}

b.c:

void a();

void b() {
    a();
}
Last edited 19 months ago by Hello71 (previous) (diff)

comment:19 Changed 19 months ago by Hello71

(my point is that if you start fiddling with .c files, you get to play 52 pickup on your own.)

comment:21 Changed 19 months ago by nickm

Keywords: review-group-32 added

comment:22 Changed 18 months ago by mikeperry

Reviewer: mikeperry

comment:23 Changed 18 months ago by mikeperry

Status: needs_reviewneeds_revision

It has been a while since I have done autoconf, but as far as that goes, this looks good - has a sane default for cross compile given the situation (assume it works).

As to the larger question, I can see an argument for just going this check for one library to check if lazy linking is enabled globally. But there probably should be a comment in the autoconf test to say that's what we're doing (and why we are only testing this with one library). Marking needs_revision for that.

Also make check-changes fails because of the # in the bug line.

Version 0, edited 18 months ago by mikeperry (next)

comment:24 Changed 18 months ago by mikeperry

Oh, also did we find a platform to test such that this test fails when weak/lazy binding is in effect?

comment:25 Changed 18 months ago by teor

shadow's tor plugin will fail this test, as will many platforms that use tor as a library.

comment:26 in reply to:  25 Changed 18 months ago by Hello71

Replying to teor:

shadow's tor plugin will fail this test, as will many platforms that use tor as a library.

did you test this? if so, what steps exactly did you use? as far as I can tell, shadow-plugin-tor:

  1. does not use weak linking at all
  2. does not actually build the plugin using autotools, so almost any "funny business" that the plugin build process could do would not be detected by any configure.ac checks anyways.

however, after extensive research, I have found that apparently on Mac, the linker (whose official man page is not even available online!) allows you to shoot your whole limb off at the same time using the poorly-engineered -weak_reference_mismatches and -weak-l options. as pointed out in the man page, these options are poorly designed because it silently alters the behavior of unrelated program code. these options are in fact so poorly conceived that Googling for "weak_reference_mismatches" returns almost entirely results saying to turn it off.

however! even if this option is passed, I am still convinced that the configure check will be almost entirely ineffective for three reasons.

  1. assuming the library is installed correctly, even if the user has enabled the limb-gun option, this configure check will pass just fine, and even the build products will function correctly. only if the user subsequently *uninstalls* the library, tor will suddenly start crashing. (and again, only if you used the limb-gun option.)
  2. if the library provides only a subset of the necessary symbols, tor will compile but fail to run. unfortunately, the configure check still doesn't help, because it only checks that the version function works (which is the only function sorta guaranteed not to ever change, the opposite of the requirement here), not that all the functions work.
  3. as far as I know, autotools does not support magically configuring an executable to be compiled as a shared library instead. this is why shadow-plugin-tor doesn't bother and builds tor with cmake instead. this configure check will obviously not work if it is not called.

comment:27 Changed 18 months ago by teor

There's a lot of detail here, and I don't have time to check it all on my Mac.

The original bug report was from the shadow author, who now wants to integrate building the tor shadow plugin into configure.ac.

comment:28 Changed 17 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:29 Changed 17 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:30 Changed 17 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

comment:31 Changed 2 months ago by gaba

Cc: ahf added
Owner: ahf deleted
Status: needs_revisionassigned

Liberating some of the tickets that ahf had.

Note: See TracTickets for help on using tickets.