Opened 18 months ago

Last modified 2 weeks ago

#24755 assigned defect

Shell scripts refactoring and bash privacy leak. Heredoc should not be used in start-tor-browser script.

Reported by: asan Owned by: tbb-team
Priority: Low Milestone:
Component: Applications/Tor Browser Version:
Severity: Minor Keywords: tbb-disk-leak
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In most of shells (including bash) heredoc, i.e. << and <<<, is implemented through creation of temporary files in TMP. In the case of bash these are the files like /tmp/sh-thd-1234567890. This can be checked using the command [1]

sleep 3 <<<"here string" & lsof -p $! | grep 0r

Furthermore, these TMP files may remain if, e.g., shell script crashes. There were some complaints that these files are still accessible through file descriptors even after removal [2], [3].

Since TBB and similar applications are intended to be portable, they should not leave traces outside of their portable directory. However, bash commands in scripts like start-tor-browser may run when separate TMP for TBB is not yet set, i.e. system TMP (/tmp), which is not always mounted in memory, may be used. It means that traces (that TBB was used) will be created outside of TBB directory. This is a minor leak in comparison to en elephant 7449 (yet unfixed), but it is still a leak.

In general, if TMP for TBB is created before the use of heredoc command in script, it should be fine. However, as heredoc is potentially leaky and dangerous thing, it should be avoided in secure scripts. One could use simple echo command instead.

Now start-tor-browser has at least one cat <<EOF. AFAIK, tor-messenger also has this problem. By the way, in this case writing cat <<"EOF" (i.e. with quotation) is the safer alternative, as variables substitution will not be done, and substituted text will be verbatim. Moreover, new safer notation $(command) should be used instead of old-style `command` in start-tor-browser.

There are also other things in this script, which are often considered to be a bad practice. In particular,

  1. Multiple characters variables should be always in braces (${show_output} instead of $show_output).
  2. Quotation "" should be used everywhere and in all assignments.
  3. [[ and ]], as much safer alternative, should be used instead of [ and ].

I would refer to Google shell style guide [4] as a good starting point to learn how to write secure shell scripts.

All these notes should be applied to all shell scripts within Tor Project.

Child Tickets

Change History (5)

comment:1 Changed 8 months ago by asan

Keywords: tbb-disk-leak added
Owner: changed from tbb-team to gk
Status: newassigned

comment:2 Changed 8 months ago by gk

Owner: changed from gk to tbb-team

comment:3 Changed 8 months ago by boklm

It seems that setting TMPDIR to a tmp directory inside the Tor Browser directory at the beginning of start-tor-browser would solve the issue with <<.

If I run the following script:

#!/bin/bash
export TMPDIR='/tmp/test-heredoc'
mkdir -p $TMPDIR
sleep 3 <<<"here string" & lsof -p $! | grep 0r

I get the following output:

sleep   3719 user    0r   REG   0,36       12  27468 /tmp/test-heredoc/sh-thd-811442066 (deleted)

It might also be useful to set TMPDIR in case some part of firefox is using it to select the tmp directory.

comment:4 Changed 8 months ago by asan

It seems that setting TMPDIR to a tmp directory inside the Tor Browser directory at the beginning of start-tor-browser would solve the issue with <<

Your solution looks working. However, if you want to set temporary directories for the script correctly, it would be logical to set them in a way which resolves #7449 too.

There are also other things in this script, which are often considered to be a bad practice. In particular

I would add also another point: echo must be replaced by printf everywhere.

comment:5 Changed 2 weeks ago by teor

shellcheck is a useful tool for finding shell script issues.

We run it during out CI in tor (and other network team projects).

Note: See TracTickets for help on using tickets.