From 6e7ddb2d2ec6b163dc2c178bc4d73b5943f51206 Mon Sep 17 00:00:00 2001 From: Ophestra Date: Sat, 15 Mar 2025 23:57:44 +0900 Subject: helper: eliminate commandContext replacement This is done more cleanly by modifying Args in cmdF. Signed-off-by: Ophestra --- dbus/dbus_test.go | 66 +++++++++++------ dbus/proc.go | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++ dbus/proxy.go | 2 + dbus/run.go | 184 ---------------------------------------------- dbus/samples_test.go | 37 ++++++---- dbus/stub_test.go | 4 +- 6 files changed, 268 insertions(+), 226 deletions(-) create mode 100644 dbus/proc.go delete mode 100644 dbus/run.go (limited to 'dbus') diff --git a/dbus/dbus_test.go b/dbus/dbus_test.go index 57a441a4..0b264dcf 100644 --- a/dbus/dbus_test.go +++ b/dbus/dbus_test.go @@ -3,6 +3,9 @@ package dbus_test import ( "context" "errors" + "fmt" + "os" + "os/exec" "strings" "testing" "time" @@ -100,12 +103,13 @@ func TestProxy_Seal(t *testing.T) { } func TestProxy_Start_Wait_Close_String(t *testing.T) { - t.Run("sandboxed", func(t *testing.T) { + t.Run("sandbox", func(t *testing.T) { + proxyName := dbus.ProxyName + dbus.ProxyName = os.Args[0] + t.Cleanup(func() { dbus.ProxyName = proxyName }) testProxyStartWaitCloseString(t, true) }) - t.Run("direct", func(t *testing.T) { - testProxyStartWaitCloseString(t, false) - }) + t.Run("direct", func(t *testing.T) { testProxyStartWaitCloseString(t, false) }) } func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { @@ -125,14 +129,30 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { }) t.Run("proxy for "+id, func(t *testing.T) { - helper.InternalReplaceExecCommand(t) - overridePath(t) - p := dbus.New(tc[0].bus, tc[1].bus) + p.CmdF = func(cmd *exec.Cmd) { + wantArgv0 := dbus.ProxyName + if sandbox { + wantArgv0 = "bwrap" + } + if cmd.Args[0] != wantArgv0 { + panic(fmt.Sprintf("unexpected argv0 %q", os.Args[0])) + } + cmd.Err = nil + cmd.Path = os.Args[0] + + if sandbox { + cmd.Args = append([]string{os.Args[0], "-test.run=TestHelperStub", "--"}, + append(cmd.Args[:5], append([]string{"-test.run=TestHelperStub", "--"}, cmd.Args[5:]...)...)...) + cmd.Env = append(cmd.Env, "GO_TEST_FORTIFY_BWRAP_STUB_TYPE=dbus") + } else { + cmd.Args = append([]string{os.Args[0], "-test.run=TestHelperStub", "--"}, cmd.Args[1:]...) + } + } output := new(strings.Builder) - t.Run("unsealed behaviour of "+id, func(t *testing.T) { - t.Run("unsealed string of "+id, func(t *testing.T) { + t.Run("unsealed", func(t *testing.T) { + t.Run("string", func(t *testing.T) { want := "(unsealed dbus proxy)" if got := p.String(); got != want { t.Errorf("String() = %v, want %v", @@ -141,7 +161,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { } }) - t.Run("unsealed start of "+id, func(t *testing.T) { + t.Run("start", func(t *testing.T) { want := "proxy not sealed" if err := p.Start(context.Background(), nil, sandbox); err == nil || err.Error() != want { t.Errorf("Start() error = %v, wantErr %q", @@ -150,7 +170,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { } }) - t.Run("unsealed wait of "+id, func(t *testing.T) { + t.Run("wait", func(t *testing.T) { wantErr := "dbus: not started" if err := p.Wait(); err == nil || err.Error() != wantErr { t.Errorf("Wait() error = %v, wantErr %v", @@ -168,7 +188,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { } }) - t.Run("sealed behaviour of "+id, func(t *testing.T) { + t.Run("sealed", func(t *testing.T) { want := strings.Join(append(tc[0].want, tc[1].want...), " ") if got := p.String(); got != want { t.Errorf("String() = %v, want %v", @@ -176,7 +196,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { return } - t.Run("sealed start of "+id, func(t *testing.T) { + t.Run("start", func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -185,8 +205,14 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { err) } - t.Run("started string of "+id, func(t *testing.T) { - wantSubstr := dbus.ProxyName + " --args=" + t.Run("string", func(t *testing.T) { + wantSubstr := fmt.Sprintf("%s -test.run=TestHelperStub -- --args=3 --fd=4", os.Args[0]) + if sandbox { + wantSubstr = fmt.Sprintf( + "%s -test.run=TestHelperStub -- bwrap --args 6 -- %s -test.run=TestHelperStub -- --args=3 --fd=4", + os.Args[0], os.Args[0], + ) + } if got := p.String(); !strings.Contains(got, wantSubstr) { t.Errorf("String() = %v, want %v", p.String(), wantSubstr) @@ -194,7 +220,7 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { } }) - t.Run("started wait of "+id, func(t *testing.T) { + t.Run("wait", func(t *testing.T) { p.Close() if err := p.Wait(); err != nil { t.Errorf("Wait() error = %v\noutput: %s", @@ -206,11 +232,3 @@ func testProxyStartWaitCloseString(t *testing.T, sandbox bool) { }) } } - -func overridePath(t *testing.T) { - proxyName := dbus.ProxyName - dbus.ProxyName = "/nonexistent-xdg-dbus-proxy" - t.Cleanup(func() { - dbus.ProxyName = proxyName - }) -} diff --git a/dbus/proc.go b/dbus/proc.go new file mode 100644 index 00000000..379f9fd3 --- /dev/null +++ b/dbus/proc.go @@ -0,0 +1,201 @@ +package dbus + +import ( + "context" + "errors" + "io" + "os" + "os/exec" + "path" + "path/filepath" + "slices" + "strconv" + "strings" + "syscall" + + "git.gensokyo.uk/security/fortify/helper" + "git.gensokyo.uk/security/fortify/helper/bwrap" + "git.gensokyo.uk/security/fortify/ldd" +) + +// 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() + + if p.seal == nil { + return errors.New("proxy not sealed") + } + + var h helper.Helper + + c, cancel := context.WithCancelCause(ctx) + if !sandbox { + h = helper.NewDirect(c, p.name, p.seal, true, argF, func(cmd *exec.Cmd) { + cmdF(cmd, output, p.CmdF) + + // xdg-dbus-proxy does not need to inherit the environment + cmd.Env = make([]string, 0) + }, nil) + } else { + // look up absolute path if name is just a file name + toolPath := p.name + if filepath.Base(p.name) == p.name { + if s, err := exec.LookPath(p.name); err != nil { + return err + } else { + toolPath = s + } + } + + // resolve libraries by parsing ldd output + var proxyDeps []*ldd.Entry + if toolPath != os.Args[0] { + if l, err := ldd.Exec(ctx, toolPath); err != nil { + return err + } else { + proxyDeps = l + } + } + + bc := &bwrap.Config{ + Hostname: "fortify-dbus", + Chdir: "/", + Syscall: &bwrap.SyscallPolicy{DenyDevel: true, Multiarch: true}, + Clearenv: true, + NewSession: true, + DieWithParent: true, + } + + // resolve proxy socket directories + bindTargetM := make(map[string]struct{}, 2) + + for _, ps := range []string{p.session[1], p.system[1]} { + if pd := path.Dir(ps); len(pd) > 0 { + if pd[0] == '/' { + bindTargetM[pd] = struct{}{} + } + } + } + + bindTarget := make([]string, 0, len(bindTargetM)) + for k := range bindTargetM { + bindTarget = append(bindTarget, k) + } + slices.Sort(bindTarget) + for _, name := range bindTarget { + bc.Bind(name, name, false, true) + } + + roBindTargetM := make(map[string]struct{}, 2+1+len(proxyDeps)) + + // xdb-dbus-proxy bin and dependencies + roBindTargetM[path.Dir(toolPath)] = struct{}{} + for _, ent := range proxyDeps { + if path.IsAbs(ent.Path) { + roBindTargetM[path.Dir(ent.Path)] = struct{}{} + } + if path.IsAbs(ent.Name) { + roBindTargetM[path.Dir(ent.Name)] = struct{}{} + } + } + + // resolve upstream bus directories + for _, as := range []string{p.session[0], p.system[0]} { + if len(as) > 0 && strings.HasPrefix(as, "unix:path=/") { + // leave / intact + roBindTargetM[path.Dir(as[10:])] = struct{}{} + } + } + + roBindTarget := make([]string, 0, len(roBindTargetM)) + for k := range roBindTargetM { + roBindTarget = append(roBindTarget, k) + } + slices.Sort(roBindTarget) + for _, name := range roBindTarget { + bc.Bind(name, name) + } + + h = helper.MustNewBwrap(c, toolPath, + p.seal, true, + argF, func(cmd *exec.Cmd) { cmdF(cmd, output, p.CmdF) }, + nil, + bc, nil, + ) + p.bwrap = bc + } + + if err := h.Start(); err != nil { + cancel(err) + return err + } + + p.helper = h + p.ctx = c + p.cancel = cancel + return nil +} + +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("dbus: not started") + } + + 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 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 +} + +func argF(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 cmdF(cmd *exec.Cmd, output io.Writer, cmdF func(cmd *exec.Cmd)) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if output != nil { + cmd.Stdout, cmd.Stderr = output, output + } + if cmdF != nil { + cmdF(cmd) + } +} diff --git a/dbus/proxy.go b/dbus/proxy.go index 5e92e341..e7785fd8 100644 --- a/dbus/proxy.go +++ b/dbus/proxy.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "os/exec" "sync" "git.gensokyo.uk/security/fortify/helper" @@ -26,6 +27,7 @@ type Proxy struct { name string session [2]string system [2]string + CmdF func(cmd *exec.Cmd) sysP bool seal io.WriterTo diff --git a/dbus/run.go b/dbus/run.go deleted file mode 100644 index 30b06f4e..00000000 --- a/dbus/run.go +++ /dev/null @@ -1,184 +0,0 @@ -package dbus - -import ( - "context" - "errors" - "io" - "os" - "os/exec" - "path" - "path/filepath" - "strconv" - "strings" - "syscall" - - "git.gensokyo.uk/security/fortify/helper" - "git.gensokyo.uk/security/fortify/helper/bwrap" - "git.gensokyo.uk/security/fortify/ldd" -) - -// 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() - - if p.seal == nil { - return errors.New("proxy not sealed") - } - - var ( - h helper.Helper - - argF = func(argsFd, statFd int) []string { - if statFd == -1 { - return []string{"--args=" + strconv.Itoa(argsFd)} - } else { - return []string{"--args=" + strconv.Itoa(argsFd), "--fd=" + strconv.Itoa(statFd)} - } - } - ) - - c, cancel := context.WithCancelCause(ctx) - if !sandbox { - h = helper.NewDirect(c, p.name, p.seal, true, argF, func(cmd *exec.Cmd) { - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - if output != nil { - cmd.Stdout, cmd.Stderr = output, output - } - - // xdg-dbus-proxy does not need to inherit the environment - cmd.Env = make([]string, 0) - }, nil) - } else { - // look up absolute path if name is just a file name - toolPath := p.name - if filepath.Base(p.name) == p.name { - if s, err := exec.LookPath(p.name); err != nil { - return err - } else { - toolPath = s - } - } - - // resolve libraries by parsing ldd output - var proxyDeps []*ldd.Entry - if toolPath != "/nonexistent-xdg-dbus-proxy" { - if l, err := ldd.Exec(ctx, toolPath); err != nil { - return err - } else { - proxyDeps = l - } - } - - bc := &bwrap.Config{ - Unshare: nil, - Hostname: "fortify-dbus", - Chdir: "/", - Syscall: &bwrap.SyscallPolicy{DenyDevel: true, Multiarch: true}, - Clearenv: true, - NewSession: true, - DieWithParent: true, - } - - // resolve proxy socket directories - bindTarget := make(map[string]struct{}, 2) - for _, ps := range []string{p.session[1], p.system[1]} { - if pd := path.Dir(ps); len(pd) > 0 { - if pd[0] == '/' { - bindTarget[pd] = struct{}{} - } - } - } - for k := range bindTarget { - bc.Bind(k, k, false, true) - } - - roBindTarget := make(map[string]struct{}, 2+1+len(proxyDeps)) - - // xdb-dbus-proxy bin and dependencies - roBindTarget[path.Dir(toolPath)] = struct{}{} - for _, ent := range proxyDeps { - if path.IsAbs(ent.Path) { - roBindTarget[path.Dir(ent.Path)] = struct{}{} - } - if path.IsAbs(ent.Name) { - roBindTarget[path.Dir(ent.Name)] = struct{}{} - } - } - - // resolve upstream bus directories - for _, as := range []string{p.session[0], p.system[0]} { - if len(as) > 0 && strings.HasPrefix(as, "unix:path=/") { - // leave / intact - roBindTarget[path.Dir(as[10:])] = struct{}{} - } - } - - for k := range roBindTarget { - bc.Bind(k, k) - } - - h = helper.MustNewBwrap(c, toolPath, p.seal, true, argF, func(cmd *exec.Cmd) { - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - if output != nil { - cmd.Stdout, cmd.Stderr = output, output - } - }, nil, bc, nil) - p.bwrap = bc - } - - if err := h.Start(); err != nil { - cancel(err) - return err - } - - p.helper = h - p.ctx = c - p.cancel = cancel - return nil -} - -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("dbus: not started") - } - - 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 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/dbus/samples_test.go b/dbus/samples_test.go index 653a02f4..75ed0225 100644 --- a/dbus/samples_test.go +++ b/dbus/samples_test.go @@ -6,6 +6,12 @@ import ( "git.gensokyo.uk/security/fortify/dbus" ) +const ( + sampleHostPath = "/run/user/1971/bus" + sampleHostAddr = "unix:path=" + sampleHostPath + sampleBindPath = "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/bus" +) + var samples = []dbusTestCase{ { "org.chromium.Chromium", &dbus.Config{ @@ -19,10 +25,10 @@ var samples = []dbusTestCase{ Log: false, Filter: true, }, false, false, - [2]string{"unix:path=/run/user/1971/bus", "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/bus"}, + [2]string{sampleHostAddr, sampleBindPath}, []string{ - "unix:path=/run/user/1971/bus", - "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/bus", + sampleHostAddr, + sampleBindPath, "--filter", "--talk=org.freedesktop.Notifications", "--talk=org.freedesktop.FileManager1", @@ -48,9 +54,10 @@ var samples = []dbusTestCase{ Log: false, Filter: true, }, false, false, - [2]string{"unix:path=/run/dbus/system_bus_socket", "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/system_bus_socket"}, - []string{"unix:path=/run/dbus/system_bus_socket", - "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/system_bus_socket", + [2]string{sampleHostAddr, sampleBindPath}, + []string{ + sampleHostAddr, + sampleBindPath, "--filter", "--talk=org.bluez", "--talk=org.freedesktop.Avahi", @@ -68,10 +75,10 @@ var samples = []dbusTestCase{ Log: false, Filter: true, }, false, false, - [2]string{"unix:path=/run/user/1971/bus", "/tmp/fortify.1971/34c24f16a0d791d28835ededaf446033/bus"}, + [2]string{sampleHostAddr, sampleBindPath}, []string{ - "unix:path=/run/user/1971/bus", - "/tmp/fortify.1971/34c24f16a0d791d28835ededaf446033/bus", + sampleHostAddr, + sampleBindPath, "--filter", "--talk=org.freedesktop.Notifications", "--talk=org.kde.StatusNotifierWatcher", @@ -91,10 +98,10 @@ var samples = []dbusTestCase{ Log: true, Filter: true, }, false, false, - [2]string{"unix:path=/run/user/1971/bus", "/tmp/fortify.1971/5da7845287a936efbc2fa75d7d81e501/bus"}, + [2]string{sampleHostAddr, sampleBindPath}, []string{ - "unix:path=/run/user/1971/bus", - "/tmp/fortify.1971/5da7845287a936efbc2fa75d7d81e501/bus", + sampleHostAddr, + sampleBindPath, "--filter", "--see=uk.gensokyo.CrashTestDummy1", "--talk=org.freedesktop.Notifications", @@ -114,10 +121,10 @@ var samples = []dbusTestCase{ Log: true, Filter: true, }, false, true, - [2]string{"unix:path=/run/user/1971/bus", "/tmp/fortify.1971/5da7845287a936efbc2fa75d7d81e501/bus"}, + [2]string{sampleHostAddr, sampleBindPath}, []string{ - "unix:path=/run/user/1971/bus", - "/tmp/fortify.1971/5da7845287a936efbc2fa75d7d81e501/bus", + sampleHostAddr, + sampleBindPath, "--filter", "--see=uk.gensokyo.CrashTestDummy", "--talk=org.freedesktop.Notifications", diff --git a/dbus/stub_test.go b/dbus/stub_test.go index b83deb20..13a7d1df 100644 --- a/dbus/stub_test.go +++ b/dbus/stub_test.go @@ -6,6 +6,4 @@ import ( "git.gensokyo.uk/security/fortify/helper" ) -func TestHelperChildStub(t *testing.T) { - helper.InternalChildStub() -} +func TestHelperStub(t *testing.T) { helper.InternalHelperStub() } -- cgit v1.3.1