Opened 6 years ago

Closed 6 years ago

#11351 closed defect (implemented)

Make sandbox code work with chutney, strong enough to be plausible

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 025-triaged, nickm-review-0254
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Using the sandbox code with chutney crashes.

Also, it doesn't restrict rename(), which makes restrictions on open() kind of pointless.

Child Tickets

Change History (17)

comment:1 Changed 6 years ago by nickm

Status: newneeds_review

See branch "sandbox_fixes" in my public repo.

comment:2 Changed 6 years ago by andrea

Keywords: 025-triaged added

comment:3 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Still looks okay to me. Merged to 0.2.5.

comment:4 Changed 6 years ago by nickm

Resolution: fixed
Status: closedreopened

oops; I didn't mean to close this one; I haven't actually merged it yet.

comment:5 Changed 6 years ago by nickm

Status: reopenedneeds_review

comment:6 Changed 6 years ago by nickm

Self-reviewing:

  • Unconditionally allowing _sysctl is kinda bogus; can we restrict it to the minimum thing we need, or just have it give EPERM? Otherwise looks fine.

comment:7 Changed 6 years ago by nickm

Rebased and squashed as "sandbox_fixes_rebased." This lets me get the benefits of #11465 as I try to figure out where that _sysctl is coming from.

comment:8 Changed 6 years ago by nickm

(currently I suspect libevent but who can say)

comment:9 Changed 6 years ago by nickm

Indeed. It's the call from evutil_secure_rng_init to sysctl() to make the stupid internal RNG get set up. This is apparently happening because opening /dev/urandom isn't working, because the libevent I have is current, and therefore doesn't have evutil_set_secure_random_device_file().

I think I need a work around... Seeding by hand might work.

comment:10 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

The other bug here is that I only checked one arg of rename(). If I make it check both, it breaks again. Investigating.

comment:11 Changed 6 years ago by nickm

okay, the problem is that sb_rename() uses sandbox_intern_string, which doesn't work until register_cfg() is called, which doesn't happen till after sb_rename().

comment:12 Changed 6 years ago by nickm

note: openat also has the "only one argument checked" problem. mprotect() is under-checked.

comment:13 Changed 6 years ago by nickm

Keywords: nickm-review-0254 added

comment:14 Changed 6 years ago by nickm

Numerous fixes in "sandbox_fixes_rebased" now. The chutney network survives HUP.

comment:15 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

comment:16 Changed 6 years ago by nickm

Latest version is "sandbox_fixes_rebased_2". I'll let you know in an hour if it has survived the "crash after an hour" bug.

comment:17 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

ok, looks good, and Andrea thinks it's unlikely to break non-sandbox-users. Excelsior!

Note: See TracTickets for help on using tickets.