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

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]

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

added 031-backport in Legacy / Trac 032-backport in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.3.x-final in Legacy / Trac owner::dgoulet in Legacy / Trac priority::high in Legacy / Trac regression in Legacy / Trac resolution::fixed in Legacy / Trac sandbox in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac version::tor 0.3.1.3-alpha in Legacy / Trac labels

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

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

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

Trac:
Priority: Medium to High
Keywords: N/A deleted, sandbox, regression added
Milestone: N/A to Tor: 0.3.1.x-final

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.

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.

Trac:
Status: new to accepted
Owner: N/A to dgoulet

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

Trac:
Cc: N/A to danielpinto52@gmail.com

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

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".

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.

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.

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.

I gave it a shot here: bug22605_031_01

Trac:
Status: accepted to needs_review

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.

Trac:
Status: needs_review to needs_revision

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.

Trac:
Status: needs_revision to needs_review

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

Trac:
Keywords: N/A deleted, review-group-23 added

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

• Why adding this? SCMP_SYS(getdents),

Trac:
Status: needs_review to needs_revision

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.

Trac:
Status: needs_revision to needs_review

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?

Trac:
Status: needs_review to merge_ready

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.

Trac:
Keywords: review-group-23 deleted, 031-backport 032-backport added
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.3.x-final

review-group-24 is now open.

Trac:
Keywords: 031-backport 032-backport deleted, review-group-24, 032-backport, 031-backport added

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

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.2.x-final

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

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

Trac:
Keywords: review-group-24 deleted, N/A added

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

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.3.x-final
Resolution: N/A to fixed
Status: merge_ready to closed

We closed legacy/trac#25677 (moved) 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.)

closed

mentioned in issue legacy/trac#25677 (moved)

moved from legacy/trac#22605 (moved)

added Bug label and removed 1 deleted label

added Regression label and removed 1 deleted label