BUG: Tor sandbox issue with libseccomp v2.4.0

[toralf]

There's a small regression with 2.40 and Tor:

https://trac.torproject.org/projects/tor/ticket/29819#comment:5

and I do wonder if this is a shortcoming of the Tor code?

[pcmoore]

Hi @toralf, unfortunately I'm not very familiar with the Tor sandboxing code. Is there anyway for you to capture the libseccomp PFC output from the Tor libseccomp filter so we can compare it with the desired Tor filter?

See the seccomp_export_pfc() function as a way of capturing the PFC output.

[pcmoore]

@drakenclimber you wouldn't happen to have any experience with Tor, would you?

[toralf]

Hi @toralf, unfortunately I'm not very familiar with the Tor sandboxing code
Well, seems to be an issue tih the Tor code, can be reproduced there now at different systems too.
Thx for your help.

[pcmoore]

Thanks for keeping us updated @toralf, if you need any help/advice on writing libseccomp filters for Tor (or any other application) please let us know, we're happy to help.

Good luck!

[toralf]

I do wonder about your opinion about https://trac.torproject.org/projects/tor/ticket/29819#comment:10 ?

[pcmoore]

Hello again. As I said previously, I'm really not very familiar with the Tor libseccomp filters and there isn't much information in the linked ticket. I think it might be helpful if you could capture the libseccomp PFC output from the Tor libseccomp filters so we can compare it to the desired filter.

[pgerber]

I got the PFC output (pfc.txt).

rt_sigaction(1 (=SIGHUP), …) is what causes the crash but I'm not sure if the PFC is trying to tell me that that's allowed or not:

  # filter for syscall "rt_sigaction" (13) [priority: 65526]
  if ($syscall == 13)
    if ($a0.hi32 == 0)
      if ($a0.lo32 == 25)
        action ALLOW;
      if ($a0.lo32 == 17)
        action ALLOW;
      if ($a0.lo32 == 15)
        action ALLOW;
      if ($a0.lo32 == 13)
        action ALLOW;
      if ($a0.lo32 == 12)
        action ALLOW;
      if ($a0.lo32 == 10)
        action ALLOW;
      if ($a0.lo32 == 2)
        action ALLOW;
      if ($a0.lo32 == 1)
        action ALLOW;

[nmathewson]

I get the same rt_sigaction filter for the case that fails. For the versions of libseccomp that succeed, I get:

  # filter for syscall "rt_sigaction" (13) [priority: 65526]
  if ($syscall == 13)
    if ($a0.hi32 == 0)
      if ($a0.lo32 == 25)
        action ALLOW;
      if ($a0.lo32 == 17)
        action ALLOW;
      if ($a0.lo32 == 1)
        action ALLOW;
      if ($a0.lo32 == 12)
        action ALLOW;
      if ($a0.lo32 == 10)
        action ALLOW;
      if ($a0.lo32 == 13)
        action ALLOW;
      if ($a0.lo32 == 15)
        action ALLOW;
      if ($a0.lo32 == 2)
        action ALLOW;

The order is difference, but the permitted signals seem to be the same.

[pcmoore]

@pgerber can you share the full PFC output for the older/success case?

[pcmoore]

@pgerber accroding to the PFC output, rt_sigaction(1, ...) should be allowed. Although it is possible that there is an issue with BPF that is generated. First I want to take a closer look at the PFC output from @pgerber.

[pgerber]

Full PFC output:

bad (libseccomp 2.4.0)
good (libseccomp 2.3.3)

[pcmoore]

Hmm, nothing is immediately jumping out at me. Would you mind capturing the BPF output? I only need the libseccomp v2.4.0 BPF output.

[pgerber]

BPF output for v2.4.0:

seccomp.bpf.gz

[pcmoore]

Thanks @pgerber, for reference, here is the disassembled BPF:

https://gist.github.com/pcmoore/ec93299cf460683464fdc3847b4cdf13

[pcmoore]

Okay, yes, it looks like the BPF is incorrect. Take the case of rt_sigaction(1, ...) using the provided BPF (see gist above):

First the normal x86_64 preamble:

 line  OP   JT   JF   K
=================================
 0000: 0x20 0x00 0x00 0x00000004   ld  $data[4]
 0001: 0x15 0x00 0x85 0xc000003e   jeq 3221225534 true:0002 false:0135
 0002: 0x20 0x00 0x00 0x00000000   ld  $data[0]
 0003: 0x35 0x00 0x01 0x40000000   jge 1073741824 true:0004 false:0005
 0004: 0x15 0x00 0x82 0xffffffff   jeq 4294967295 true:0005 false:0135

... eventually we get to rt_sigaction() with a syscall number of 13:

 0150: 0x15 0x00 0x0a 0x0000000d   jeq 13   true:0151 false:0161
 0151: 0x20 0x00 0x00 0x00000014   ld  $data[20]
 0152: 0x15 0x00 0x83 0x00000000   jeq 0    true:0153 false:0284
 0153: 0x20 0x00 0x00 0x00000010   ld  $data[16]
 0154: 0x15 0x7d 0x00 0x00000019   jeq 25   true:0280 false:0155
 0155: 0x15 0x7c 0x00 0x00000011   jeq 17   true:0280 false:0156
 0156: 0x15 0x7b 0x00 0x0000000f   jeq 15   true:0280 false:0157
 0157: 0x15 0x7a 0x00 0x0000000d   jeq 13   true:0280 false:0158
 0158: 0x15 0x79 0x00 0x0000000c   jeq 12   true:0280 false:0159
 0159: 0x15 0x78 0x00 0x0000000a   jeq 10   true:0280 false:0160
 0160: 0x15 0x77 0xc2 0x00000002   jeq 2    true:0280 false:0355

... and we don't see 1/SIGHUP listed to we need to take the false branch at the end which jumps down to line 355:

 0355: 0x20 0x00 0x00 0x00000018   ld  $data[24]
 0356: 0x54 0x00 0x00 0xfff7f7ff   and 0xfff7f7ff
 0357: 0x15 0xb8 0xb7 0x00000001   jeq 1    true:0542 false:0541

... which we can see is definitely not correct.

The PFC looks correct, so this is likely a problem with the BPF code generator. Thanks for bringing this to our attention, and providing the filter output so we could identify that the problem was with libseccomp. We'll look into this and keep this issue updated as we make progress.

[pcmoore]

@drakenclimber fixing this is obviously a priority for libseccomp, any help you can provide would be good.

[pcmoore]

That definitely made a positive difference:

# ./src/app/tor --Sandbox 1 --DataDirectory data
Apr 08 23:23:37.090 [notice] Tor 0.4.1.0-alpha-dev (git-540c52d4dc619b65) running on Linux with Libevent 2.1.8-stable, OpenSSL 1.1.1b, Zlib 1.2.11, Liblzma 5.2.4, and Libzstd 1.3.8.
Apr 08 23:23:37.090 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Apr 08 23:23:37.090 [notice] This version is not a stable Tor release. Expect more bugs than usual.
Apr 08 23:23:37.090 [notice] Configuration file "/usr/local/etc/tor/torrc" not present, using reasonable defaults.
Apr 08 23:23:37.098 [warn] Path for DataDirectory (data) is relative and will resolve to /home/pmoore/sources/libseccomp/tor-upstream/data. Is this what you wanted?
Apr 08 23:23:37.100 [notice] Opening Socks listener on 127.0.0.1:9050
Apr 08 23:23:37.100 [notice] Opened Socks listener on 127.0.0.1:9050
seccomp export status: 0
seccomp export status: 0
Apr 08 23:23:37.000 [notice] Bootstrapped 0% (starting): Starting
Apr 08 23:23:37.000 [notice] Starting with guard context "default"
Apr 08 23:23:38.000 [notice] Bootstrapped 5% (conn): Connecting to a relay
Apr 08 23:23:38.000 [notice] Bootstrapped 10% (conn_done): Connected to a relay
Apr 08 23:23:38.000 [notice] Bootstrapped 14% (handshake): Handshaking with a relay
Apr 08 23:23:38.000 [notice] Bootstrapped 15% (handshake_done): Handshake with a relay done
Apr 08 23:23:38.000 [notice] Bootstrapped 75% (enough_dirinfo): Loaded enough directory info to build circuits
Apr 08 23:23:38.000 [notice] Bootstrapped 90% (ap_handshake_done): Handshake finished with a relay to build circuits
Apr 08 23:23:38.000 [notice] Bootstrapped 95% (circuit_create): Establishing a Tor circuit
Apr 08 23:23:38.000 [notice] Bootstrapped 100% (done): Done

[pcmoore]

For reference, this is my current libseccomp patch:

commit 6e965cce0249d643bcd2f51b49adf319f7cd106b
Author: Paul Moore <paul@paul-moore.com>
Date:   Fri Mar 29 23:47:23 2019 -0400

    bpf: add accumulator state to the instruction block hash
    
    This addresses a problem where dissimilar instruction blocks were
    improperly hashed to the same value because we were not taking into
    account the accumulator state.
    
    See the GitHub issue below for more information:
     * https://github.com/seccomp/libseccomp/issues/148
    
    Signed-off-by: Paul Moore <paul@paul-moore.com>

diff --git a/src/gen_bpf.c b/src/gen_bpf.c
index 9f8f5c3..a95e0f7 100644
--- a/src/gen_bpf.c
+++ b/src/gen_bpf.c
@@ -141,6 +141,9 @@ struct bpf_blk {
 
 struct bpf_hash_bkt {
        struct bpf_blk *blk;
+       struct acc_state acc_start;
+       struct acc_state acc_end;
+
        struct bpf_hash_bkt *next;
        unsigned int found;
 };
@@ -556,7 +559,7 @@ static void _state_release(struct bpf_state *state)
 static int _hsh_add(struct bpf_state *state, struct bpf_blk **blk_p,
                    unsigned int found)
 {
-       uint64_t h_val;
+       uint64_t h_val, h_val_tmp[3];
        struct bpf_hash_bkt *h_new, *h_iter, *h_prev = NULL;
        struct bpf_blk *blk = *blk_p;
        struct bpf_blk *b_iter;
@@ -569,11 +572,16 @@ static int _hsh_add(struct bpf_state *state, struct bpf_b>
                return -ENOMEM;
 
        /* generate the hash */
-       h_val = hash(blk->blks, _BLK_MSZE(blk));
+       h_val_tmp[0] = hash(blk->blks, _BLK_MSZE(blk));
+       h_val_tmp[1] = hash(&blk->acc_start, sizeof(blk->acc_start));
+       h_val_tmp[2] = hash(&blk->acc_end, sizeof(blk->acc_end));
+       h_val = hash(h_val_tmp, sizeof(h_val_tmp));
        blk->hash = h_val;
        blk->flag_hash = true;
        blk->node = NULL;
        h_new->blk = blk;
+       h_new->acc_start = blk->acc_start;
+       h_new->acc_end = blk->acc_end;
        h_new->found = (found ? 1 : 0);
 
        /* insert the block into the hash table */
@@ -584,7 +592,9 @@ hsh_add_restart:
                        if ((h_iter->blk->hash == h_val) &&
                            (_BLK_MSZE(h_iter->blk) == _BLK_MSZE(blk)) &&
                            (memcmp(h_iter->blk->blks, blk->blks,
-                                   _BLK_MSZE(blk)) == 0)) {
+                                   _BLK_MSZE(blk)) == 0) &&
+                           _ACC_CMP_EQ(h_iter->acc_start, blk->acc_start) &&
+                           _ACC_CMP_EQ(h_iter->acc_end, blk->acc_end)) {
                                /* duplicate block */
                                free(h_new);

[pcmoore]

Let me try and explain what is happening and why I think you don't want to leave those two open(...)/openat(...) filter rules in sb_open(...).

While you had multiple open(...)/openat(...) rules, you had one rule for each open syscall that only checked the flags passed to the syscall. If any call to the open syscalls matched on Tor's flag-only open/openat filter than ERRNO(13) would be returned, regardless of any other rules that had been installed by Tor.

Does that make sense?

[pcmoore]

@drakenclimber First off, does that patch above look reasonable to you? If so, can I add your Acked-by to the patch? Regardless of anything else, I think that is a fix we should make.

On the issue of reverting some changes to preserve the ordering of the argument checks from v2.3.x, I'd like to leave things the way they are in v2.4.0 simply because now there is an explicit ordering. Prior to the v2.4.0 release, the order of argument checks was dependent on what order the rules were added to the filter, but starting with v2.4.0 we explicitly order the argument checks. While it does differ from v2.3.x, it shouldn't cause any filters to break unless there are other issues (as we've seen here).

[drakenclimber]

@pgerber I wasn't familiar with the intricacies of openat and seccomp, so I dug in deeper. It looks like your concerns are correct. The kernel is doing a pointer comparison and not a string comparison.

I wrote a small test to verify this behavior.

Here are the critical parts of it:

        char foo1[] = "foo";
        char foo2[] = "foo";

        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), 2,
                              SCMP_A0(SCMP_CMP_EQ, (uint64_t)AT_FDCWD),
                              SCMP_A1(SCMP_CMP_EQ, (uint64_t)foo1));

        /* seccomp will allow this command to succeed */
        foo1_fd = openat(AT_FDCWD, foo1, O_CREAT);

        /* seccomp will fail this command */
        foo2_fd = openat(AT_FDCWD, foo2, O_CREAT);

I'm still not certain how libseccomp v2.3.3 and Tor is not encountering this problem. The BPF for v2.3.3 and v2.4 (with the hash fix) look very similar. I'll keep researching this.

[pcmoore]

@pgerber I wasn't familiar with the intricacies of openat and seccomp, so I dug in deeper. It looks like your concerns are correct. The kernel is doing a pointer comparison and not a string comparison.

Sorry, I forgot to respond to that last night. Yes, you are both correct, for safety reasons the kernel's seccomp code doesn't look into user provided pointers.

However, as I mentioned above, the open(...)/openat(...) filters appear to be a problem with the filters that Tor has configured and not with libseccomp itself. It may have worked with libseccomp v2.3.x due to some other bugs in the libseccomp code. The libseccomp v2.4.0 released fixed a lot of bugs in the libseccomp filter generation code.

[drakenclimber]

However, as I mentioned above, the open(...)/openat(...) filters appear to be a problem with the filters that Tor has configured and not with libseccomp itself.

Thanks, @pcmoore. I agree. I waded through their code quite a bit and they are using different string pointers throughout the codebase. That will prove problematic for seccomp and the openat() command.

I did get a chance to look through the Tor's v2.3.3 openat BPF logic and the openat BPF logic looks sane. (But I will say it's a very narrow path for any openat call to be allowed. A specific pointer must be used and only a few fcntl flags are legal.) As you shared, a lot of bugs were fixed since then.

@pgerber and others - once we get the hash collision fix into libseccomp, let me know if want another set of eyes on the seccomp changes you need to make on your end.

[pgerber]

Thanks for you responses, both of you.

I left a comment on Tor's bug tracker in the hope of getting some feedback from some actual Tor developers. It would appear that there is some logic in place to statically allocate all the strings used as paths prot_strings() in sandbox.c:1239) and reuse them later (sandbox_intern_string() in sandbox:1169).

I guess what @pcmoore mentioned in an earrlier comment about there being some filters just looking at the flags of open()/openat()/... could be why Tor isn't working still.

[drakenclimber]

I was able to create a test case that reproduces the hash collision reported by Tor. I added it (and the _hsh_add() fix) to pull request #149

[pgerber]

Let me try and explain what is happening and why I think you don't want to leave those two open(...)/openat(...) filter rules in sb_open(...).

While you had multiple open(...)/openat(...) rules, you had one rule for each open syscall that only checked the flags passed to the syscall. If any call to the open syscalls matched on Tor's flag-only open/openat filter than ERRNO(13) would be returned, regardless of any other rules that had been installed by Tor.

Does that make sense?

I'm not sure I understand this fully. Tor uses this logic currently:

if path is explicitly allowed {
   allow
} elif read only access {
   // Some libraries try to read some files which they don't strictly need.
   deny with EPERM
} else {
   raise SIGSYS
}

In order to achieve this, Tor assumes that the order rules are added is the order in which they are later applied. If I understood you, @pcmoore, correctly, the order in which rules are added has no longer an impact on the order rules are applied. Did I understand this correctly? If this is indeed the case, is there still a way to restore the old logic? Also, I'd expect this to be a common pattern and I'd also expect that sooner or later others run into the same issue as Tor when upgrading to v2.4.0. Intuitively, I'd expect the rule to be applied in the user-given order.

[toralf]

I'd also expect that sooner or later others run into the same issue as Tor when upgrading to v2.4.0. Intuitively, I'd expect the rule to be applied in the user-given order.

At Gentoo these packages were at least candidates:

# equery d libseccomp
 * These packages depend on libseccomp:
app-emulation/qemu-3.1.0-r4 (seccomp ? >=sys-libs/libseccomp-2.1.0)
                            (seccomp ? >=sys-libs/libseccomp-2.1.0[static-libs(+)])
app-misc/pax-utils-1.2.3 (seccomp ? sys-libs/libseccomp)
kde-plasma/kscreenlocker-5.14.5 (seccomp ? sys-libs/libseccomp)
net-dns/bind-tools-9.12.3_p4 (seccomp ? sys-libs/libseccomp)
net-libs/gnutls-3.6.7 (seccomp ? sys-libs/libseccomp)
net-vpn/tor-0.4.0.4_rc (seccomp ? sys-libs/libseccomp)

[pcmoore]

In order to achieve this, Tor assumes that the order rules are added is the order in which they are later applied. If I understood you, @pcmoore, correctly, the order in which rules are added has no longer an impact on the order rules are applied. Did I understand this correctly?

First off I think a bit of clarification is in order. The "ordering" that @drakenclimber and I were talking about is the ordering related to checking syscall argument values for the same syscall argument. Imagine a syscall named "foo" which takes a single numerical argument, e.g. int foo(int arg);. If you created multiple libseccomp rules to filter on the argument values using the LT/LE/GT/GE then there is an ordering that must be applied to ensure that the rules are applied correctly. In libseccomp v2.4.0 we handle this correctly, and even apply a strict ordering for EQ/NE comparisons, mostly so that the filters are more predictable regardless of the order in which they are created.

The ordering you are talking about, general ordering between rules depending on the order in which they are created, is somewhat related but it is quite different in practice and not something libseccomp has ever guaranteed. Even the syscall priority functions are just "hints" to the filter optimization engine and do not guarantee a strict ordering.

If this is indeed the case, is there still a way to restore the old logic? Also, I'd expect this to be a common pattern and I'd also expect that sooner or later others run into the same issue as Tor when upgrading to v2.4.0. Intuitively, I'd expect the rule to be applied in the user-given order.

Unfortunately that is not the way libseccomp has ever worked. Thinking quickly, I'm not sure how difficult it would be, but we may be able to allow callers to request a specific rule ordering in the future, but doing so would severely handicap the rule/BPF optimizations to the point that I don't believe we could do anything meaningful.

The only guarantees that libseccomp provides regarding rule execution is that argument comparisons within a single rule are applied using a boolean "AND" when determining if a particular syscall invocation is a match, and that the individual rules are combined using a boolean "OR". I apologize if that was not clear from the beginning.

If you are having trouble adapting the Tor filter, please let us know and we can try to help.

[pcmoore]

I believe we should now have this fixed in the master and release-2.4 branches.

@drakenclimber I'm thinking we should do a v2.4.1 release soon (e.g. this week) to address this problem, what do you think? Have you seen any other bugs in the v2.4.0 release?

[drakenclimber]

I'm thinking we should do a v2.4.1 release soon (e.g. this week) to address this problem, what do you think?

Yes, I think this bug is serious enough to merit another release. Let me know how I can help.

Have you seen any other bugs in the v2.4.0 release?

Nope. In fact this fixed an issue that had been bothering me with the binary tree work. I think it's looking pretty solid.

[pcmoore]

Yes, I think this bug is serious enough to merit another release. Let me know how I can help.

Okay, since we're in agreement I'll start working through the release process for v2.4.1.

As for help ... I thought we would have a bit more time before the next release to get all our co-maintainer processes in place, but that didn't quite work to plan :) In the interest of getting this out the door quickly I'll just take care of it (it should be relatively easy given the single fix). However, I did just add issue #146 as a blocker for v2.4.2 so we're on the hook to getting you fully plugged-in for v2.4.2 ;)

You've probably already seen it, but if you haven't, I've documented the release process in https://github.com/seccomp/libseccomp/blob/master/RELEASE_PROCESS.md .

Nope. In fact this fixed an issue that had been bothering me with the binary tree work. I think it's looking pretty solid.

Fingers crossed.

[pcmoore]

@toralf @pgerber since it appears we've resolved this issue in the libseccomp upstream, I'm going to close this bug now and start the libseccomp v2.4.1 with the fix we've discussed here. If you have any additional questions or concerns, please do let us know. Thanks again for bringing this to our attention and helping us out while we arrived at a solution.

[pcmoore]

For reference, libseccomp v2.4.1 is out with the fix for this bug.

• https://github.com/seccomp/libseccomp/releases/tag/v2.4.1