Opened 17 months ago

Closed 12 months ago

Last modified 8 months ago

#22605 closed defect (fixed)

sandbox_intern_string(): Bug: No interned sandbox parameter found for /etc/tor/torrc.d/

Reported by: toralf Owned by: dgoulet
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.3-alpha
Severity: Normal Keywords: sandbox, regression, 031-backport, 032-backport
Cc: danielpinto52@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

got this today at a stable hardened Gentoo linux server:

mr-fox ~ # cat x.log 
Jun 14 21:19:55.000 [warn] sandbox_intern_string(): Bug: No interned sandbox parameter found for /etc/tor/torrc.d/ (on Tor 0.3.1.3-alpha dc47d936d47ffc25)
Jun 14 21:19:55.000 [warn] sandbox_intern_string(): Bug: No interned sandbox parameter found for /etc/tor/torrc.d/ (on Tor 0.3.1.3-alpha dc47d936d47ffc25)

============================================================ T= 1497467995
(Sandbox) Caught a bad syscall attempt (syscall open)
/usr/bin/tor(+0x18e5f5)[0x55ec9b7a75f5]
/lib64/libc.so.6(opendir+0x11)[0x7f5dfe271421]
/lib64/libc.so.6(opendir+0x11)[0x7f5dfe271421]
/usr/bin/tor(tor_listdir+0x32)[0x55ec9b7a32e2]
/usr/bin/tor(+0x179080)[0x55ec9b792080]
/usr/bin/tor(config_get_lines_include+0x38)[0x55ec9b792258]
/usr/bin/tor(options_init_from_string+0x1df)[0x55ec9b716d0f]
/usr/bin/tor(options_init_from_torrc+0x1f1)[0x55ec9b717221]
/usr/bin/tor(+0x4f399)[0x55ec9b668399]
/usr/lib64/libevent-2.1.so.6(+0x23749)[0x7f5dff649749]
/usr/lib64/libevent-2.1.so.6(event_base_loop+0x57f)[0x7f5dff64a5ef]
/usr/bin/tor(do_main_loop+0x24d)[0x55ec9b66693d]
/usr/bin/tor(tor_main+0x1c3d)[0x55ec9b66a32d]
/usr/bin/tor(main+0x28)[0x55ec9b661d18]
/lib64/libc.so.6(__libc_start_main+0xfc)[0x7f5dfe1d972c]
/usr/bin/tor(_start+0x29)[0x55ec9b661d69]

Child Tickets

Change History (28)

comment:1 Changed 17 months ago by toralf

I should mention that Tor crashed after that, meaning "SandBox 1" can't be used at a hardened Gentoo Linux server.

comment:2 Changed 17 months ago by dgoulet

Keywords: sandbox regression added
Milestone: Tor: 0.3.1.x-final
Priority: MediumHigh

It appears that we merged torrc.d/ capability in Tor with commit ba3a5f82f11388237a3ba4995ddf0b6ffaaf492a but sandbox did not follow.

$ git describe --contains ba3a5f82f1
tor-0.3.1.1-alpha~30^2

comment:3 Changed 16 months ago by Jigsaw52

Here's how to reproduce this bug:

/etc/tor/torrc:

Sandbox 1
%include /etc/tor/torrc.d/

Make sure /etc/tor/torrc.d/ exists.

Run tor:

tor -f /etc/tor/torrc

Send a SIGHUP to the tor process to make it reload the configuration file:

kill -1 <TOR_PID>

I am working on a fix.

comment:4 Changed 16 months ago by nickm

We should fix this in 0.3.1 if we can; if we can't, we should document that sandbox and include-directories don't work together yet.

comment:5 Changed 16 months ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:6 Changed 16 months ago by Jigsaw52

I'm having some problems fixing this one. I tried to change the sandbox code to allow adding more filters at runtime but it seems that the rules added after the initial seccomp initialization are being ignored.

More specifically, the problem I am having is the following (I am using the example in the above comments):

  1. When the config is reloaded, the filter that allows opening /etc/tor/torrc.d/ appears to be installed correctly (sb_open adds the filter to the context and seccomp_load returns 0 when loading the context)
  2. However, when open is called with /etc/tor/torrc.d/, the process is still killed
  3. I've checked the value of the pointer to the "/etc/tor/torrc.d/" string and it is the same on sb_open when the rule is added and on the tor_listdir function, where opendir is called, which then calls the open syscall.

I believe the problem is related to adding filters after the initial seccomp initialization.
It would be great if someone who has some understanding of the sandbox code and libseccomp could take a look at this too.

My code is in this branch: https://github.com/Jigsaw52/tor/tree/fix-torrcd-sandbox-22605

comment:7 Changed 16 months ago by Jigsaw52

Cc: danielpinto52@… added

comment:8 Changed 16 months ago by Jigsaw52

I believe the reason my patch is not working is a bug in libseccomp. I've opened an issue on their github: https://github.com/seccomp/libseccomp/issues/87

comment:9 Changed 16 months ago by yawning

I believe the reason my patch is not working is a bug in libseccomp.

No libseccomp is working exactly as expected. The reason your patch isn't working is because seccomp-bfp does not work that way.

https://www.kernel.org/doc/Documentation/prctl/seccomp_filter.txt

A seccomp filter may return any of the following values. If multiple
filters exist, the return value for the evaluation of a given system
call will always use the highest precedent value. (For example,
SECCOMP_RET_KILL will always take precedence.)

SECCOMP_RET_ALLOW has the lowest precedence out of all of the filter actions. Essentially, "you can't loosen restrictions by installing an additional filter".

comment:10 Changed 16 months ago by Jigsaw52

Ok. I understand why it does not work now.

This means that the best thing that can be done in this case is to check for all includes in the config files before setting up the sandbox and include rules for those.

Reloading the configuration after adding more %includes or even more files to already included folders will not be possible if the sandbox is enabled.

comment:11 in reply to:  10 Changed 16 months ago by yawning

Replying to Jigsaw52:

This means that the best thing that can be done in this case is to check for all includes in the config files before setting up the sandbox and include rules for those.

In the long term, there's certainly ways to make this work, but it will involve using IPC and a separate process, or a separate sandboxing mechanism for filesystem access. Either of those changes would be objectively superior to what's done currently, but would be rather involved.

Reloading the configuration after adding more %includes or even more files to already included folders will not be possible if the sandbox is enabled.

Indeed. There's already certain preferences that are incompatible with the sandbox all together, or with reloads, so supporting it to the limits of the existing code and documenting the caveats may be sufficient.

comment:12 Changed 16 months ago by nickm

Yeah; for now, I think the best we can do is to declare that sandbox + include + reload is not going to work, and warn the user if they try to do it.

comment:13 Changed 15 months ago by dgoulet

Status: acceptedneeds_review

I gave it a shot here: bug22605_031_01

comment:14 Changed 15 months ago by nickm

Status: needs_reviewneeds_revision

I think this isn't quite right, for a couple of reasons:

  • The code here gets run after the new options are parsed. But the included files will make the load fail before we reach this point.
  • This patch makes it impossible to disable includes, not only to enable them.


comment:15 Changed 15 months ago by Jigsaw52

Sorry for disappearing for a while but life got in the way.

I've been working on a new patch that:

  1. Allows %include to work with sandbox and reload as long as no new files are added.
  2. Fails to reload the configuration (without crashing :) when a new file has been added.

You can see my branch on https://github.com/Jigsaw52/tor/commits/fix-torrcd-sandbox-22605v2

Currently, 1. seems to be working for this example but it still needs further testing.
I haven't started working on 2. yet.

comment:16 Changed 15 months ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 15 months ago by nickm

Keywords: review-group-23 added

Put 0.3.1.x needs_review tickets into review-group-23

comment:18 Changed 14 months ago by dgoulet

Status: needs_reviewneeds_revision
  • smartlist_t *opened_files = smartlist_new(); memleak on error.
  • Why adding this? SCMP_SYS(getdents),

comment:19 in reply to:  18 Changed 14 months ago by Jigsaw52

Status: needs_revisionneeds_review

Replying to dgoulet:

  • smartlist_t *opened_files = smartlist_new(); memleak on error.

Fixed.

  • Why adding this? SCMP_SYS(getdents),

readdir which is called by tor_listdir uses getdents on my test system (Ubuntu 16.10 64bits).
This problem is not related to this ticket but it should be fixed too. I can create a different ticket and patch for it if you prefer.

comment:20 Changed 14 months ago by dgoulet

Status: needs_reviewmerge_ready

Ok, I think this lgtm. Thanks Jigsaw52!

Nick, you want to keep that for 031... I mean the stable has been released? What about early merge 033?

comment:21 Changed 14 months ago by nickm

Keywords: 031-backport 032-backport added; review-group-23 removed
Milestone: Tor: 0.3.1.x-finalTor: 0.3.3.x-final

Yes, I think 0.3.2 or 0.3.3 might be better, with a possible backport if it works out. The fix looks good to me, but it's subtle and we could have missed something, so it's best to try it in a pre-alpha first.

comment:22 Changed 13 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:23 Changed 13 months ago by nickm

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

Oh hey; 0.3.3 is open. Merging it there. Marking for possible 0.3.2 backport, though I lean towards "no backport" here.

comment:24 Changed 13 months ago by nickm

The branch is now fix-torrcd-sandbox-22605v2 in my public repository -- I added a changes file.

comment:25 Changed 13 months ago by nickm

b76a161e019dd808119f9e6d3bfa54990e7dcb2c is the merge commit for putting this into master. Might need to look at that one if we do choose to backport.

comment:26 Changed 13 months ago by nickm

Keywords: review-group-24 removed

comment:27 Changed 12 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Resolution: fixed
Status: merge_readyclosed

I'm thinking this is complex enough that we shouldn't backport.

comment:28 Changed 8 months ago by arma

We closed #25677 as a duplicate. (Though it was reported on 0.3.2.10, which it isn't fixed in. I guess if we get too many reports, we should reconsider whether to backport.)

Note: See TracTickets for help on using tickets.