diff --git a/core/gallery/delete_model_paths_test.go b/core/gallery/delete_model_paths_test.go new file mode 100644 index 000000000..a61cfe636 --- /dev/null +++ b/core/gallery/delete_model_paths_test.go @@ -0,0 +1,72 @@ +package gallery_test + +import ( + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/mudler/LocalAI/core/gallery" + "github.com/mudler/LocalAI/pkg/system" +) + +// DeleteModelFromSystem removes files named by a model name and by the +// model's gallery file. Neither may reach outside the models directory: the +// name can come from an API caller or from an assistant tool call, and the +// gallery file is a YAML file on disk. +var _ = Describe("DeleteModelFromSystem path containment", func() { + var ( + root string + modelsPath string + outside string + state *system.SystemState + ) + + BeforeEach(func() { + root = GinkgoT().TempDir() + modelsPath = filepath.Join(root, "models") + outside = filepath.Join(root, "outside") + Expect(os.MkdirAll(modelsPath, 0o755)).To(Succeed()) + Expect(os.MkdirAll(outside, 0o755)).To(Succeed()) + var err error + state, err = system.GetSystemState(system.WithModelPath(modelsPath)) + Expect(err).ToNot(HaveOccurred()) + }) + + It("refuses a model name that escapes the models directory", func() { + victim := filepath.Join(outside, "victim.yaml") + Expect(os.WriteFile(victim, []byte("name: victim\n"), 0o644)).To(Succeed()) + + Expect(gallery.DeleteModelFromSystem(state, "../outside/victim")).ToNot(Succeed()) + Expect(victim).To(BeARegularFile()) + }) + + It("does not remove gallery-declared files outside the models directory", func() { + secret := filepath.Join(outside, "secret.bin") + Expect(os.WriteFile(secret, []byte("x"), 0o644)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(modelsPath, "m.yaml"), []byte("name: m\n"), 0o644)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(modelsPath, gallery.GalleryFileName("m")), []byte(` +files: + - filename: ../outside/secret.bin +`), 0o644)).To(Succeed()) + + _ = gallery.DeleteModelFromSystem(state, "m") + Expect(secret).To(BeARegularFile()) + }) + + It("still deletes a normal model and its declared files", func() { + weights := filepath.Join(modelsPath, "m", "w.gguf") + Expect(os.MkdirAll(filepath.Dir(weights), 0o755)).To(Succeed()) + Expect(os.WriteFile(weights, []byte("w"), 0o644)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(modelsPath, "m.yaml"), []byte("name: m\nparameters:\n model: m/w.gguf\n"), 0o644)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(modelsPath, gallery.GalleryFileName("m")), []byte(` +files: + - filename: m/w.gguf +`), 0o644)).To(Succeed()) + + Expect(gallery.DeleteModelFromSystem(state, "m")).To(Succeed()) + Expect(weights).ToNot(BeAnExistingFile()) + Expect(filepath.Join(modelsPath, "m.yaml")).ToNot(BeAnExistingFile()) + }) +}) diff --git a/core/gallery/models.go b/core/gallery/models.go index c787dcde4..6d4ceb346 100644 --- a/core/gallery/models.go +++ b/core/gallery/models.go @@ -808,8 +808,11 @@ func GetLocalModelConfiguration(basePath string, name string) (*ModelConfig, err func listModelFiles(systemState *system.SystemState, name string) ([]string, error) { + // VerifyPath joins its argument onto the models path itself, so every + // check below passes the relative name: an already-joined absolute path + // always lands inside the base and the check would pass anything. configFile := filepath.Join(systemState.Model.ModelsPath, fmt.Sprintf("%s.yaml", name)) - if err := utils.VerifyPath(configFile, systemState.Model.ModelsPath); err != nil { + if err := utils.VerifyPath(fmt.Sprintf("%s.yaml", name), systemState.Model.ModelsPath); err != nil { return nil, fmt.Errorf("failed to verify path %s: %w", configFile, err) } @@ -817,7 +820,7 @@ func listModelFiles(systemState *system.SystemState, name string) ([]string, err name = strings.ReplaceAll(name, string(os.PathSeparator), "__") galleryFile := filepath.Join(systemState.Model.ModelsPath, galleryFileName(name)) - if err := utils.VerifyPath(galleryFile, systemState.Model.ModelsPath); err != nil { + if err := utils.VerifyPath(galleryFileName(name), systemState.Model.ModelsPath); err != nil { return nil, fmt.Errorf("failed to verify path %s: %w", galleryFile, err) } @@ -847,7 +850,7 @@ func listModelFiles(systemState *system.SystemState, name string) ([]string, err if err == nil && galleryconfig != nil { for _, f := range galleryconfig.Files { fullPath := filepath.Join(systemState.Model.ModelsPath, f.Filename) - if err := utils.VerifyPath(fullPath, systemState.Model.ModelsPath); err != nil { + if err := utils.VerifyPath(f.Filename, systemState.Model.ModelsPath); err != nil { return allFiles, fmt.Errorf("failed to verify path %s: %w", fullPath, err) } allFiles = append(allFiles, fullPath) @@ -858,7 +861,7 @@ func listModelFiles(systemState *system.SystemState, name string) ([]string, err for _, f := range additionalFiles { fullPath := filepath.Join(filepath.Join(systemState.Model.ModelsPath, f)) - if err := utils.VerifyPath(fullPath, systemState.Model.ModelsPath); err != nil { + if err := utils.VerifyPath(f, systemState.Model.ModelsPath); err != nil { return allFiles, fmt.Errorf("failed to verify path %s: %w", fullPath, err) } allFiles = append(allFiles, fullPath) diff --git a/pkg/utils/path.go b/pkg/utils/path.go index 1ae11d123..df3af9072 100644 --- a/pkg/utils/path.go +++ b/pkg/utils/path.go @@ -13,13 +13,18 @@ func ExistsInPath(path string, s string) bool { } func InTrustedRoot(path string, trustedRoot string) error { - for path != "/" { - path = filepath.Dir(path) + for { + parent := filepath.Dir(path) + // Dir stops changing at "/" for an absolute path and at "." for a + // relative one; waiting for "/" alone spins forever on the latter. + if parent == path { + return fmt.Errorf("path is outside of trusted root") + } + path = parent if path == trustedRoot { return nil } } - return fmt.Errorf("path is outside of trusted root") } // VerifyPath verifies that path is based in basePath. diff --git a/pkg/utils/path_test.go b/pkg/utils/path_test.go index 79c415cd4..1c9d18b2c 100644 --- a/pkg/utils/path_test.go +++ b/pkg/utils/path_test.go @@ -3,6 +3,7 @@ package utils_test import ( "os" "path/filepath" + "time" . "github.com/mudler/LocalAI/pkg/utils" . "github.com/onsi/ginkgo/v2" @@ -93,6 +94,21 @@ var _ = Describe("utils/path tests", func() { It("rejects an unrelated absolute path", func() { Expect(InTrustedRoot("/etc/passwd", "/srv/models")).ToNot(Succeed()) }) + + It("rejects a relative path outside a relative root instead of looping", func() { + // Walking up a relative path ends at ".", never at "/", so the + // walk must stop when it stops making progress. + done := make(chan error, 1) + go func() { done <- InTrustedRoot("x", "models") }() + Eventually(done).WithTimeout(2 * time.Second).Should(Receive(HaveOccurred())) + + go func() { done <- VerifyPath("../x", "models") }() + Eventually(done).WithTimeout(2 * time.Second).Should(Receive(HaveOccurred())) + }) + + It("accepts a relative descendant of a relative root", func() { + Expect(InTrustedRoot("models/a/file", "models")).To(Succeed()) + }) }) Describe("SanitizeFileName", func() {