Opened 2 years ago

Last modified 5 months ago

#19376 needs_review defect

Fix a few torsocks bugs caused by unquoted variables

Reported by: cypherpunks Owned by: dgoulet
Priority: Medium Milestone:
Component: Core Tor/Torsocks Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

For such a short script, the torsocks wrapper has a fair few bugs, almost entirely caused by unquoted variables. This patch fixes it by quoting them where they need to be. A shell session showing just one example of a bug caused by failing to quote variables, followed by intended behavior after applying a patch:

$ torsocks --version
Torsocks 2.1.0
$ mkdir foo\ bar
$ echo echo hello\ world > foo\ bar/hello.sh
$ chmod 700 foo\ bar/hello.sh
$ ./foo\ bar/hello.sh
hello world
$ torsocks ./foo\ bar/hello.sh
/usr/bin/torsocks: 103: [: ./foo: unexpected operator
ERROR: ./foo bar/hello.sh cannot be found.
$ (cd / && sudo patch -p0) < quote.patch
patching file /usr/bin/torsocks
$ torsocks ./foo\ bar/hello.sh
hello world

The patch to fix it is attached.

Child Tickets

Attachments (2)

quote.patch (1.3 KB) - added by cypherpunks 2 years ago.
quote2.patch (1.8 KB) - added by cypherpunks 23 months ago.

Download all attachments as: .zip

Change History (10)

Changed 2 years ago by cypherpunks

Attachment: quote.patch added

comment:1 Changed 2 years ago by cypherpunks

Oops, just noticed that this bug was already reported in #17760, though without a patch to fix it. I can't seem to delete this ticket and turn it into a reply to the first bug report, oh well. Hopefully the patch will render both tickets obsolete.

comment:2 Changed 2 years ago by cypherpunks

Status: newneeds_review

Different cypherpunk here. I closed #17760 in favor of this ticket because it has a patch.

comment:3 Changed 2 years ago by dgoulet

Status: needs_reviewaccepted

Accept a bunch of tickets for torsocks.

comment:4 Changed 2 years ago by dgoulet

Status: acceptedneeds_revision

I can't apply this patch. Can you send me a git diff or a branch or git format-patch? The diff file assume that torsocks is in /usr/bin/torsocks and thus fails :S ... If you can't let me know, I'll apply it by hand but I would rather not.

comment:5 in reply to:  4 Changed 23 months ago by cypherpunks

Replying to dgoulet:

I can't apply this patch. Can you send me a git diff or a branch or git format-patch? The diff file assume that torsocks is in /usr/bin/torsocks and thus fails :S ... If you can't let me know, I'll apply it by hand but I would rather not.

Even after all this time, the script is just as horrifying. It's reminiscent of reading glibc. Not as traumatic though, it's a lot shorter.

Since the bug is still present in 2.2.0 (and I had forgot about this bug), I wrote a quick new one in git diff format. The new patch assumes it's in src/bin/torsocks (where it is in the source tgz).

Changed 23 months ago by cypherpunks

Attachment: quote2.patch added

comment:6 Changed 20 months ago by dgoulet

Resolution: fixed
Status: needs_revisionclosed

Took quote2.patch and added more. This has been applied to torsocks.in.

Merged. Thanks!

comment:7 Changed 5 months ago by torsocksbug

Resolution: fixed
Status: closedreopened

Doesn't work for me, the quotes were not applied correctly. I don't know what happened here, the original attachment actually contained the correct fix.

Here's the patch for the latest master (8013dfb):

diff --git a/src/bin/torsocks.in b/src/bin/torsocks.in
index fe8b67a..bf545a3 100644
--- a/src/bin/torsocks.in
+++ b/src/bin/torsocks.in
@@ -130,7 +130,7 @@ tor_shell ()
 
 torify_app ()
 {
-       local app_path="`which $1`"
+       local app_path="`which "$1"`"
        local getcap="`PATH="$PATH:/usr/sbin:/sbin" which getcap`"
        local caps=
 
@@ -144,7 +144,7 @@ torify_app ()
        # This must be before torifying because getcap uses cap_get_file(3)
        # via syscall(2) which breaks torsocks.
        if [ -n "$getcap" ]; then
-               caps="`$getcap $app_path 2>/dev/null`"
+               caps="`$getcap "$app_path" 2>/dev/null`"
        fi
 
        # Check if Apple's System Integrity Protection is enabled if the user is

comment:8 Changed 5 months ago by torsocksbug

Status: reopenedneeds_review
Note: See TracTickets for help on using tickets.