From 124743ffd3da7c31ef9c562593e15d88bb19008e Mon Sep 17 00:00:00 2001 From: Ophestra Date: Wed, 15 Jan 2025 23:39:51 +0900 Subject: [PATCH] app: expose single run method App is no longer just a simple [exec.Cmd] wrapper, so exposing these steps separately no longer makes sense and actually hinders proper error handling, cleanup and cancellation. This change removes the five-second wait when the shim dies before receiving the payload, and provides caller the ability to gracefully stop execution of the confined process. Signed-off-by: Ophestra --- cmd/fshim/ipc/shim/shim.go | 59 +++++----- internal/app/app.go | 24 ++-- internal/app/start.go | 226 +++++++++++++++++++------------------ internal/proc/fd.go | 3 + main.go | 44 +++++--- 5 files changed, 195 insertions(+), 161 deletions(-) diff --git a/cmd/fshim/ipc/shim/shim.go b/cmd/fshim/ipc/shim/shim.go index c08422a..4f60531 100644 --- a/cmd/fshim/ipc/shim/shim.go +++ b/cmd/fshim/ipc/shim/shim.go @@ -1,14 +1,13 @@ package shim import ( + "context" "encoding/gob" "errors" "os" "os/exec" - "os/signal" "strconv" "strings" - "syscall" "time" shim0 "git.gensokyo.uk/security/fortify/cmd/fshim/ipc" @@ -17,8 +16,6 @@ import ( "git.gensokyo.uk/security/fortify/internal/proc" ) -const shimSetupTimeout = 5 * time.Second - // used by the parent process type Shim struct { @@ -34,6 +31,8 @@ type Shim struct { killFallback chan error // shim setup payload payload *shim0.Payload + // monitor to shim encoder + encoder *gob.Encoder } func New(uid uint32, aid string, supp []string, payload *shim0.Payload) *Shim { @@ -56,7 +55,7 @@ func (s *Shim) WaitFallback() chan error { } func (s *Shim) Start() (*time.Time, error) { - // start user switcher process and save time + // prepare user switcher invocation var fsu string if p, ok := internal.Check(internal.Fsu); !ok { fmsg.Fatal("invalid fsu path, this copy of fshim is not compiled correctly") @@ -66,18 +65,19 @@ func (s *Shim) Start() (*time.Time, error) { } s.cmd = exec.Command(fsu) - var encoder *gob.Encoder + // pass shim setup pipe if fd, e, err := proc.Setup(&s.cmd.ExtraFiles); err != nil { return nil, fmsg.WrapErrorSuffix(err, "cannot create shim setup pipe:") } else { - encoder = e + s.encoder = e s.cmd.Env = []string{ shim0.Env + "=" + strconv.Itoa(fd), "FORTIFY_APP_ID=" + s.aid, } } + // format fsu supplementary groups if len(s.supp) > 0 { fmsg.VPrintf("attaching supplementary group ids %s", s.supp) s.cmd.Env = append(s.cmd.Env, "FORTIFY_GROUPS="+strings.Join(s.supp, " ")) @@ -92,13 +92,17 @@ func (s *Shim) Start() (*time.Time, error) { } fmsg.VPrintln("starting shim via fsu:", s.cmd) - fmsg.Suspend() // withhold messages to stderr + // withhold messages to stderr + fmsg.Suspend() if err := s.cmd.Start(); err != nil { return nil, fmsg.WrapErrorSuffix(err, "cannot start fsu:") } startTime := time.Now().UTC() + return &startTime, nil +} +func (s *Shim) Serve(ctx context.Context) error { // kill shim if something goes wrong and an error is returned s.killFallback = make(chan error, 1) killShim := func() { @@ -108,30 +112,31 @@ func (s *Shim) Start() (*time.Time, error) { } defer func() { killShim() }() - // take alternative exit path on signal - 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) - s.killFallback <- nil - signal.Ignore(syscall.SIGINT, syscall.SIGTERM) - }() - - shimErr := make(chan error) - go func() { shimErr <- encoder.Encode(s.payload) }() + encodeErr := make(chan error) + go func() { encodeErr <- s.encoder.Encode(s.payload) }() select { - case err := <-shimErr: + // encode return indicates setup completion + case err := <-encodeErr: if err != nil { - return &startTime, fmsg.WrapErrorSuffix(err, + return fmsg.WrapErrorSuffix(err, "cannot transmit shim config:") } killShim = func() {} - case <-time.After(shimSetupTimeout): - return &startTime, fmsg.WrapError(errors.New("timed out waiting for shim"), - "timed out waiting for shim") - } + return nil - return &startTime, nil + // setup canceled before payload was accepted + case <-ctx.Done(): + err := ctx.Err() + if errors.Is(err, context.Canceled) { + return fmsg.WrapError(errors.New("shim setup canceled"), + "shim setup canceled") + } + if errors.Is(err, context.DeadlineExceeded) { + return fmsg.WrapError(errors.New("deadline exceeded waiting for shim"), + "deadline exceeded waiting for shim") + } + // unreachable + return err + } } diff --git a/internal/app/app.go b/internal/app/app.go index 0adc121..175c315 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -1,6 +1,7 @@ package app import ( + "context" "sync" "sync/atomic" @@ -12,17 +13,22 @@ import ( type App interface { // ID returns a copy of App's unique ID. ID() fst.ID - // Start sets up the system and starts the App. - Start() error - // Wait waits for App's process to exit and reverts system setup. - Wait() (int, error) - // WaitErr returns error returned by the underlying wait syscall. - WaitErr() error + // Run sets up the system and runs the App. + Run(ctx context.Context, rs *RunState) error Seal(config *fst.Config) error String() string } +type RunState struct { + // Start is true if fsu is successfully started. + Start bool + // ExitCode is the value returned by fshim. + ExitCode int + // WaitErr is error returned by the underlying wait syscall. + WaitErr error +} + type app struct { // single-use config reference ct *appCt @@ -35,8 +41,6 @@ type app struct { shim *shim.Shim // child process related information seal *appSeal - // error returned waiting for process - waitErr error lock sync.RWMutex } @@ -64,10 +68,6 @@ func (a *app) String() string { return "(unsealed fortified app)" } -func (a *app) WaitErr() error { - return a.waitErr -} - func New(os linux.System) (App, error) { a := new(app) a.id = new(fst.ID) diff --git a/internal/app/start.go b/internal/app/start.go index cc13181..6fffda5 100644 --- a/internal/app/start.go +++ b/internal/app/start.go @@ -1,11 +1,13 @@ package app import ( + "context" "errors" "fmt" "os/exec" "path/filepath" "strings" + "time" shim0 "git.gensokyo.uk/security/fortify/cmd/fshim/ipc" "git.gensokyo.uk/security/fortify/cmd/fshim/ipc/shim" @@ -15,12 +17,16 @@ import ( "git.gensokyo.uk/security/fortify/internal/system" ) -// Start selects a user switcher and starts shim. -// Note that Wait must be called regardless of error returned by Start. -func (a *app) Start() error { +const shimSetupTimeout = 5 * time.Second + +func (a *app) Run(ctx context.Context, rs *RunState) error { a.lock.Lock() defer a.lock.Unlock() + if rs == nil { + panic("attempted to pass nil state to run") + } + // resolve exec paths shimExec := [2]string{helper.BubblewrapName} if len(a.seal.command) > 0 { @@ -64,10 +70,30 @@ func (a *app) Start() error { // export sync pipe from sys a.seal.sys.bwrap.SetSync(a.seal.sys.Sync()) + // start shim via manager + waitErr := make(chan error, 1) if startTime, err := a.shim.Start(); err != nil { return err } else { - // shim start and setup success, create process state + // shim process created + rs.Start = true + + shimSetupCtx, shimSetupCancel := context.WithDeadline(ctx, time.Now().Add(shimSetupTimeout)) + defer shimSetupCancel() + + // start waiting for shim + go func() { + waitErr <- a.shim.Unwrap().Wait() + // cancel shim setup in case shim died before receiving payload + shimSetupCancel() + }() + + // send payload + if err = a.shim.Serve(shimSetupCtx); err != nil { + return err + } + + // shim accepted setup payload, create process state sd := state.State{ ID: *a.id, PID: a.shim.Unwrap().Process.Pid, @@ -81,111 +107,42 @@ func (a *app) Start() error { err0.InnerErr = c.Save(&sd) }) a.seal.sys.saveState = true - return err0.equiv("cannot save process state:") - } -} - -// StateStoreError is returned for a failed state save -type StateStoreError struct { - // whether inner function was called - Inner bool - // error returned by state.Store Do method - DoErr error - // error returned by state.Backend Save method - InnerErr error - // any other errors needing to be tracked - Err error -} - -func (e *StateStoreError) equiv(a ...any) error { - if e.Inner && e.DoErr == nil && e.InnerErr == nil && e.Err == nil { - return nil - } else { - return fmsg.WrapErrorSuffix(e, a...) - } -} - -func (e *StateStoreError) Error() string { - if e.Inner && e.InnerErr != nil { - return e.InnerErr.Error() - } - - if e.DoErr != nil { - return e.DoErr.Error() - } - - if e.Err != nil { - return e.Err.Error() - } - - return "(nil)" -} - -func (e *StateStoreError) Unwrap() (errs []error) { - errs = make([]error, 0, 3) - if e.DoErr != nil { - errs = append(errs, e.DoErr) - } - if e.InnerErr != nil { - errs = append(errs, e.InnerErr) - } - if e.Err != nil { - errs = append(errs, e.Err) - } - return -} - -type RevertCompoundError interface { - Error() string - Unwrap() []error -} - -func (a *app) Wait() (int, error) { - a.lock.Lock() - defer a.lock.Unlock() - - if a.shim == nil { - fmsg.VPrintln("shim not initialised, skipping cleanup") - return 1, nil - } - - var r int - - if cmd := a.shim.Unwrap(); cmd == nil { - // failure prior to process start - r = 255 - } else { - wait := make(chan error, 1) - go func() { wait <- cmd.Wait() }() - - select { - // wait for process and resolve exit code - case err := <-wait: - if err != nil { - var exitError *exec.ExitError - if !errors.As(err, &exitError) { - // should be unreachable - a.waitErr = err - } - - // store non-zero return code - r = exitError.ExitCode() - } else { - r = cmd.ProcessState.ExitCode() - } - fmsg.VPrintf("process %d exited with exit code %d", cmd.Process.Pid, r) - - // alternative exit path when kill was unsuccessful - case err := <-a.shim.WaitFallback(): - r = 255 - if err != nil { - fmsg.Printf("cannot terminate shim on faulted setup: %v", err) - } else { - fmsg.VPrintln("alternative exit path selected") - } + if err = err0.equiv("cannot save process state:"); err != nil { + return err } } + select { + // wait for process and resolve exit code + case err := <-waitErr: + if err != nil { + var exitError *exec.ExitError + if !errors.As(err, &exitError) { + // should be unreachable + rs.WaitErr = err + } + + // store non-zero return code + rs.ExitCode = exitError.ExitCode() + } else { + rs.ExitCode = a.shim.Unwrap().ProcessState.ExitCode() + } + if fmsg.Verbose() { + fmsg.VPrintf("process %d exited with exit code %d", a.shim.Unwrap().Process.Pid, rs.ExitCode) + } + + // this is reached when a fault makes an already running shim impossible to continue execution + // however a kill signal could not be delivered (should actually always happen like that since fsu) + // the effects of this is similar to the alternative exit path and ensures shim death + case err := <-a.shim.WaitFallback(): + rs.ExitCode = 255 + fmsg.Printf("cannot terminate shim on faulted setup: %v", err) + + // alternative exit path relying on shim behaviour on monitor process exit + case <-ctx.Done(): + fmsg.VPrintln("alternative exit path selected") + } + // child process exited, resume output fmsg.Resume() @@ -262,5 +219,60 @@ func (a *app) Wait() (int, error) { }) e.Err = a.seal.store.Close() - return r, e.equiv("error returned during cleanup:", e) + return e.equiv("error returned during cleanup:", e) +} + +// StateStoreError is returned for a failed state save +type StateStoreError struct { + // whether inner function was called + Inner bool + // error returned by state.Store Do method + DoErr error + // error returned by state.Backend Save method + InnerErr error + // any other errors needing to be tracked + Err error +} + +func (e *StateStoreError) equiv(a ...any) error { + if e.Inner && e.DoErr == nil && e.InnerErr == nil && e.Err == nil { + return nil + } else { + return fmsg.WrapErrorSuffix(e, a...) + } +} + +func (e *StateStoreError) Error() string { + if e.Inner && e.InnerErr != nil { + return e.InnerErr.Error() + } + + if e.DoErr != nil { + return e.DoErr.Error() + } + + if e.Err != nil { + return e.Err.Error() + } + + return "(nil)" +} + +func (e *StateStoreError) Unwrap() (errs []error) { + errs = make([]error, 0, 3) + if e.DoErr != nil { + errs = append(errs, e.DoErr) + } + if e.InnerErr != nil { + errs = append(errs, e.InnerErr) + } + if e.Err != nil { + errs = append(errs, e.Err) + } + return +} + +type RevertCompoundError interface { + Error() string + Unwrap() []error } diff --git a/internal/proc/fd.go b/internal/proc/fd.go index 5400abe..e99abb3 100644 --- a/internal/proc/fd.go +++ b/internal/proc/fd.go @@ -12,6 +12,7 @@ var ( ErrInvalid = errors.New("bad file descriptor") ) +// Setup appends the read end of a pipe for payload transmission and returns its fd. func Setup(extraFiles *[]*os.File) (int, *gob.Encoder, error) { if r, w, err := os.Pipe(); err != nil { return -1, nil, err @@ -22,6 +23,8 @@ func Setup(extraFiles *[]*os.File) (int, *gob.Encoder, error) { } } +// Receive retrieves payload pipe fd from the environment, +// receives its payload and returns the Close method of the pipe. func Receive(key string, e any) (func() error, error) { var setup *os.File diff --git a/main.go b/main.go index e4f2860..197dd1e 100644 --- a/main.go +++ b/main.go @@ -1,14 +1,17 @@ package main import ( + "context" _ "embed" "flag" "fmt" "os" + "os/signal" "os/user" "strconv" "strings" "sync" + "syscall" "text/tabwriter" "git.gensokyo.uk/security/fortify/dbus" @@ -288,27 +291,38 @@ func main() { } func runApp(config *fst.Config) { - a, err := app.New(sys) - if err != nil { + rs := new(app.RunState) + ctx, cancel := context.WithCancel(context.Background()) + + // 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 { logBaseError(err, "cannot seal app:") fmsg.Exit(1) - } else if err = a.Start(); err != nil { - logBaseError(err, "cannot start app:") - } - - var r int - // wait must be called regardless of result of start - if r, err = a.Wait(); err != nil { - if r < 1 { - r = 1 + } else if err = a.Run(ctx, rs); err != nil { + if !rs.Start { + logBaseError(err, "cannot start app:") + } else { + logWaitError(err) } - logWaitError(err) } - if err = a.WaitErr(); err != nil { - fmsg.Println("inner wait failed:", err) + if rs.WaitErr != nil { + fmsg.Println("inner wait failed:", rs.WaitErr) } - fmsg.Exit(r) + if rs.ExitCode < 0 { + fmsg.VPrintf("got negative exit %v", rs.ExitCode) + fmsg.Exit(1) + } + fmsg.Exit(rs.ExitCode) panic("unreachable") }