Skip to content

Commit 923adf0

Browse files
amoebazeroshade
andauthored
fix(uninstall): fix issues with uninstalling manifest-only drivers (#5)
This primarily fixes a bug with dbc uninstall for manifest-only drivers where the extra folder we install alongside the driver manifest wasn't being removed. I also factored out the common logic related to removing the driver shared library and added some defensive checks to limit the impact of a maliciously-crafted driver manifest. Ref #15 --------- Co-authored-by: Matt Topol <zotthewizard@gmail.com>
1 parent f9fecb7 commit 923adf0

File tree

8 files changed

+107
-67
lines changed

8 files changed

+107
-67
lines changed

.github/workflows/release.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ jobs:
1616
snapshot:
1717
runs-on: ubuntu-latest
1818
environment: snapshot
19-
if: github.event_name == 'pull_request'
19+
if: github.event_name == 'pull_request' && !github.event.pull_request.head.repo.fork
2020
steps:
2121
- uses: actions/checkout@v5
2222
with:

cmd/dbc/install_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ func (suite *SubcommandTestSuite) TestInstallManifestOnlyDriver() {
120120
m := InstallCmd{Driver: "test-driver-manifest-only"}.
121121
GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg})
122122
suite.validateOutput("\r[✓] searching\r\n[✓] downloading\r\n[✓] installing\r\n[✓] verifying signature\r\n"+
123-
"\r\nInstalled test-driver-manifest-only 0.1.0 to "+suite.tempdir+"\r\n"+
123+
"\r\nInstalled test-driver-manifest-only 1.0.0 to "+suite.tempdir+"\r\n"+
124124
"\r\nMust have libtest_driver installed to load this driver\r\n", suite.runCmd(m))
125125
if runtime.GOOS != "windows" {
126126
suite.FileExists(filepath.Join(suite.tempdir, "test-driver-manifest-only.toml"))

cmd/dbc/search_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func (suite *SubcommandTestSuite) TestSearchCmdVerbose() {
4141
"Available Versions:\r\n ├── 2.0.0\r\n ╰── 2.1.0\r\n"+
4242
"• test-driver-manifest-only\r\n Title: Test Driver Manifest Only\r\n "+
4343
"Description: This is manifest-only driver\r\n License: Apache-2.0\r\n "+
44-
"Available Versions:\r\n ╰── 0.1.0\r\n\r ", suite.runCmd(m))
44+
"Available Versions:\r\n ╰── 1.0.0\r\n\r ", suite.runCmd(m))
4545
}
4646

4747
func (suite *SubcommandTestSuite) TestSearchCmdVerboseWithInstalled() {
@@ -64,5 +64,5 @@ func (suite *SubcommandTestSuite) TestSearchCmdVerboseWithInstalled() {
6464
" Description: This is manifest-only driver\r\n"+
6565
" License: Apache-2.0\r\n"+
6666
" Available Versions:\r\n"+
67-
" ╰── 0.1.0\r\n\r ", suite.runCmd(m))
67+
" ╰── 1.0.0\r\n\r ", suite.runCmd(m))
6868
}

cmd/dbc/testdata/test_manifest.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ drivers:
5454
license: Apache-2.0
5555
path: test-driver-manifest-only
5656
pkginfo:
57-
- version: v0.1.0
57+
- version: v1.0.0
5858
packages:
5959
- platform: linux_amd64
6060
- platform: macos_amd64

cmd/dbc/uninstall_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package main
44

55
import (
6+
"fmt"
67
"os"
78
"path"
89
"path/filepath"
@@ -124,3 +125,31 @@ func (suite *SubcommandTestSuite) TestUninstallMultipleLocationsNonDefault() {
124125
suite.FileExists(filepath.Join(suite.tempdir, "test-driver-1.toml"))
125126
suite.NoFileExists(filepath.Join(installModel.cfg.Location, "test-driver-1.toml"))
126127
}
128+
129+
func (suite *SubcommandTestSuite) TestUninstallManifestOnlyDriver() {
130+
m := InstallCmd{Driver: "test-driver-manifest-only"}.
131+
GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg})
132+
suite.validateOutput("\r[✓] searching\r\n[✓] downloading\r\n[✓] installing\r\n[✓] verifying signature\r\n"+
133+
"\r\nInstalled test-driver-manifest-only 1.0.0 to "+suite.tempdir+"\r\n"+
134+
"\r\nMust have libtest_driver installed to load this driver\r\n", suite.runCmd(m))
135+
if runtime.GOOS != "windows" {
136+
suite.FileExists(filepath.Join(suite.tempdir, "test-driver-manifest-only.toml"))
137+
}
138+
139+
// Verify the sidecar folder exists before we uninstall
140+
new_sidecar_path := fmt.Sprintf("test-driver-manifest-only_%s_v1.0.0", config.PlatformTuple())
141+
err := os.Rename(filepath.Join(suite.tempdir, "test-driver-manifest-only"), filepath.Join(suite.tempdir, new_sidecar_path))
142+
if err != nil {
143+
suite.Fail(fmt.Sprintf("Failed to rename sidecar folder. Something is wrong with this test: %v", err))
144+
}
145+
suite.DirExists(filepath.Join(suite.tempdir, new_sidecar_path))
146+
147+
// Now uninstall and verify we clean up
148+
m = UninstallCmd{Driver: "test-driver-manifest-only"}.
149+
GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg})
150+
suite.validateOutput("Driver `test-driver-manifest-only` uninstalled successfully!\r\n\r\n\r ", suite.runCmd(m))
151+
if runtime.GOOS != "windows" {
152+
suite.NoFileExists(filepath.Join(suite.tempdir, "test-driver-manifest-only.toml"))
153+
}
154+
suite.NoDirExists(filepath.Join(suite.tempdir, new_sidecar_path))
155+
}

config/config.go

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"io/fs"
1212
"maps"
1313
"os"
14-
"path"
1514
"path/filepath"
1615
"runtime"
1716
"slices"
@@ -201,7 +200,7 @@ func InstallDriver(cfg Config, shortName string, downloaded *os.File) (Manifest,
201200
if loc, err = EnsureLocation(cfg); err != nil {
202201
return Manifest{}, fmt.Errorf("could not ensure config location: %w", err)
203202
}
204-
base := strings.TrimSuffix(path.Base(downloaded.Name()), ".tar.gz")
203+
base := strings.TrimSuffix(filepath.Base(downloaded.Name()), ".tar.gz")
205204
finalDir := filepath.Join(loc, base)
206205

207206
if err := os.MkdirAll(finalDir, 0o755); err != nil {
@@ -315,3 +314,60 @@ func decodeManifest(r io.Reader, driverName string, requireShared bool) (Manifes
315314

316315
return result, nil
317316
}
317+
318+
// Common, non-platform-specific code for uninstalling a driver. Called by
319+
// platform-specific UninstallDriver function.
320+
func UninstallDriverShared(cfg Config, info DriverInfo) error {
321+
for sharedPath := range info.Driver.Shared.Paths() {
322+
// Run filepath.Clean on sharedPath mainly to catch inner ".." in the path
323+
sharedPath = filepath.Clean(sharedPath)
324+
325+
// Don't remove anything that isn't contained withing the found driver's
326+
// config directory (i.e., avoid malicious driver manifests)
327+
if !strings.HasPrefix(sharedPath, cfg.Location) {
328+
continue
329+
}
330+
331+
// dbc installs drivers in a folder, other tools may not so we handle each
332+
// differently.
333+
if info.Source == "dbc" {
334+
if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil {
335+
// Ignore only when not found. This supports manifest-only drivers.
336+
// TODO: Come up with a better mechanism to handle manifest-only drivers
337+
// and remove this continue when we do
338+
if errors.Is(err, fs.ErrNotExist) {
339+
continue
340+
}
341+
return fmt.Errorf("error removing driver %s: %w", info.ID, err)
342+
}
343+
} else {
344+
if err := os.Remove(sharedPath); err != nil {
345+
// Ignore only when not found. This supports manifest-only drivers.
346+
// TODO: Come up with a better mechanism to handle manifest-only drivers
347+
// and remove this continue when we do
348+
if errors.Is(err, fs.ErrNotExist) {
349+
continue
350+
}
351+
return fmt.Errorf("error removing driver %s: %w", info.ID, err)
352+
}
353+
}
354+
}
355+
356+
// Manifest only drivers can come with extra files such as a LICENSE and we
357+
// create a folder next to the driver manifest to store them, same as we'd
358+
// store the actual driver shared library. Above, we find the path of this
359+
// folder by looking at the Driver.shared path. For manifest-only drivers,
360+
// Driver.shared is not a valid path (it's just a name), so this trick doesn't
361+
// work. We do want to clean this folder up so here we guess what it is and
362+
// try to remove it e.g., "somedriver_macos_arm64_v1.2.3."
363+
extra_folder := fmt.Sprintf("%s_%s_v%s", info.ID, platformTuple, info.Version)
364+
extra_folder = filepath.Clean(extra_folder)
365+
extra_path := filepath.Join(cfg.Location, extra_folder)
366+
finfo, err := os.Stat(extra_path)
367+
if err == nil && finfo.IsDir() && extra_path != "." {
368+
_ = os.RemoveAll(extra_path)
369+
// ignore errors
370+
}
371+
372+
return nil
373+
}

config/dirs_unixlike.go

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@
55
package config
66

77
import (
8-
"errors"
98
"fmt"
10-
"io/fs"
119
"maps"
1210
"os"
1311
"path/filepath"
@@ -90,36 +88,14 @@ func CreateManifest(cfg Config, driver DriverInfo) (err error) {
9088
}
9189

9290
func UninstallDriver(cfg Config, info DriverInfo) error {
93-
if info.Source == "dbc" {
94-
for sharedPath := range info.Driver.Shared.Paths() {
95-
if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil {
96-
// Ignore only when not found. This supports manifest-only drivers.
97-
// TODO: Come up with a better mechanism to handle manifest-only drivers
98-
// and remove this continue when we do
99-
if errors.Is(err, fs.ErrNotExist) {
100-
continue
101-
}
102-
return fmt.Errorf("error removing driver %s: %w", info.ID, err)
103-
}
104-
}
105-
} else {
106-
for sharedPath := range info.Driver.Shared.Paths() {
107-
if err := os.Remove(sharedPath); err != nil {
108-
// Ignore only when not found. This supports manifest-only drivers.
109-
// TODO: Come up with a better mechanism to handle manifest-only drivers
110-
// and remove this continue when we do
111-
if errors.Is(err, fs.ErrNotExist) {
112-
continue
113-
}
114-
return fmt.Errorf("error removing driver %s: %w", info.ID, err)
115-
}
116-
}
117-
}
118-
11991
manifest := filepath.Join(cfg.Location, info.ID+".toml")
12092
if err := os.Remove(manifest); err != nil {
12193
return fmt.Errorf("error removing manifest %s: %w", manifest, err)
12294
}
12395

96+
if err := UninstallDriverShared(cfg, info); err != nil {
97+
return fmt.Errorf("failed to delete driver shared object: %w", err)
98+
}
99+
124100
return nil
125101
}

config/dirs_windows.go

Lines changed: 11 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ package config
55
import (
66
"errors"
77
"fmt"
8-
"io/fs"
98
"log"
109
"maps"
1110
"os"
@@ -275,40 +274,20 @@ func CreateManifest(cfg Config, driver DriverInfo) (err error) {
275274
}
276275

277276
func UninstallDriver(cfg Config, info DriverInfo) error {
278-
k, err := registry.OpenKey(cfg.Level.key(), regKeyADBC, registry.WRITE)
279-
if err != nil {
280-
return err
281-
}
282-
defer k.Close()
277+
if cfg.Level != ConfigEnv {
278+
k, err := registry.OpenKey(cfg.Level.key(), regKeyADBC, registry.WRITE)
279+
if err != nil {
280+
return err
281+
}
282+
defer k.Close()
283283

284-
if err := registry.DeleteKey(k, info.ID); err != nil {
285-
return fmt.Errorf("failed to delete driver registry key: %w", err)
284+
if err := registry.DeleteKey(k, info.ID); err != nil {
285+
return fmt.Errorf("failed to delete driver registry key: %w", err)
286+
}
286287
}
287288

288-
if info.Source == "dbc" {
289-
for sharedPath := range info.Driver.Shared.Paths() {
290-
if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil {
291-
// Ignore only when not found. This supports manifest-only drivers.
292-
// TODO: Come up with a better mechanism to handle manifest-only drivers
293-
// and remove this continue when we do
294-
if errors.Is(err, fs.ErrNotExist) {
295-
continue
296-
}
297-
return fmt.Errorf("error removing driver %s: %w", info.ID, err)
298-
}
299-
}
300-
} else {
301-
for sharedPath := range info.Driver.Shared.Paths() {
302-
if err := os.Remove(sharedPath); err != nil {
303-
// Ignore only when not found. This supports manifest-only drivers.
304-
// TODO: Come up with a better mechanism to handle manifest-only drivers
305-
// and remove this continue when we do
306-
if errors.Is(err, fs.ErrNotExist) {
307-
continue
308-
}
309-
return fmt.Errorf("error removing driver %s: %w", info.ID, err)
310-
}
311-
}
289+
if err := UninstallDriverShared(cfg, info); err != nil {
290+
return fmt.Errorf("failed to delete driver shared object: %w", err)
312291
}
313292

314293
return nil

0 commit comments

Comments
 (0)