From 50c284fdcc77bface613b8a838c36cdd2208f4f5 Mon Sep 17 00:00:00 2001 From: mudler-agent Date: Mon, 28 Sep 2026 10:04:22 +0200 Subject: [PATCH] fix: make the remaining VerifyPath checks effective (#12326) utils.VerifyPath joins its argument onto the base path, so a path that the caller already joined always passes. Several callers gave it joined paths, and their checks could not fail: - modeladmin (config view, patch, edit, pin and state): the config file path from the loader. A config loaded from outside the models directory (--models-config-file) could be pinned, and the pin wrote the outside file. The patch and state paths stopped later, in the mutation snapshot, with a different error. - core/backend/tts.go: the model path joined onto the models path. - The trellis2cpp and stablediffusion-ggml backends: option paths (*_path) joined onto the model path. A "../" value outside the model directory was accepted. Add utils.VerifyResolvedPath for a full path. modeladmin and tts use it. The backends now check the relative option value before they join it. A rename in modeladmin checks the new relative name. For models from a config file outside the models directory, the admin API and web UI now return ErrPathNotTrusted for view, edit, pin, and enable or disable. The docs describe this. Assisted-by: Claude:claude-opus-5-5 [Claude Code] Signed-off-by: Ettore Di Giacinto Co-authored-by: Ettore Di Giacinto --- backend/go/stablediffusion-ggml/gosd.go | 2 +- backend/go/trellis2cpp/trellis2.go | 2 +- backend/go/trellis2cpp/trellis2_test.go | 5 +- core/backend/tts.go | 4 +- core/services/modeladmin/config.go | 8 +-- .../modeladmin/config_path_trust_test.go | 58 +++++++++++++++++++ core/services/modeladmin/pinned.go | 2 +- core/services/modeladmin/state.go | 2 +- docs/content/advanced/model-configuration.md | 2 + pkg/utils/path.go | 11 +++- pkg/utils/path_test.go | 19 ++++++ 11 files changed, 103 insertions(+), 12 deletions(-) create mode 100644 core/services/modeladmin/config_path_trust_test.go diff --git a/backend/go/stablediffusion-ggml/gosd.go b/backend/go/stablediffusion-ggml/gosd.go index e1567bd18..60f2efede 100644 --- a/backend/go/stablediffusion-ggml/gosd.go +++ b/backend/go/stablediffusion-ggml/gosd.go @@ -137,8 +137,8 @@ func (sd *SDGGML) Load(opts *pb.ModelOptions) error { // If it's an option path, we resolve absolute path from the model path if strings.Contains(op, ":") && strings.Contains(op, "path") { data := strings.Split(op, ":") - data[1] = filepath.Join(opts.ModelPath, data[1]) if err := utils.VerifyPath(data[1], opts.ModelPath); err == nil { + data[1] = filepath.Join(opts.ModelPath, data[1]) oo = append(oo, strings.Join(data, ":")) } } else { diff --git a/backend/go/trellis2cpp/trellis2.go b/backend/go/trellis2cpp/trellis2.go index addbf1bb0..c83480aa4 100644 --- a/backend/go/trellis2cpp/trellis2.go +++ b/backend/go/trellis2cpp/trellis2.go @@ -161,10 +161,10 @@ func resolveModels(modelFile, modelPath string, options []string) (modelSet, err continue } if !filepath.IsAbs(value) { - value = filepath.Join(modelPath, value) if err := utils.VerifyPath(value, modelPath); err != nil { return modelSet{}, fmt.Errorf("option %s: %w", key, err) } + value = filepath.Join(modelPath, value) } overrides[key] = value } diff --git a/backend/go/trellis2cpp/trellis2_test.go b/backend/go/trellis2cpp/trellis2_test.go index d492756e4..b221f00c5 100644 --- a/backend/go/trellis2cpp/trellis2_test.go +++ b/backend/go/trellis2cpp/trellis2_test.go @@ -134,9 +134,12 @@ var _ = Describe("resolveModels", func() { It("rejects option paths escaping the model directory", func() { touch(dir, fullSet...) + // The escaping file exists, so only the containment check can + // reject it; a missing file would fail for an unrelated reason. + touch(filepath.Dir(dir), "outside.gguf") _, err := resolveModels("ss_flow_f16.gguf", dir, []string{"dino_path:../outside.gguf"}) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("outside of trusted root"))) }) }) diff --git a/core/backend/tts.go b/core/backend/tts.go index 3451eb471..1172fb8f2 100644 --- a/core/backend/tts.go +++ b/core/backend/tts.go @@ -88,7 +88,7 @@ func ModelTTS( // a FS path mp := filepath.Join(loader.ModelPath, modelConfig.Model) if _, err := os.Stat(mp); err == nil { - if err := utils.VerifyPath(mp, appConfig.SystemState.Model.ModelsPath); err != nil { + if err := utils.VerifyResolvedPath(mp, appConfig.SystemState.Model.ModelsPath); err != nil { return "", nil, err } modelPath = mp @@ -189,7 +189,7 @@ func ModelTTSStream( // a FS path mp := filepath.Join(loader.ModelPath, modelConfig.Model) if _, err := os.Stat(mp); err == nil { - if err := utils.VerifyPath(mp, appConfig.SystemState.Model.ModelsPath); err != nil { + if err := utils.VerifyResolvedPath(mp, appConfig.SystemState.Model.ModelsPath); err != nil { return err } modelPath = mp diff --git a/core/services/modeladmin/config.go b/core/services/modeladmin/config.go index 2cadfcaed..f85f29ee9 100644 --- a/core/services/modeladmin/config.go +++ b/core/services/modeladmin/config.go @@ -93,7 +93,7 @@ func (s *ConfigService) GetConfig(_ context.Context, name string) (*ConfigView, if configPath == "" { return nil, ErrConfigFileMissing } - if err := utils.VerifyPath(configPath, s.modelsPath()); err != nil { + if err := utils.VerifyResolvedPath(configPath, s.modelsPath()); err != nil { return nil, fmt.Errorf("%w: %v", ErrPathNotTrusted, err) } data, err := os.ReadFile(configPath) @@ -137,7 +137,7 @@ func (s *ConfigService) patchConfig(ctx context.Context, name string, patch map[ return nil, fmt.Errorf("%w: PATCH cannot rename model %q to %q; use the model edit endpoint", ErrInvalidConfig, name, patchedName) } configPath := cfg.GetModelConfigFile() - if err := utils.VerifyPath(configPath, s.modelsPath()); err != nil { + if err := utils.VerifyResolvedPath(configPath, s.modelsPath()); err != nil { return nil, fmt.Errorf("%w: %v", ErrPathNotTrusted, err) } diskYAML, err := os.ReadFile(configPath) @@ -289,7 +289,7 @@ func (s *ConfigService) editYAML(ctx context.Context, name string, body []byte) configPath := existing.GetModelConfigFile() modelsPath := s.modelsPath() - if err := utils.VerifyPath(configPath, modelsPath); err != nil { + if err := utils.VerifyResolvedPath(configPath, modelsPath); err != nil { return nil, fmt.Errorf("%w: %v", ErrPathNotTrusted, err) } @@ -304,7 +304,7 @@ func (s *ConfigService) editYAML(ctx context.Context, name string, body []byte) } newConfigPath := filepath.Join(modelsPath, req.Name+".yaml") paths = append(paths, newConfigPath, filepath.Join(modelsPath, gallery.GalleryFileName(name)), filepath.Join(modelsPath, gallery.GalleryFileName(req.Name))) - if err := utils.VerifyPath(newConfigPath, modelsPath); err != nil { + if err := utils.VerifyPath(req.Name+".yaml", modelsPath); err != nil { return nil, fmt.Errorf("%w: %v", ErrPathNotTrusted, err) } if _, err := os.Stat(newConfigPath); err == nil { diff --git a/core/services/modeladmin/config_path_trust_test.go b/core/services/modeladmin/config_path_trust_test.go new file mode 100644 index 000000000..5b375b699 --- /dev/null +++ b/core/services/modeladmin/config_path_trust_test.go @@ -0,0 +1,58 @@ +package modeladmin + +import ( + "context" + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// A model config can be loaded from outside the models directory (for +// example with --config-file). The admin mutations write the config file +// back, so they must refuse a file outside the models directory rather than +// write wherever the loader found it. +var _ = Describe("ConfigService config file containment", func() { + var ( + svc *ConfigService + ctx context.Context + outside string + orig []byte + ) + + BeforeEach(func() { + svc, _ = newTestService() + ctx = context.Background() + outside = filepath.Join(GinkgoT().TempDir(), "external.yaml") + orig = []byte("name: external\nbackend: llama-cpp\n") + Expect(os.WriteFile(outside, orig, 0o644)).To(Succeed()) + Expect(svc.Loader.ReadModelConfig(outside, svc.AppConfig.ToConfigLoaderOptions()...)).To(Succeed()) + cfg, ok := svc.Loader.GetModelConfig("external") + Expect(ok).To(BeTrue()) + Expect(cfg.GetModelConfigFile()).To(Equal(outside)) + }) + + It("refuses to pin a model whose config file is outside the models directory", func() { + _, err := svc.TogglePinned(ctx, "external", ActionPin, nil) + Expect(err).To(MatchError(ErrPathNotTrusted)) + Expect(os.ReadFile(outside)).To(Equal(orig)) + }) + + It("refuses to toggle the state of such a model", func() { + _, err := svc.ToggleState(ctx, "external", ActionDisable) + Expect(err).To(MatchError(ErrPathNotTrusted)) + Expect(os.ReadFile(outside)).To(Equal(orig)) + }) + + It("refuses to patch such a model", func() { + _, err := svc.PatchConfig(ctx, "external", map[string]any{"context_size": 4096}) + Expect(err).To(MatchError(ErrPathNotTrusted)) + Expect(os.ReadFile(outside)).To(Equal(orig)) + }) + + It("refuses to read such a model's config", func() { + _, err := svc.GetConfig(ctx, "external") + Expect(err).To(MatchError(ErrPathNotTrusted)) + }) +}) diff --git a/core/services/modeladmin/pinned.go b/core/services/modeladmin/pinned.go index b9ef45724..17c4a2f39 100644 --- a/core/services/modeladmin/pinned.go +++ b/core/services/modeladmin/pinned.go @@ -29,7 +29,7 @@ func (s *ConfigService) TogglePinned(_ context.Context, name string, action Acti if configPath == "" { return nil, ErrConfigFileMissing } - if err := utils.VerifyPath(configPath, s.modelsPath()); err != nil { + if err := utils.VerifyResolvedPath(configPath, s.modelsPath()); err != nil { return nil, fmt.Errorf("%w: %v", ErrPathNotTrusted, err) } if err := mutateYAMLBoolFlag(configPath, "pinned", action == ActionPin); err != nil { diff --git a/core/services/modeladmin/state.go b/core/services/modeladmin/state.go index d37368d9d..cee6d7855 100644 --- a/core/services/modeladmin/state.go +++ b/core/services/modeladmin/state.go @@ -49,7 +49,7 @@ func (s *ConfigService) toggleState(ctx context.Context, name string, action Act if configPath == "" { return nil, ErrConfigFileMissing } - if err := utils.VerifyPath(configPath, s.modelsPath()); err != nil { + if err := utils.VerifyResolvedPath(configPath, s.modelsPath()); err != nil { return nil, fmt.Errorf("%w: %v", ErrPathNotTrusted, err) } var result *ToggleResult diff --git a/docs/content/advanced/model-configuration.md b/docs/content/advanced/model-configuration.md index 1c2ade4de..5cfa74ccd 100644 --- a/docs/content/advanced/model-configuration.md +++ b/docs/content/advanced/model-configuration.md @@ -74,6 +74,8 @@ When using `--models-config-file`, you can define multiple models as a list: backend: llama-cpp ``` +LocalAI changes only config files that are inside the models directory. If the file from `--models-config-file` is outside the models directory, you cannot view, edit, pin, enable or disable its models from the web UI or the model admin API. Edit the file directly, then restart LocalAI. + ## Core Configuration Fields ### Basic Model Settings diff --git a/pkg/utils/path.go b/pkg/utils/path.go index df3af9072..16c29de15 100644 --- a/pkg/utils/path.go +++ b/pkg/utils/path.go @@ -27,12 +27,21 @@ func InTrustedRoot(path string, trustedRoot string) error { } } -// VerifyPath verifies that path is based in basePath. +// VerifyPath verifies that path, taken relative to basePath, is based in +// basePath. It joins path onto basePath first, so an absolute path is read as +// relative to the base as well: give it the untrusted relative name, never a +// path that has already been joined. For a full path use VerifyResolvedPath. func VerifyPath(path, basePath string) error { c := filepath.Clean(filepath.Join(basePath, path)) return InTrustedRoot(c, filepath.Clean(basePath)) } +// VerifyResolvedPath verifies that path, a full path rather than one relative +// to basePath, is based in basePath. +func VerifyResolvedPath(path, basePath string) error { + return InTrustedRoot(filepath.Clean(path), filepath.Clean(basePath)) +} + // SanitizeFileName sanitizes the given filename func SanitizeFileName(fileName string) string { // filepath.Clean to clean the path diff --git a/pkg/utils/path_test.go b/pkg/utils/path_test.go index 1c9d18b2c..e970a45ab 100644 --- a/pkg/utils/path_test.go +++ b/pkg/utils/path_test.go @@ -72,6 +72,25 @@ var _ = Describe("utils/path tests", func() { }) }) + Describe("VerifyResolvedPath", func() { + It("accepts a full path inside the base", func() { + Expect(VerifyResolvedPath("/srv/models/a/model.yaml", "/srv/models")).To(Succeed()) + }) + + It("rejects a full path outside the base", func() { + // VerifyPath would join this onto the base and accept it. + Expect(VerifyResolvedPath("/etc/passwd", "/srv/models")).ToNot(Succeed()) + }) + + It("rejects a joined path that climbed out of the base", func() { + Expect(VerifyResolvedPath(filepath.Join("/srv/models", "../other/x"), "/srv/models")).ToNot(Succeed()) + }) + + It("cleans both paths before comparing", func() { + Expect(VerifyResolvedPath("/srv/models/./a/../b.yaml", "/srv/models/")).To(Succeed()) + }) + }) + Describe("InTrustedRoot", func() { It("accepts a strict descendant of the trusted root", func() { Expect(InTrustedRoot("/srv/models/file", "/srv/models")).To(Succeed())