From fe7d208cf76fa6f24bb9d12ba29b5ed61d837ce3 Mon Sep 17 00:00:00 2001 From: Ophestra Date: Thu, 13 Feb 2025 23:15:34 +0900 Subject: [PATCH] helper: use generic extra files interface This replaces the pipes object and integrates context into helper process lifecycle. Signed-off-by: Ophestra --- dbus/dbus_test.go | 27 ++---- dbus/proxy.go | 24 ++--- dbus/run.go | 69 ++++++++++----- helper/bwrap.go | 112 +++++------------------- helper/bwrap/arg.go | 94 +++++--------------- helper/bwrap/seccomp.go | 52 +++++------ helper/bwrap/trivial.go | 52 +++++++++++ helper/bwrap_test.go | 47 ++-------- helper/direct.go | 79 +++-------------- helper/direct_test.go | 17 +--- helper/helper.go | 130 ++++++++++++++++++++++++---- helper/helper_test.go | 114 +++++++++--------------- helper/pipe.go | 149 -------------------------------- helper/pipe_test.go | 42 --------- helper/seccomp/api.go | 47 ++++++---- helper/seccomp/seccomp-export.c | 8 -- helper/seccomp/seccomp-export.h | 1 - helper/seccomp/seccomp.go | 9 -- helper/stub.go | 136 ++++++++++++++--------------- internal/app/start.go | 2 +- internal/priv/shim/main.go | 25 ++++-- internal/system/dbus.go | 72 ++------------- internal/system/op.go | 21 +++-- ldd/exec.go | 24 ++--- main.go | 14 +-- 25 files changed, 517 insertions(+), 850 deletions(-) create mode 100644 helper/bwrap/trivial.go delete mode 100644 helper/pipe.go delete mode 100644 helper/pipe_test.go diff --git a/dbus/dbus_test.go b/dbus/dbus_test.go index 7128c08..57a441a 100644 --- a/dbus/dbus_test.go +++ b/dbus/dbus_test.go @@ -1,9 +1,11 @@ package dbus_test import ( + "context" "errors" "strings" "testing" + "time" "git.gensokyo.uk/security/fortify/dbus" "git.gensokyo.uk/security/fortify/helper" @@ -141,7 +143,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { t.Run("unsealed start of "+id, func(t *testing.T) { want := "proxy not sealed" - if err := p.Start(nil, nil, sandbox, false); err == nil || err.Error() != want { + if err := p.Start(context.Background(), nil, sandbox); err == nil || err.Error() != want { t.Errorf("Start() error = %v, wantErr %q", err, errors.New(want)) return @@ -149,7 +151,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { }) t.Run("unsealed wait of "+id, func(t *testing.T) { - wantErr := "proxy not started" + wantErr := "dbus: not started" if err := p.Wait(); err == nil || err.Error() != wantErr { t.Errorf("Wait() error = %v, wantErr %v", err, wantErr) @@ -175,7 +177,10 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { } t.Run("sealed start of "+id, func(t *testing.T) { - if err := p.Start(nil, output, sandbox, false); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + if err := p.Start(ctx, output, sandbox); err != nil { t.Fatalf("Start(nil, nil) error = %v", err) } @@ -189,22 +194,8 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { } }) - t.Run("sealed closing of "+id+" without status", func(t *testing.T) { - wantPanic := "attempted to close helper with no status pipe" - defer func() { - if r := recover(); r != wantPanic { - t.Errorf("Close() panic = %v, wantPanic %v", - r, wantPanic) - } - }() - - if err := p.Close(); err != nil { - t.Errorf("Close() error = %v", - err) - } - }) - t.Run("started wait of "+id, func(t *testing.T) { + p.Close() if err := p.Wait(); err != nil { t.Errorf("Wait() error = %v\noutput: %s", err, output.String()) diff --git a/dbus/proxy.go b/dbus/proxy.go index e547855..77307dc 100644 --- a/dbus/proxy.go +++ b/dbus/proxy.go @@ -1,6 +1,7 @@ package dbus import ( + "context" "errors" "fmt" "io" @@ -19,29 +20,21 @@ var ProxyName = "xdg-dbus-proxy" type Proxy struct { helper helper.Helper bwrap *bwrap.Config + ctx context.Context + cancel context.CancelCauseFunc name string session [2]string system [2]string + sysP bool seal io.WriterTo lock sync.RWMutex } -func (p *Proxy) Session() [2]string { - return p.session -} - -func (p *Proxy) System() [2]string { - return p.system -} - -func (p *Proxy) Sealed() bool { - p.lock.RLock() - defer p.lock.RUnlock() - - return p.seal != nil -} +func (p *Proxy) Session() [2]string { return p.session } +func (p *Proxy) System() [2]string { return p.system } +func (p *Proxy) Sealed() bool { p.lock.RLock(); defer p.lock.RUnlock(); return p.seal != nil } var ( ErrConfig = errors.New("no configuration to seal") @@ -56,7 +49,7 @@ func (p *Proxy) String() string { defer p.lock.RUnlock() if p.helper != nil { - return p.helper.Unwrap().String() + return p.helper.String() } if p.seal != nil { @@ -96,6 +89,7 @@ func (p *Proxy) Seal(session, system *Config) error { } if system != nil { args = append(args, system.Args(p.system)...) + p.sysP = true } if seal, err := helper.NewCheckedArgs(args); err != nil { return err diff --git a/dbus/run.go b/dbus/run.go index 0dfe7f8..087990d 100644 --- a/dbus/run.go +++ b/dbus/run.go @@ -1,8 +1,10 @@ package dbus import ( + "context" "errors" "io" + "os" "os/exec" "path" "path/filepath" @@ -14,9 +16,8 @@ import ( "git.gensokyo.uk/security/fortify/ldd" ) -// Start launches the D-Bus proxy and sets up the Wait method. -// ready should be buffered and must only be received from once. -func (p *Proxy) Start(ready chan error, output io.Writer, sandbox, seccomp bool) error { +// Start launches the D-Bus proxy. +func (p *Proxy) Start(ctx context.Context, output io.Writer, sandbox bool) error { p.lock.Lock() defer p.lock.Unlock() @@ -25,8 +26,7 @@ func (p *Proxy) Start(ready chan error, output io.Writer, sandbox, seccomp bool) } var ( - h helper.Helper - cmd *exec.Cmd + h helper.Helper argF = func(argsFD, statFD int) []string { if statFD == -1 { @@ -39,9 +39,8 @@ func (p *Proxy) Start(ready chan error, output io.Writer, sandbox, seccomp bool) if !sandbox { h = helper.New(p.seal, p.name, argF) - cmd = h.Unwrap() // xdg-dbus-proxy does not need to inherit the environment - cmd.Env = []string{} + h.SetEnv(make([]string, 0)) } else { // look up absolute path if name is just a file name toolPath := p.name @@ -56,7 +55,7 @@ func (p *Proxy) Start(ready chan error, output io.Writer, sandbox, seccomp bool) // resolve libraries by parsing ldd output var proxyDeps []*ldd.Entry if toolPath != "/nonexistent-xdg-dbus-proxy" { - if l, err := ldd.Exec(toolPath); err != nil { + if l, err := ldd.Exec(ctx, toolPath); err != nil { return err } else { proxyDeps = l @@ -73,10 +72,6 @@ func (p *Proxy) Start(ready chan error, output io.Writer, sandbox, seccomp bool) DieWithParent: true, } - if !seccomp { - bc.Syscall = nil - } - // resolve proxy socket directories bindTarget := make(map[string]struct{}, 2) for _, ps := range []string{p.session[1], p.system[1]} { @@ -116,35 +111,65 @@ func (p *Proxy) Start(ready chan error, output io.Writer, sandbox, seccomp bool) } h = helper.MustNewBwrap(bc, toolPath, p.seal, argF, nil, nil) - cmd = h.Unwrap() p.bwrap = bc } if output != nil { - cmd.Stdout = output - cmd.Stderr = output + h.Stdout(output).Stderr(output) } - if err := h.StartNotify(ready); err != nil { + c, cancel := context.WithCancelCause(ctx) + if err := h.Start(c, true); err != nil { + cancel(err) return err } p.helper = h + p.ctx = c + p.cancel = cancel return nil } -// Wait waits for xdg-dbus-proxy to exit or fault. +var proxyClosed = errors.New("proxy closed") + +// Wait blocks until xdg-dbus-proxy exits and releases resources. func (p *Proxy) Wait() error { p.lock.RLock() defer p.lock.RUnlock() if p.helper == nil { - return errors.New("proxy not started") + return errors.New("dbus: not started") } - return p.helper.Wait() + errs := make([]error, 3) + + errs[0] = p.helper.Wait() + if p.cancel == nil && + errors.Is(errs[0], context.Canceled) && + errors.Is(context.Cause(p.ctx), proxyClosed) { + errs[0] = nil + } + + // ensure socket removal so ephemeral directory is empty at revert + if err := os.Remove(p.session[1]); err != nil && !errors.Is(err, os.ErrNotExist) { + errs[1] = err + } + if p.sysP { + if err := os.Remove(p.system[1]); err != nil && !errors.Is(err, os.ErrNotExist) { + errs[2] = err + } + } + + return errors.Join(errs...) } -// Close closes the status file descriptor passed to xdg-dbus-proxy, causing it to stop. -func (p *Proxy) Close() error { - return p.helper.Close() +// Close cancels the context passed to the helper instance attached to xdg-dbus-proxy. +func (p *Proxy) Close() { + p.lock.Lock() + defer p.lock.Unlock() + + if p.cancel == nil { + panic("dbus: not started") + } + p.cancel(proxyClosed) + p.cancel = nil } diff --git a/helper/bwrap.go b/helper/bwrap.go index 851f244..8044e6a 100644 --- a/helper/bwrap.go +++ b/helper/bwrap.go @@ -1,111 +1,47 @@ package helper import ( + "context" "errors" "io" "os" - "os/exec" + "slices" "strconv" "sync" "git.gensokyo.uk/security/fortify/helper/bwrap" + "git.gensokyo.uk/security/fortify/helper/proc" ) // BubblewrapName is the file name or path to bubblewrap. var BubblewrapName = "bwrap" type bubblewrap struct { - // bwrap child file name + // final args fd of bwrap process + argsFd uintptr + + // name of the command to run in bwrap name string - // bwrap pipes - control *pipes - // returns an array of arguments passed directly - // to the child process spawned by bwrap - argF func(argsFD, statFD int) []string - - // pipes received by the child - // nil if no pipes are required - controlPt *pipes - lock sync.RWMutex - *exec.Cmd + *helperCmd } -func (b *bubblewrap) StartNotify(ready chan error) error { +func (b *bubblewrap) Start(ctx context.Context, stat bool) error { b.lock.Lock() defer b.lock.Unlock() - if ready != nil && b.controlPt == nil { - panic("attempted to start with status monitoring on a bwrap child initialised without pipes") - } - // Check for doubled Start calls before we defer failure cleanup. If the prior // call to Start succeeded, we don't want to spuriously close its pipes. - if b.Cmd.Process != nil { + if b.Cmd != nil && b.Cmd.Process != nil { return errors.New("exec: already started") } - // prepare bwrap pipe and args - if argsFD, _, err := b.control.prepareCmd(b.Cmd); err != nil { - return err - } else { - b.Cmd.Args = append(b.Cmd.Args, "--args", strconv.Itoa(argsFD), "--", b.name) - } - - // prepare child args and pipes if enabled - if b.controlPt != nil { - b.controlPt.ready = ready - if argsFD, statFD, err := b.controlPt.prepareCmd(b.Cmd); err != nil { - return err - } else { - b.Cmd.Args = append(b.Cmd.Args, b.argF(argsFD, statFD)...) - } - } else { - b.Cmd.Args = append(b.Cmd.Args, b.argF(-1, -1)...) - } - - if ready != nil { - b.Cmd.Env = append(b.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=1") - } else if b.controlPt != nil { - b.Cmd.Env = append(b.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=0") - } else { - b.Cmd.Env = append(b.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=-1") - } - - if err := b.Cmd.Start(); err != nil { - return err - } - - // write bwrap args first - if err := b.control.readyWriteArgs(); err != nil { - return err - } - - // write child args if enabled - if b.controlPt != nil { - if err := b.controlPt.readyWriteArgs(); err != nil { - return err - } - } - - return nil -} - -func (b *bubblewrap) Close() error { - if b.controlPt == nil { - panic("attempted to close bwrap child initialised without pipes") - } - - return b.controlPt.closeStatus() -} - -func (b *bubblewrap) Start() error { - return b.StartNotify(nil) -} - -func (b *bubblewrap) Unwrap() *exec.Cmd { - return b.Cmd + args := b.finalise(ctx, stat) + b.Cmd.Args = slices.Grow(b.Cmd.Args, 4+len(args)) + b.Cmd.Args = append(b.Cmd.Args, "--args", strconv.Itoa(int(b.argsFd)), "--", b.name) + b.Cmd.Args = append(b.Cmd.Args, args...) + return proc.Fulfill(ctx, b.Cmd, b.files, b.extraFiles) } // MustNewBwrap initialises a new Bwrap instance with wt as the null-terminated argument writer. @@ -130,27 +66,23 @@ func MustNewBwrap( // Function argF returns an array of arguments passed directly to the child process. func NewBwrap( conf *bwrap.Config, name string, - wt io.WriterTo, argF func(argsFD, statFD int) []string, + wt io.WriterTo, argF func(argsFd, statFd int) []string, extraFiles []*os.File, syncFd *os.File, ) (Helper, error) { b := new(bubblewrap) - b.argF = argF b.name = name - if wt != nil { - b.controlPt = &pipes{args: wt} - } + b.helperCmd = newHelperCmd(b, BubblewrapName, wt, argF, extraFiles) - b.Cmd = execCommand(BubblewrapName) - b.control = new(pipes) args := conf.Args() - if fdArgs, err := conf.FDArgs(syncFd, &extraFiles); err != nil { - return nil, err - } else if b.control.args, err = NewCheckedArgs(append(args, fdArgs...)); err != nil { + conf.FDArgs(syncFd, &args, b.extraFiles, &b.files) + if v, err := NewCheckedArgs(args); err != nil { return nil, err } else { - b.Cmd.ExtraFiles = extraFiles + f := proc.NewWriterTo(v) + b.argsFd = proc.InitFile(f, b.extraFiles) + b.files = append(b.files, f) } return b, nil diff --git a/helper/bwrap/arg.go b/helper/bwrap/arg.go index d69c597..050643e 100644 --- a/helper/bwrap/arg.go +++ b/helper/bwrap/arg.go @@ -1,10 +1,8 @@ package bwrap import ( - "encoding/gob" "os" "slices" - "strconv" "git.gensokyo.uk/security/fortify/helper/proc" ) @@ -20,68 +18,8 @@ type FSBuilder interface { } type FDBuilder interface { - Len() int - Append(args *[]string, extraFiles *[]*os.File) error -} - -func init() { - gob.Register(new(pairF)) - gob.Register(new(stringF)) -} - -type pairF [3]string - -func (p *pairF) Path() string { - return p[2] -} - -func (p *pairF) Len() int { - return len(p) // compiler replaces this with 3 -} - -func (p *pairF) Append(args *[]string) { - *args = append(*args, p[0], p[1], p[2]) -} - -type stringF [2]string - -func (s stringF) Path() string { - return s[1] -} - -func (s stringF) Len() int { - return len(s) // compiler replaces this with 2 -} - -func (s stringF) Append(args *[]string) { - *args = append(*args, s[0], s[1]) -} - -type fileF struct { - name string - file *os.File -} - -func (f *fileF) Len() int { - if f.file == nil { - return 0 - } - return 2 -} - -func (f *fileF) Append(args *[]string, extraFiles *[]*os.File) error { - if f.file == nil { - return nil - } - extraFile(args, extraFiles, f.name, f.file) - return nil -} - -func extraFile(args *[]string, extraFiles *[]*os.File, name string, f *os.File) { - if f == nil { - return - } - *args = append(*args, name, strconv.Itoa(int(proc.ExtraFileSlice(extraFiles, f)))) + proc.File + Builder } // Args returns a slice of bwrap args corresponding to c. @@ -115,24 +53,36 @@ func (c *Config) Args() (args []string) { return } -func (c *Config) FDArgs(syncFd *os.File, extraFiles *[]*os.File) (args []string, err error) { +func (c *Config) FDArgs(syncFd *os.File, args *[]string, extraFiles *proc.ExtraFilesPre, files *[]proc.File) { builders := []FDBuilder{ - &seccompBuilder{c}, - &fileF{positionalArgs[SyncFd], syncFd}, + c.seccompArgs(), + newFile(positionalArgs[SyncFd], syncFd), } argc := 0 + fc := 0 for _, b := range builders { - argc += b.Len() + l := b.Len() + if l < 1 { + continue + } + argc += l + fc++ + + proc.InitFile(b, extraFiles) } - args = make([]string, 0, argc) - *extraFiles = slices.Grow(*extraFiles, len(builders)) + fc++ // allocate extra slot for stat fd + *args = slices.Grow(*args, argc) + *files = slices.Grow(*files, fc) for _, b := range builders { - if err = b.Append(&args, extraFiles); err != nil { - break + if b.Len() < 1 { + continue } + + b.Append(args) + *files = append(*files, b) } return } diff --git a/helper/bwrap/seccomp.go b/helper/bwrap/seccomp.go index 860d720..d6f34df 100644 --- a/helper/bwrap/seccomp.go +++ b/helper/bwrap/seccomp.go @@ -2,8 +2,9 @@ package bwrap import ( "fmt" - "os" + "strconv" + "git.gensokyo.uk/security/fortify/helper/proc" "git.gensokyo.uk/security/fortify/helper/seccomp" "git.gensokyo.uk/security/fortify/internal/fmsg" ) @@ -23,35 +24,13 @@ type SyscallPolicy struct { Bluetooth bool `json:"bluetooth"` } -type seccompBuilder struct { - config *Config -} - -func (s *seccompBuilder) Len() int { - if s == nil { - return 0 - } - return 2 -} - -func (s *seccompBuilder) Append(args *[]string, extraFiles *[]*os.File) error { - if s == nil { - return nil - } - if f, err := s.config.resolveSeccomp(); err != nil { - return err - } else { - extraFile(args, extraFiles, positionalArgs[Seccomp], f) - return nil - } -} - -func (c *Config) resolveSeccomp() (*os.File, error) { +func (c *Config) seccompArgs() FDBuilder { + // explicitly disable syscall filter if c.Syscall == nil { - return nil, nil + // nil File skips builder + return new(seccompBuilder) } - // resolve seccomp filter opts var ( opts seccomp.SyscallOpts optd []string @@ -86,5 +65,22 @@ func (c *Config) resolveSeccomp() (*os.File, error) { seccomp.CPrintln(fmt.Sprintf("seccomp flags: %s", optd)) } - return seccomp.Export(opts) + return &seccompBuilder{seccomp.NewFile(opts)} +} + +type seccompBuilder struct{ proc.File } + +func (s *seccompBuilder) Len() int { + if s == nil || s.File == nil { + return 0 + } + return 2 +} + +func (s *seccompBuilder) Append(args *[]string) { + if s == nil || s.File == nil { + return + } + + *args = append(*args, positionalArgs[Seccomp], strconv.Itoa(int(s.Fd()))) } diff --git a/helper/bwrap/trivial.go b/helper/bwrap/trivial.go new file mode 100644 index 0000000..5aa8a1e --- /dev/null +++ b/helper/bwrap/trivial.go @@ -0,0 +1,52 @@ +package bwrap + +import ( + "context" + "encoding/gob" + "os" + "strconv" + + "git.gensokyo.uk/security/fortify/helper/proc" +) + +func init() { + gob.Register(new(pairF)) + gob.Register(new(stringF)) +} + +type pairF [3]string + +func (p *pairF) Path() string { return p[2] } +func (p *pairF) Len() int { return len(p) } +func (p *pairF) Append(args *[]string) { *args = append(*args, p[0], p[1], p[2]) } + +type stringF [2]string + +func (s stringF) Path() string { return s[1] } +func (s stringF) Len() int { return len(s) /* compiler replaces this with 2 */ } +func (s stringF) Append(args *[]string) { *args = append(*args, s[0], s[1]) } + +func newFile(name string, f *os.File) FDBuilder { return &fileF{name: name, file: f} } + +type fileF struct { + name string + file *os.File + proc.BaseFile +} + +func (f *fileF) ErrCount() int { return 0 } +func (f *fileF) Fulfill(_ context.Context, _ func(error)) error { f.Set(f.file); return nil } + +func (f *fileF) Len() int { + if f.file == nil { + return 0 + } + return 2 +} + +func (f *fileF) Append(args *[]string) { + if f.file == nil { + return + } + *args = append(*args, f.name, strconv.Itoa(int(f.Fd()))) +} diff --git a/helper/bwrap_test.go b/helper/bwrap_test.go index 0860d57..1569a82 100644 --- a/helper/bwrap_test.go +++ b/helper/bwrap_test.go @@ -1,11 +1,12 @@ package helper_test import ( + "context" "errors" - "fmt" "os" "strings" "testing" + "time" "git.gensokyo.uk/security/fortify/helper" "git.gensokyo.uk/security/fortify/helper/bwrap" @@ -13,9 +14,7 @@ import ( func TestBwrap(t *testing.T) { sc := &bwrap.Config{ - Unshare: nil, Net: true, - UserNS: false, Hostname: "localhost", Chdir: "/nonexistent", Clearenv: true, @@ -37,8 +36,8 @@ func TestBwrap(t *testing.T) { nil, nil, ) - if err := h.Start(); !errors.Is(err, os.ErrNotExist) { - t.Errorf("Start() error = %v, wantErr %v", + if err := h.Start(context.Background(), false); !errors.Is(err, os.ErrNotExist) { + t.Errorf("Start: error = %v, wantErr %v", err, os.ErrNotExist) } }) @@ -71,23 +70,6 @@ func TestBwrap(t *testing.T) { ) }) - t.Run("start notify without pipes panic", func(t *testing.T) { - defer func() { - wantPanic := "attempted to start with status monitoring on a bwrap child initialised without pipes" - if r := recover(); r != wantPanic { - t.Errorf("StartNotify: panic = %q, want %q", - r, wantPanic) - } - }() - - panic(fmt.Sprintf("unreachable: %v", - helper.MustNewBwrap( - sc, "fortify", - nil, argF, - nil, nil, - ).StartNotify(make(chan error)))) - }) - t.Run("start without pipes", func(t *testing.T) { helper.InternalReplaceExecCommand(t) @@ -96,26 +78,15 @@ func TestBwrap(t *testing.T) { nil, argFChecked, nil, nil, ) - cmd := h.Unwrap() stdout, stderr := new(strings.Builder), new(strings.Builder) - cmd.Stdout, cmd.Stderr = stdout, stderr + h.Stdout(stdout).Stderr(stderr) - t.Run("close without pipes panic", func(t *testing.T) { - defer func() { - wantPanic := "attempted to close bwrap child initialised without pipes" - if r := recover(); r != wantPanic { - t.Errorf("Close: panic = %q, want %q", - r, wantPanic) - } - }() + c, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() - panic(fmt.Sprintf("unreachable: %v", - h.Close())) - }) - - if err := h.Start(); err != nil { - t.Errorf("Start() error = %v", + if err := h.Start(c, false); err != nil { + t.Errorf("Start: error = %v", err) return } diff --git a/helper/direct.go b/helper/direct.go index 91e4045..9198952 100644 --- a/helper/direct.go +++ b/helper/direct.go @@ -1,93 +1,40 @@ package helper import ( + "context" "errors" "io" - "os/exec" "sync" + + "git.gensokyo.uk/security/fortify/helper/proc" ) // direct wraps *exec.Cmd and manages status and args fd. // Args is always 3 and status if set is always 4. type direct struct { - // helper pipes - // cannot be nil - p *pipes - - // returns an array of arguments passed directly - // to the helper process - argF func(argsFD, statFD int) []string - lock sync.RWMutex - *exec.Cmd + *helperCmd } -func (h *direct) StartNotify(ready chan error) error { +func (h *direct) Start(ctx context.Context, stat bool) error { h.lock.Lock() defer h.lock.Unlock() // Check for doubled Start calls before we defer failure cleanup. If the prior // call to Start succeeded, we don't want to spuriously close its pipes. - if h.Cmd.Process != nil { + if h.Cmd != nil && h.Cmd.Process != nil { return errors.New("exec: already started") } - h.p.ready = ready - if argsFD, statFD, err := h.p.prepareCmd(h.Cmd); err != nil { - return err - } else { - h.Cmd.Args = append(h.Cmd.Args, h.argF(argsFD, statFD)...) - } - - if ready != nil { - h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=1") - } else { - h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=0") - } - - if err := h.Cmd.Start(); err != nil { - return err - } - if err := h.p.readyWriteArgs(); err != nil { - return err - } - - return nil -} - -func (h *direct) Wait() error { - h.lock.RLock() - defer h.lock.RUnlock() - - if h.Cmd.Process == nil { - return errors.New("exec: not started") - } - defer h.p.mustClosePipes() - if h.Cmd.ProcessState != nil { - return errors.New("exec: Wait was already called") - } - - return h.Cmd.Wait() -} - -func (h *direct) Close() error { - return h.p.closeStatus() -} - -func (h *direct) Start() error { - return h.StartNotify(nil) -} - -func (h *direct) Unwrap() *exec.Cmd { - return h.Cmd + args := h.finalise(ctx, stat) + h.Cmd.Args = append(h.Cmd.Args, args...) + return proc.Fulfill(ctx, h.Cmd, h.files, h.extraFiles) } // New initialises a new direct Helper instance with wt as the null-terminated argument writer. // Function argF returns an array of arguments passed directly to the child process. -func New(wt io.WriterTo, name string, argF func(argsFD, statFD int) []string) Helper { - if wt == nil { - panic("attempted to create helper with invalid argument writer") - } - - return &direct{p: &pipes{args: wt}, argF: argF, Cmd: execCommand(name)} +func New(wt io.WriterTo, name string, argF func(argsFd, statFd int) []string) Helper { + d := new(direct) + d.helperCmd = newHelperCmd(d, name, wt, argF, nil) + return d } diff --git a/helper/direct_test.go b/helper/direct_test.go index 1832357..67b8682 100644 --- a/helper/direct_test.go +++ b/helper/direct_test.go @@ -1,6 +1,7 @@ package helper_test import ( + "context" "errors" "os" "testing" @@ -12,8 +13,8 @@ func TestDirect(t *testing.T) { t.Run("start non-existent helper path", func(t *testing.T) { h := helper.New(argsWt, "/nonexistent", argF) - if err := h.Start(); !errors.Is(err, os.ErrNotExist) { - t.Errorf("Start() error = %v, wantErr %v", + if err := h.Start(context.Background(), false); !errors.Is(err, os.ErrNotExist) { + t.Errorf("Start: error = %v, wantErr %v", err, os.ErrNotExist) } }) @@ -26,18 +27,6 @@ func TestDirect(t *testing.T) { } }) - t.Run("invalid new helper panic", func(t *testing.T) { - defer func() { - wantPanic := "attempted to create helper with invalid argument writer" - if r := recover(); r != wantPanic { - t.Errorf("New: panic = %q, want %q", - r, wantPanic) - } - }() - - helper.New(nil, "fortify", argF) - }) - t.Run("implementation compliance", func(t *testing.T) { testHelper(t, func() helper.Helper { return helper.New(argsWt, "crash-test-dummy", argF) }) }) diff --git a/helper/helper.go b/helper/helper.go index d3115c5..2ef1afa 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -2,35 +2,133 @@ package helper import ( - "errors" + "context" + "fmt" + "io" + "os" "os/exec" + "slices" + "syscall" + "time" + + "git.gensokyo.uk/security/fortify/helper/proc" ) var ( - ErrStatusFault = errors.New("generic status pipe fault") - ErrStatusRead = errors.New("unexpected status response") + WaitDelay = 2 * time.Second ) const ( - // FortifyHelper is set for the process launched by Helper. + // FortifyHelper is set to 1 when args fd is enabled and 0 otherwise. FortifyHelper = "FORTIFY_HELPER" - // FortifyStatus is 1 when sync fd is enabled and 0 otherwise. + // FortifyStatus is set to 1 when stat fd is enabled and 0 otherwise. FortifyStatus = "FORTIFY_STATUS" ) type Helper interface { - // StartNotify starts the helper process. - // A status pipe is passed to the helper if ready is not nil. - StartNotify(ready chan error) error + // Stdin sets the standard input of Helper. + Stdin(r io.Reader) Helper + // Stdout sets the standard output of Helper. + Stdout(w io.Writer) Helper + // Stderr sets the standard error of Helper. + Stderr(w io.Writer) Helper + // SetEnv sets the environment of Helper. + SetEnv(env []string) Helper + // Start starts the helper process. - Start() error - // Close closes the status pipe. - // If helper is started without the status pipe, Close panics. - Close() error - // Wait calls wait on the child process and cleans up pipes. + // A status pipe is passed to the helper if stat is true. + Start(ctx context.Context, stat bool) error + // Wait blocks until Helper exits and releases all its resources. Wait() error - // Unwrap returns the underlying exec.Cmd instance. - Unwrap() *exec.Cmd + + fmt.Stringer } -var execCommand = exec.Command +func newHelperCmd( + h Helper, name string, + wt io.WriterTo, argF func(argsFd, statFd int) []string, + extraFiles []*os.File, +) (cmd *helperCmd) { + cmd = new(helperCmd) + + cmd.r = h + cmd.name = name + + cmd.extraFiles = new(proc.ExtraFilesPre) + for _, f := range extraFiles { + _, v := cmd.extraFiles.Append() + *v = f + } + + argsFd := -1 + if wt != nil { + f := proc.NewWriterTo(wt) + argsFd = int(proc.InitFile(f, cmd.extraFiles)) + cmd.files = append(cmd.files, f) + cmd.hasArgsFd = true + } + cmd.argF = func(statFd int) []string { return argF(argsFd, statFd) } + + return +} + +// helperCmd wraps Cmd and implements methods shared across all Helper implementations. +type helperCmd struct { + // ref to parent + r Helper + + // returns an array of arguments passed directly + // to the helper process + argF func(statFd int) []string + // whether argsFd is present + hasArgsFd bool + + // closes statFd + stat io.Closer + // deferred extraFiles fulfillment + files []proc.File + // passed through to [proc.Fulfill] and [proc.InitFile] + extraFiles *proc.ExtraFilesPre + + name string + stdin io.Reader + stdout, stderr io.Writer + env []string + *exec.Cmd +} + +func (h *helperCmd) Stdin(r io.Reader) Helper { h.stdin = r; return h.r } +func (h *helperCmd) Stdout(w io.Writer) Helper { h.stdout = w; return h.r } +func (h *helperCmd) Stderr(w io.Writer) Helper { h.stderr = w; return h.r } +func (h *helperCmd) SetEnv(env []string) Helper { h.env = env; return h.r } + +// finalise initialises the underlying [exec.Cmd] object. +func (h *helperCmd) finalise(ctx context.Context, stat bool) (args []string) { + h.Cmd = commandContext(ctx, h.name) + h.Cmd.Stdin, h.Cmd.Stdout, h.Cmd.Stderr = h.stdin, h.stdout, h.stderr + h.Cmd.Env = slices.Grow(h.env, 2) + if h.hasArgsFd { + h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1") + } else { + h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=0") + } + + h.Cmd.Cancel = func() error { return h.Cmd.Process.Signal(syscall.SIGTERM) } + h.Cmd.WaitDelay = WaitDelay + + statFd := -1 + if stat { + f := proc.NewStat(&h.stat) + statFd = int(proc.InitFile(f, h.extraFiles)) + h.files = append(h.files, f) + h.Cmd.Env = append(h.Cmd.Env, FortifyStatus+"=1") + + // stat is populated on fulfill + h.Cmd.Cancel = func() error { return h.stat.Close() } + } else { + h.Cmd.Env = append(h.Cmd.Env, FortifyStatus+"=0") + } + return h.argF(statFd) +} + +var commandContext = exec.CommandContext diff --git a/helper/helper_test.go b/helper/helper_test.go index 92a8c55..69be474 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -1,6 +1,9 @@ package helper_test import ( + "context" + "errors" + "fmt" "strconv" "strings" "testing" @@ -23,20 +26,23 @@ var ( argsWt = helper.MustNewCheckedArgs(wantArgs) ) -func argF(argsFD, statFD int) []string { - if argsFD == -1 { +func argF(argsFd, statFd int) []string { + if argsFd == -1 { panic("invalid args fd") } - return argFChecked(argsFD, statFD) + return argFChecked(argsFd, statFd) } -func argFChecked(argsFD, statFD int) []string { - if statFD == -1 { - return []string{"--args", strconv.Itoa(argsFD)} - } else { - return []string{"--args", strconv.Itoa(argsFD), "--fd", strconv.Itoa(statFD)} +func argFChecked(argsFd, statFd int) (args []string) { + args = make([]string, 0, 4) + if argsFd > -1 { + args = append(args, "--args", strconv.Itoa(argsFd)) } + if statFd > -1 { + args = append(args, "--fd", strconv.Itoa(statFd)) + } + return } // this function tests an implementation of the helper.Helper interface @@ -45,66 +51,42 @@ func testHelper(t *testing.T, createHelper func() helper.Helper) { t.Run("start helper with status channel and wait", func(t *testing.T) { h := createHelper() - ready := make(chan error, 1) - cmd := h.Unwrap() stdout, stderr := new(strings.Builder), new(strings.Builder) - cmd.Stdout, cmd.Stderr = stdout, stderr + h.Stdout(stdout).Stderr(stderr) t.Run("wait not yet started helper", func(t *testing.T) { - wantErr := "exec: not started" - if err := h.Wait(); err != nil && err.Error() != wantErr { - t.Errorf("Wait(%v) error = %v, wantErr %v", - ready, - err, wantErr) - return - } + defer func() { + r := recover() + if r == nil { + t.Fatalf("Wait did not panic") + } + }() + panic(fmt.Sprintf("unreachable: %v", h.Wait())) }) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Log("starting helper stub") - if err := h.StartNotify(ready); err != nil { - t.Errorf("StartNotify(%v) error = %v", - ready, - err) + if err := h.Start(ctx, true); err != nil { + t.Errorf("Start: error = %v", err) + cancel() return } + t.Log("cancelling context") + cancel() t.Run("start already started helper", func(t *testing.T) { wantErr := "exec: already started" - if err := h.StartNotify(ready); err != nil && err.Error() != wantErr { - t.Errorf("StartNotify(%v) error = %v, wantErr %v", - ready, + if err := h.Start(ctx, true); err != nil && err.Error() != wantErr { + t.Errorf("Start: error = %v, wantErr %v", err, wantErr) return } }) - t.Log("waiting on status channel with timeout") - select { - case <-time.NewTimer(5 * time.Second).C: - t.Errorf("never got a ready response") - t.Errorf("stdout:\n%s", stdout.String()) - t.Errorf("stderr:\n%s", stderr.String()) - if err := cmd.Process.Kill(); err != nil { - panic(err.Error()) - } - return - case err := <-ready: - if err != nil { - t.Errorf("StartNotify(%v) latent error = %v", - ready, - err) - } - } - - t.Log("closing status pipe") - if err := h.Close(); err != nil { - t.Errorf("Close() error = %v", - err) - } - t.Log("waiting on helper") - if err := h.Wait(); err != nil { + if err := h.Wait(); !errors.Is(err, context.Canceled) { t.Errorf("Wait() err = %v stderr = %s", err, stderr) } @@ -112,51 +94,35 @@ func testHelper(t *testing.T, createHelper func() helper.Helper) { t.Run("wait already finalised helper", func(t *testing.T) { wantErr := "exec: Wait was already called" if err := h.Wait(); err != nil && err.Error() != wantErr { - t.Errorf("Wait(%v) error = %v, wantErr %v", - ready, + t.Errorf("Wait: error = %v, wantErr %v", err, wantErr) return } }) if got := stdout.String(); !strings.HasPrefix(got, wantPayload) { - t.Errorf("StartNotify(%v) stdout = %v, want %v", - ready, + t.Errorf("Start: stdout = %v, want %v", got, wantPayload) } }) t.Run("start helper and wait", func(t *testing.T) { h := createHelper() - cmd := h.Unwrap() stdout, stderr := new(strings.Builder), new(strings.Builder) - cmd.Stdout, cmd.Stderr = stdout, stderr + h.Stdout(stdout).Stderr(stderr) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() - if err := h.Start(); err != nil { + if err := h.Start(ctx, false); err != nil { t.Errorf("Start() error = %v", err) return } - t.Run("close helper without status pipe", func(t *testing.T) { - defer func() { - wantPanic := "attempted to close helper with no status pipe" - if r := recover(); r != wantPanic { - t.Errorf("Close() panic = %v, wantPanic %v", - r, wantPanic) - } - }() - if err := h.Close(); err != nil { - t.Errorf("Close() error = %v", - err) - return - } - }) - if err := h.Wait(); err != nil { - t.Errorf("Wait() err = %v stderr = %s", - err, stderr) + t.Errorf("Wait() err = %v stdout = %s stderr = %s", + err, stdout, stderr) } if got := stdout.String(); !strings.HasPrefix(got, wantPayload) { diff --git a/helper/pipe.go b/helper/pipe.go deleted file mode 100644 index f17b74f..0000000 --- a/helper/pipe.go +++ /dev/null @@ -1,149 +0,0 @@ -package helper - -import ( - "errors" - "io" - "os" - "os/exec" - - "git.gensokyo.uk/security/fortify/helper/proc" -) - -type pipes struct { - args io.WriterTo - - statP [2]*os.File - argsP [2]*os.File - - ready chan error - - cmd *exec.Cmd -} - -func (p *pipes) pipe() error { - if p.statP[0] != nil || p.statP[1] != nil || - p.argsP[0] != nil || p.argsP[1] != nil { - panic("attempted to pipe twice") - } - if p.args == nil { - panic("attempted to pipe without args") - } - - // create pipes - if pr, pw, err := os.Pipe(); err != nil { - return err - } else { - p.argsP[0], p.argsP[1] = pr, pw - } - - // create status pipes if ready signal is requested - if p.ready != nil { - if pr, pw, err := os.Pipe(); err != nil { - return err - } else { - p.statP[0], p.statP[1] = pr, pw - } - } - - return nil -} - -// calls pipe to create pipes and sets them up as ExtraFiles, returning their fd -func (p *pipes) prepareCmd(cmd *exec.Cmd) (argsFd, statFd int, err error) { - argsFd, statFd = -1, -1 - if err = p.pipe(); err != nil { - return - } - - // save a reference of cmd for future use - p.cmd = cmd - - argsFd = int(proc.ExtraFile(cmd, p.argsP[0])) - if p.ready != nil { - statFd = int(proc.ExtraFile(cmd, p.statP[1])) - } - - return -} - -func (p *pipes) readyWriteArgs() error { - statsP, argsP := p.statP[0], p.argsP[1] - - // write arguments and close args pipe - if _, err := p.args.WriteTo(argsP); err != nil { - if err1 := p.cmd.Process.Kill(); err1 != nil { - // should be unreachable - panic(err1.Error()) - } - return err - } else { - if err = argsP.Close(); err != nil { - if err1 := p.cmd.Process.Kill(); err1 != nil { - // should be unreachable - panic(err1.Error()) - } - return err - } - } - - if p.ready != nil { - // monitor stat pipe - go func() { - n, err := statsP.Read(make([]byte, 1)) - switch n { - case -1: - if err1 := p.cmd.Process.Kill(); err1 != nil { - // should be unreachable - panic(err1.Error()) - } - // ensure error is not nil - if err == nil { - err = ErrStatusFault - } - p.ready <- err - case 0: - // ensure error is not nil - if err == nil { - err = ErrStatusRead - } - p.ready <- err - case 1: - p.ready <- nil - default: - panic("unreachable") // unexpected read count - } - }() - } - - return nil -} - -func (p *pipes) mustClosePipes() { - if err := p.argsP[0].Close(); err != nil && !errors.Is(err, os.ErrClosed) { - // unreachable - panic(err.Error()) - } - if err := p.argsP[1].Close(); err != nil && !errors.Is(err, os.ErrClosed) { - // unreachable - panic(err.Error()) - } - - if p.ready != nil { - if err := p.statP[0].Close(); err != nil && !errors.Is(err, os.ErrClosed) { - // unreachable - panic(err.Error()) - } - if err := p.statP[1].Close(); err != nil && !errors.Is(err, os.ErrClosed) { - // unreachable - panic(err.Error()) - } - } -} - -func (p *pipes) closeStatus() error { - if p.ready == nil { - panic("attempted to close helper with no status pipe") - } - - return p.statP[0].Close() -} diff --git a/helper/pipe_test.go b/helper/pipe_test.go deleted file mode 100644 index e9c8d06..0000000 --- a/helper/pipe_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package helper - -import ( - "testing" -) - -func Test_pipes_pipe_mustClosePipes(t *testing.T) { - p := new(pipes) - - t.Run("pipe without args", func(t *testing.T) { - defer func() { - wantPanic := "attempted to pipe without args" - if r := recover(); r != wantPanic { - t.Errorf("pipe() panic = %v, wantPanic %v", - r, wantPanic) - } - }() - _ = p.pipe() - }) - - p.args = MustNewCheckedArgs(make([]string, 0)) - t.Run("obtain pipes", func(t *testing.T) { - if err := p.pipe(); err != nil { - t.Errorf("pipe() error = %v", - err) - return - } - }) - - t.Run("pipe twice", func(t *testing.T) { - defer func() { - wantPanic := "attempted to pipe twice" - if r := recover(); r != wantPanic { - t.Errorf("pipe() panic = %v, wantPanic %v", - r, wantPanic) - } - }() - _ = p.pipe() - }) - - p.mustClosePipes() -} diff --git a/helper/seccomp/api.go b/helper/seccomp/api.go index 5b4a42c..2799fc9 100644 --- a/helper/seccomp/api.go +++ b/helper/seccomp/api.go @@ -1,22 +1,15 @@ package seccomp import ( + "context" "errors" - "io" - "os" "syscall" + + "git.gensokyo.uk/security/fortify/helper/proc" ) -func Export(opts SyscallOpts) (f *os.File, err error) { - if f, err = tmpfile(); err != nil { - return - } - if err = exportFilter(f.Fd(), opts); err != nil { - return - } - _, err = f.Seek(0, io.SeekStart) - return -} +// New returns an inactive Encoder instance. +func New(opts SyscallOpts) *Encoder { return &Encoder{newExporter(opts)} } /* An Encoder writes a BPF program to an output stream. @@ -45,7 +38,31 @@ func (e *Encoder) Close() error { return errors.Join(e.closeWrite(), <-e.exportErr) } -// New returns an inactive Encoder instance. -func New(opts SyscallOpts) *Encoder { - return &Encoder{newExporter(opts)} +// NewFile returns an instance of exporter implementing [proc.File]. +func NewFile(opts SyscallOpts) proc.File { return &File{opts: opts} } + +// File implements [proc.File] and provides access to the read end of exporter pipe. +type File struct { + opts SyscallOpts + proc.BaseFile +} + +func (f *File) ErrCount() int { return 2 } +func (f *File) Fulfill(ctx context.Context, dispatchErr func(error)) error { + e := newExporter(f.opts) + if err := e.prepare(); err != nil { + return err + } + f.Set(e.r) + go func() { + select { + case err := <-e.exportErr: + dispatchErr(nil) + dispatchErr(err) + case <-ctx.Done(): + dispatchErr(e.closeWrite()) + dispatchErr(<-e.exportErr) + } + }() + return nil } diff --git a/helper/seccomp/seccomp-export.c b/helper/seccomp/seccomp-export.c index 855c3bf..5a3a56b 100644 --- a/helper/seccomp/seccomp-export.c +++ b/helper/seccomp/seccomp-export.c @@ -48,14 +48,6 @@ struct f_syscall_act { } \ } while (0) - -int f_tmpfile_fd() { - FILE *f = tmpfile(); - if (f == NULL) - return -1; - return fileno(f); -} - int32_t f_export_bpf(int fd, uint32_t arch, uint32_t multiarch, f_syscall_opts opts) { int32_t res = 0; // refer to resErr for meaning int allow_multiarch = opts & F_MULTIARCH; diff --git a/helper/seccomp/seccomp-export.h b/helper/seccomp/seccomp-export.h index 3a28b12..df158d9 100644 --- a/helper/seccomp/seccomp-export.h +++ b/helper/seccomp/seccomp-export.h @@ -20,5 +20,4 @@ typedef enum { } f_syscall_opts; extern void F_println(char *v); -int f_tmpfile_fd(); int32_t f_export_bpf(int fd, uint32_t arch, uint32_t multiarch, f_syscall_opts opts); \ No newline at end of file diff --git a/helper/seccomp/seccomp.go b/helper/seccomp/seccomp.go index affe369..a8cf8bb 100644 --- a/helper/seccomp/seccomp.go +++ b/helper/seccomp/seccomp.go @@ -9,7 +9,6 @@ import "C" import ( "errors" "fmt" - "os" "runtime" ) @@ -47,14 +46,6 @@ const ( FlagBluetooth SyscallOpts = C.F_BLUETOOTH ) -func tmpfile() (*os.File, error) { - fd, err := C.f_tmpfile_fd() - if err != nil { - return nil, err - } - return os.NewFile(uintptr(fd), "tmpfile"), err -} - func exportFilter(fd uintptr, opts SyscallOpts) error { var ( arch C.uint32_t = 0 diff --git a/helper/stub.go b/helper/stub.go index 8ffd3a0..830505c 100644 --- a/helper/stub.go +++ b/helper/stub.go @@ -1,7 +1,9 @@ package helper import ( + "context" "flag" + "fmt" "io" "os" "os/exec" @@ -11,6 +13,7 @@ import ( "testing" "git.gensokyo.uk/security/fortify/helper/bwrap" + "git.gensokyo.uk/security/fortify/helper/proc" "git.gensokyo.uk/security/fortify/internal/fmsg" ) @@ -18,20 +21,23 @@ import ( // it is part of the implementation of the helper stub. func InternalChildStub() { // this test mocks the helper process - if os.Getenv(FortifyHelper) != "1" || - os.Getenv(FortifyStatus) == "-1" { // this indicates the stub is being invoked as a bwrap child without pipes + var ap, sp string + if v, ok := os.LookupEnv(FortifyHelper); !ok { return + } else { + ap = v + } + if v, ok := os.LookupEnv(FortifyStatus); !ok { + panic(FortifyStatus) + } else { + sp = v } - - argsFD := flag.Int("args", -1, "") - statFD := flag.Int("fd", -1, "") - _ = flag.CommandLine.Parse(os.Args[4:]) switch os.Args[3] { case "bwrap": - bwrapStub(argsFD, statFD) + bwrapStub() default: - genericStub(argsFD, statFD) + genericStub(flagRestoreFiles(4, ap, sp)) } fmsg.Exit(0) @@ -40,57 +46,65 @@ func InternalChildStub() { // InternalReplaceExecCommand is an internal function but exported because it is cross-package; // it is part of the implementation of the helper stub. func InternalReplaceExecCommand(t *testing.T) { - t.Cleanup(func() { - execCommand = exec.Command - }) + t.Cleanup(func() { commandContext = exec.CommandContext }) // replace execCommand to have the resulting *exec.Cmd launch TestHelperChildStub - execCommand = func(name string, arg ...string) *exec.Cmd { + commandContext = func(ctx context.Context, name string, arg ...string) *exec.Cmd { // pass through nonexistent path if name == "/nonexistent" && len(arg) == 0 { - return exec.Command(name) + return exec.CommandContext(ctx, name) } - return exec.Command(os.Args[0], append([]string{"-test.run=TestHelperChildStub", "--", name}, arg...)...) + return exec.CommandContext(ctx, os.Args[0], append([]string{"-test.run=TestHelperChildStub", "--", name}, arg...)...) } } -func genericStub(argsFD, statFD *int) { - // simulate args pipe behaviour - func() { - if *argsFD == -1 { - panic("attempted to start helper without passing args pipe fd") - } +func newFile(fd int, name, p string) *os.File { + present := false + switch p { + case "0": + case "1": + present = true + default: + panic(fmt.Sprintf("%s fd has unexpected presence value %q", name, p)) + } - f := os.NewFile(uintptr(*argsFD), "|0") - if f == nil { - panic("attempted to start helper without args pipe") - } + f := os.NewFile(uintptr(fd), name) + if !present && f != nil { + panic(fmt.Sprintf("%s fd set but not present", name)) + } + if present && f == nil { + panic(fmt.Sprintf("%s fd preset but unset", name)) + } - if _, err := io.Copy(os.Stdout, f); err != nil { + return f +} + +func flagRestoreFiles(offset int, ap, sp string) (argsFile, statFile *os.File) { + argsFd := flag.Int("args", -1, "") + statFd := flag.Int("fd", -1, "") + _ = flag.CommandLine.Parse(os.Args[offset:]) + argsFile = newFile(*argsFd, "args", ap) + statFile = newFile(*statFd, "stat", sp) + return +} + +func genericStub(argsFile, statFile *os.File) { + if argsFile != nil { + // this output is checked by parent + if _, err := io.Copy(os.Stdout, argsFile); err != nil { panic("cannot read args: " + err.Error()) } - }() - - var wait chan struct{} + } // simulate status pipe behaviour - if os.Getenv(FortifyStatus) == "1" { - if *statFD == -1 { - panic("attempted to start helper with status reporting without passing status pipe fd") + if statFile != nil { + if _, err := statFile.Write([]byte{'x'}); err != nil { + panic("cannot write to status pipe: " + err.Error()) } - wait = make(chan struct{}) + done := make(chan struct{}) go func() { - f := os.NewFile(uintptr(*statFD), "|1") - if f == nil { - panic("attempted to start with status reporting without status pipe") - } - - if _, err := f.Write([]byte{'x'}); err != nil { - panic("cannot write to status pipe: " + err.Error()) - } - // wait for status pipe close var epoll int if fd, err := syscall.EpollCreate1(0); err != nil { @@ -103,7 +117,7 @@ func genericStub(argsFD, statFD *int) { }() epoll = fd } - if err := syscall.EpollCtl(epoll, syscall.EPOLL_CTL_ADD, int(f.Fd()), &syscall.EpollEvent{}); err != nil { + if err := syscall.EpollCtl(epoll, syscall.EPOLL_CTL_ADD, int(statFile.Fd()), &syscall.EpollEvent{}); err != nil { panic("cannot add status pipe to epoll: " + err.Error()) } events := make([]syscall.EpollEvent, 1) @@ -114,50 +128,36 @@ func genericStub(argsFD, statFD *int) { panic(strconv.Itoa(int(events[0].Events))) } - close(wait) + close(done) }() - } - - if wait != nil { - <-wait + <-done } } -func bwrapStub(argsFD, statFD *int) { - // the bwrap launcher does not ever launch with sync fd - if *statFD != -1 { - panic("attempted to launch bwrap with status monitoring") - } +func bwrapStub() { + // the bwrap launcher does not launch with a typical sync fd + argsFile, _ := flagRestoreFiles(4, "1", "0") // test args pipe behaviour func() { - if *argsFD == -1 { - panic("attempted to start bwrap without passing args pipe fd") - } - - f := os.NewFile(uintptr(*argsFD), "|0") - if f == nil { - panic("attempted to start helper without args pipe") - } - got, want := new(strings.Builder), new(strings.Builder) - - if _, err := io.Copy(got, f); err != nil { - panic("cannot read args: " + err.Error()) + if _, err := io.Copy(got, argsFile); err != nil { + panic("cannot read bwrap args: " + err.Error()) } // hardcoded bwrap configuration used by test - if _, err := MustNewCheckedArgs((&bwrap.Config{ - Unshare: nil, + sc := &bwrap.Config{ Net: true, - UserNS: false, Hostname: "localhost", Chdir: "/nonexistent", Clearenv: true, NewSession: true, DieWithParent: true, AsInit: true, - }).Args()).WriteTo(want); err != nil { + } + args := sc.Args() + sc.FDArgs(nil, &args, new(proc.ExtraFilesPre), new([]proc.File)) + if _, err := MustNewCheckedArgs(args).WriteTo(want); err != nil { panic("cannot read want: " + err.Error()) } diff --git a/internal/app/start.go b/internal/app/start.go index aba331e..837ab22 100644 --- a/internal/app/start.go +++ b/internal/app/start.go @@ -46,7 +46,7 @@ func (a *app) Run(ctx context.Context, rs *RunState) error { } // startup will go ahead, commit system setup - if err := a.seal.sys.Commit(); err != nil { + if err := a.seal.sys.Commit(ctx); err != nil { return err } a.seal.sys.needRevert = true diff --git a/internal/priv/shim/main.go b/internal/priv/shim/main.go index 4dccb87..4397cc3 100644 --- a/internal/priv/shim/main.go +++ b/internal/priv/shim/main.go @@ -1,10 +1,14 @@ package shim import ( + "context" "errors" "os" + "os/exec" + "os/signal" "path" "strconv" + "syscall" "git.gensokyo.uk/security/fortify/fst" "git.gensokyo.uk/security/fortify/helper" @@ -138,19 +142,22 @@ func Main() { ); err != nil { fmsg.Fatalf("malformed sandbox config: %v", err) } else { - cmd := b.Unwrap() - cmd.Stdin, cmd.Stdout, cmd.Stderr = os.Stdin, os.Stdout, os.Stderr + b.Stdin(os.Stdin).Stdout(os.Stdout).Stderr(os.Stderr) + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() // unreachable // run and pass through exit code - if err = b.Start(); err != nil { + if err = b.Start(ctx, false); err != nil { fmsg.Fatalf("cannot start target process: %v", err) } else if err = b.Wait(); err != nil { - fmsg.VPrintln("wait:", err) - } - if b.Unwrap().ProcessState != nil { - fmsg.Exit(b.Unwrap().ProcessState.ExitCode()) - } else { - fmsg.Exit(127) + var exitError *exec.ExitError + if !errors.As(err, &exitError) { + fmsg.Println("wait:", err) + fmsg.Exit(127) + panic("unreachable") + } + fmsg.Exit(exitError.ExitCode()) + panic("unreachable") } } } diff --git a/internal/system/dbus.go b/internal/system/dbus.go index 4220826..fd995bb 100644 --- a/internal/system/dbus.go +++ b/internal/system/dbus.go @@ -3,7 +3,6 @@ package system import ( "bytes" "errors" - "os" "strings" "sync" @@ -26,9 +25,6 @@ func (sys *I) MustProxyDBus(sessionPath string, session *dbus.Config, systemPath func (sys *I) ProxyDBus(session, system *dbus.Config, sessionPath, systemPath string) (func(), error) { d := new(DBus) - // used by waiting goroutine to notify process exit - d.done = make(chan struct{}) - // session bus is mandatory if session == nil { return nil, fmsg.WrapError(ErrDBusConfig, @@ -75,88 +71,34 @@ type DBus struct { out *scanToFmsg // whether system bus proxy is enabled system bool - // notification from goroutine waiting for dbus.Proxy - done chan struct{} } func (d *DBus) Type() Enablement { return Process } -func (d *DBus) apply(_ *I) error { +func (d *DBus) apply(sys *I) error { fmsg.VPrintf("session bus proxy on %q for upstream %q", d.proxy.Session()[1], d.proxy.Session()[0]) if d.system { fmsg.VPrintf("system bus proxy on %q for upstream %q", d.proxy.System()[1], d.proxy.System()[0]) } - // ready channel passed to dbus package - ready := make(chan error, 1) - - // background dbus proxy start - if err := d.proxy.Start(ready, d.out, true, true); err != nil { + // this starts the process and blocks until ready + if err := d.proxy.Start(sys.ctx, d.out, true); err != nil { + d.out.Dump() return fmsg.WrapErrorSuffix(err, "cannot start message bus proxy:") } fmsg.VPrintln("starting message bus proxy:", d.proxy) - if fmsg.Verbose() { // save the extra bwrap arg build when verbose logging is off - fmsg.VPrintln("message bus proxy bwrap args:", d.proxy.BwrapStatic()) - } - - // background wait for proxy instance and notify completion - go func() { - if err := d.proxy.Wait(); err != nil { - fmsg.Println("message bus proxy exited with error:", err) - go func() { ready <- err }() - } else { - fmsg.VPrintln("message bus proxy exit") - } - - // ensure socket removal so ephemeral directory is empty at revert - if err := os.Remove(d.proxy.Session()[1]); err != nil && !errors.Is(err, os.ErrNotExist) { - fmsg.Println("cannot remove dangling session bus socket:", err) - } - if d.system { - if err := os.Remove(d.proxy.System()[1]); err != nil && !errors.Is(err, os.ErrNotExist) { - fmsg.Println("cannot remove dangling system bus socket:", err) - } - } - - // notify proxy completion - close(d.done) - }() - - // ready is not nil if the proxy process faulted - if err := <-ready; err != nil { - // dump message buffer as caller does not dump this - // in an early fault condition - d.out.Dump() - - // note that err here is either an I/O error or a predetermined unexpected behaviour error - return fmsg.WrapErrorSuffix(err, - "message bus proxy fault after start:") - } - fmsg.VPrintln("message bus proxy ready") - return nil } func (d *DBus) revert(_ *I, _ *Criteria) error { // criteria ignored here since dbus is always process-scoped fmsg.VPrintln("terminating message bus proxy") - - if err := d.proxy.Close(); err != nil { - if errors.Is(err, os.ErrClosed) { - return fmsg.WrapError(err, - "message bus proxy already closed") - } else { - return fmsg.WrapErrorSuffix(err, - "cannot stop message bus proxy:") - } - } - - // block until proxy wait returns - <-d.done - return nil + d.proxy.Close() + defer fmsg.VPrintln("message bus proxy exit") + return fmsg.WrapErrorSuffix(d.proxy.Wait(), "message bus proxy error:") } func (d *DBus) Is(o Op) bool { diff --git a/internal/system/op.go b/internal/system/op.go index d8479c7..8a98fa7 100644 --- a/internal/system/op.go +++ b/internal/system/op.go @@ -1,6 +1,7 @@ package system import ( + "context" "errors" "os" "sync" @@ -57,10 +58,14 @@ func TypeString(e Enablement) string { type I struct { uid int ops []Op - sp *os.File + ctx context.Context + // sync fd passed to bwrap + sp *os.File - state [2]bool - lock sync.Mutex + // whether sys has been reverted + state bool + + lock sync.Mutex } func (sys *I) UID() int { @@ -85,14 +90,14 @@ func (sys *I) Equal(v *I) bool { return true } -func (sys *I) Commit() error { +func (sys *I) Commit(ctx context.Context) error { sys.lock.Lock() defer sys.lock.Unlock() - if sys.state[0] { + if sys.ctx != nil { panic("sys instance committed twice") } - sys.state[0] = true + sys.ctx = ctx sp := New(sys.uid) sp.ops = make([]Op, 0, len(sys.ops)) // prevent copies during commits @@ -125,10 +130,10 @@ func (sys *I) Revert(ec *Criteria) error { sys.lock.Lock() defer sys.lock.Unlock() - if sys.state[1] { + if sys.state { panic("sys instance reverted twice") } - sys.state[1] = true + sys.state = true // collect errors errs := make([]error, len(sys.ops)) diff --git a/ldd/exec.go b/ldd/exec.go index 47e20a8..d605b45 100644 --- a/ldd/exec.go +++ b/ldd/exec.go @@ -1,20 +1,19 @@ package ldd import ( - "fmt" + "context" "os" - "os/exec" "strings" + "time" "git.gensokyo.uk/security/fortify/helper" "git.gensokyo.uk/security/fortify/helper/bwrap" ) -func Exec(p string) ([]*Entry, error) { - var ( - h helper.Helper - cmd *exec.Cmd - ) +const lddTimeout = 2 * time.Second + +func Exec(ctx context.Context, p string) ([]*Entry, error) { + var h helper.Helper if b, err := helper.NewBwrap( (&bwrap.Config{ @@ -29,17 +28,20 @@ func Exec(p string) ([]*Entry, error) { ); err != nil { return nil, err } else { - cmd = b.Unwrap() h = b } - cmd.Stdout, cmd.Stderr = new(strings.Builder), os.Stderr - if err := h.Start(); err != nil { + stdout := new(strings.Builder) + h.Stdout(stdout).Stderr(os.Stderr) + + c, cancel := context.WithTimeout(ctx, lddTimeout) + defer cancel() + if err := h.Start(c, false); err != nil { return nil, err } if err := h.Wait(); err != nil { return nil, err } - return Parse(cmd.Stdout.(fmt.Stringer)) + return Parse(stdout) } diff --git a/main.go b/main.go index 1c6ab08..1ef11e1 100644 --- a/main.go +++ b/main.go @@ -307,22 +307,14 @@ func main() { func runApp(config *fst.Config) { rs := new(app.RunState) - ctx, cancel := context.WithCancel(context.Background()) + ctx, stop := signal.NotifyContext(context.Background(), + syscall.SIGINT, syscall.SIGTERM) + defer stop() // unreachable if fmsg.Verbose() { seccomp.CPrintln = fmsg.Println } - // handle signals for graceful shutdown - sig := make(chan os.Signal, 2) - signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM) - go func() { - v := <-sig - fmsg.Printf("got %s after program start", v) - cancel() - signal.Ignore(syscall.SIGINT, syscall.SIGTERM) - }() - if a, err := app.New(sys); err != nil { fmsg.Fatalf("cannot create app: %s\n", err) } else if err = a.Seal(config); err != nil {