From bc01ef235043c87a832f8cee7fe2831eeb6d9812 Mon Sep 17 00:00:00 2001 From: mudler-agent Date: Mon, 28 Sep 2026 09:28:04 +0200 Subject: [PATCH] fix(gallery): keep model deletion inside the models directory (#12324) listModelFiles gave utils.VerifyPath paths that it had already joined onto the models directory. VerifyPath joins its argument onto the base again, so an absolute path always passes and none of the four checks could fail. Model deletion then removed files outside the models directory: - A model name such as "../outside/victim" removed outside/victim.yaml. The in-process MCP delete_model tool passes the name from the tool call without a check. - A gallery file that lists a files: entry with "../" removed that file. listModelFiles now gives VerifyPath the relative names. InTrustedRoot also looped forever when a relative path was outside a relative root. filepath.Dir stops at "." for a relative path, and the loop waited for "/". The loop now stops when Dir returns its input. Assisted-by: Claude:claude-opus-5-5 [Claude Code] Signed-off-by: Ettore Di Giacinto Co-authored-by: Ettore Di Giacinto --- core/gallery/delete_model_paths_test.go | 72 +++++++++++++++++++++++++ core/gallery/models.go | 11 ++-- pkg/utils/path.go | 11 ++-- pkg/utils/path_test.go | 16 ++++++ 4 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 core/gallery/delete_model_paths_test.go 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() {