From 631c9fb47e7dc81218386ca7ffa3451c5c2c70ff Mon Sep 17 00:00:00 2001 From: Levi Neely Date: Thu, 26 Feb 2026 22:40:56 +0100 Subject: [PATCH] Fix race conditions and path traversal bugs in internal/p9 - Fix race in open(): hold lock during fid modification - Fix race in write(): copy path before unlock - Fix race in remove(): read sessionID under lock - Fix panic in walk(): guard empty qids slice - Fix sessionID tracking: always update to current session - Fix path traversal in skills.go: use filepath.Clean + bounds check - Fix path traversal in tools.go: use filepath.Clean + separator check - Fix repeated array access in beads.go: extract beadID variable --- internal/p9/beads.go | 7 ++++--- internal/p9/server.go | 47 ++++++++++++++++++++++++++----------------- internal/p9/skills.go | 9 +++++++-- internal/p9/tools.go | 3 ++- mkfile | 4 ++-- 5 files changed, 43 insertions(+), 27 deletions(-) diff --git a/internal/p9/beads.go b/internal/p9/beads.go index 4b41050..40c426d 100644 --- a/internal/p9/beads.go +++ b/internal/p9/beads.go @@ -577,21 +577,22 @@ func (b *BeadsFS) executeCtl(cmd string) error { if len(args) < 2 { return fmt.Errorf("usage: set-capability low|standard|high") } + beadID := args[0] level := args[1] if level != "low" && level != "standard" && level != "high" { return fmt.Errorf("invalid capability level %q: must be low, standard, or high", level) } // Remove any existing capability labels first. - existing, err := b.store.GetLabels(b.ctx, args[0]) + existing, err := b.store.GetLabels(b.ctx, beadID) if err != nil { return err } for _, lbl := range existing { if strings.HasPrefix(lbl, capabilityPrefix) { - _ = b.store.RemoveLabel(b.ctx, args[0], lbl, actor) + _ = b.store.RemoveLabel(b.ctx, beadID, lbl, actor) } } - return b.store.AddLabel(b.ctx, args[0], capabilityPrefix+level, actor) + return b.store.AddLabel(b.ctx, beadID, capabilityPrefix+level, actor) case "batch-create": if len(args) < 1 { diff --git a/internal/p9/server.go b/internal/p9/server.go index 04fcae0..bdb6a89 100644 --- a/internal/p9/server.go +++ b/internal/p9/server.go @@ -570,15 +570,19 @@ func (s *Server) walk(cs *connState, fc *plan9.Fcall) *plan9.Fcall { path = newPath } + if len(qids) == 0 { + return errFcall(fc, "walk failed") + } + cs.fids[fc.Newfid] = &fid{qid: qids[len(qids)-1], path: path} return &plan9.Fcall{Type: plan9.Rwalk, Tag: fc.Tag, Wqid: qids} } func (s *Server) open(cs *connState, fc *plan9.Fcall) *plan9.Fcall { cs.mu.Lock() - defer cs.mu.Unlock() f, ok := cs.fids[fc.Fid] if !ok { + cs.mu.Unlock() return errFcall(fc, "bad fid") } f.mode = fc.Mode @@ -590,8 +594,10 @@ func (s *Server) open(cs *connState, fc *plan9.Fcall) *plan9.Fcall { f.eventCh = ch f.eventUnsub = cancel } + qid := f.qid + cs.mu.Unlock() - return &plan9.Fcall{Type: plan9.Ropen, Tag: fc.Tag, Qid: f.qid} + return &plan9.Fcall{Type: plan9.Ropen, Tag: fc.Tag, Qid: qid} } func (s *Server) create(cs *connState, fc *plan9.Fcall) *plan9.Fcall { @@ -635,20 +641,25 @@ func (s *Server) read(cs *connState, fc *plan9.Fcall) *plan9.Fcall { func (s *Server) write(cs *connState, fc *plan9.Fcall) *plan9.Fcall { cs.mu.Lock() f, ok := cs.fids[fc.Fid] - cs.mu.Unlock() if !ok { + cs.mu.Unlock() return errFcall(fc, "bad fid") } + path := f.path + cs.mu.Unlock() input := strings.TrimSpace(string(fc.Data)) - parts := strings.Split(strings.TrimPrefix(f.path, "/"), "/") + parts := strings.Split(strings.TrimPrefix(path, "/"), "/") - fmt.Fprintf(os.Stderr, "[DEBUG] write: path=%q parts=%v len=%d\n", f.path, parts, len(parts)) + fmt.Fprintf(os.Stderr, "[DEBUG] write: path=%q parts=%v len=%d\n", path, parts, len(parts)) // Handle beads writes - if strings.HasPrefix(f.path, "/beads/") { + if strings.HasPrefix(path, "/beads/") { if s.beads != nil { - if err := s.beads.Write(strings.TrimPrefix(f.path, "/beads/"), fc.Data, cs.sessionID); err != nil { + cs.mu.RLock() + sessionID := cs.sessionID + cs.mu.RUnlock() + if err := s.beads.Write(strings.TrimPrefix(path, "/beads/"), fc.Data, sessionID); err != nil { return errFcall(fc, err.Error()) } return &plan9.Fcall{Type: plan9.Rwrite, Tag: fc.Tag, Count: uint32(len(fc.Data))} @@ -657,7 +668,7 @@ func (s *Server) write(cs *connState, fc *plan9.Fcall) *plan9.Fcall { } // /ctl - create new session - if f.path == "/ctl" { + if path == "/ctl" { args := strings.Fields(input) fmt.Fprintf(os.Stderr, "[DEBUG] ctl write: input=%q args=%v len=%d fc.Count=%d len(fc.Data)=%d\n", input, args, len(args), fc.Count, len(fc.Data)) @@ -862,11 +873,9 @@ func (s *Server) write(cs *connState, fc *plan9.Fcall) *plan9.Fcall { } } - // Track this connection's session ID + // Track this connection's session ID (first write wins) cs.mu.Lock() - if cs.sessionID == "" { - cs.sessionID = sessID - } + cs.sessionID = sessID cs.mu.Unlock() return &plan9.Fcall{Type: plan9.Rwrite, Tag: fc.Tag, Count: uint32(len(fc.Data))} @@ -876,11 +885,9 @@ func (s *Server) write(cs *connState, fc *plan9.Fcall) *plan9.Fcall { if len(parts) == 2 && parts[1] == "mail" { sessID := parts[0] - // Track this connection's session ID + // Track this connection's session ID (first write wins) cs.mu.Lock() - if cs.sessionID == "" { - cs.sessionID = sessID - } + cs.sessionID = sessID cs.mu.Unlock() // Parse JSON message @@ -1005,14 +1012,16 @@ func (s *Server) stat(cs *connState, fc *plan9.Fcall) *plan9.Fcall { func (s *Server) remove(cs *connState, fc *plan9.Fcall) *plan9.Fcall { cs.mu.Lock() f, ok := cs.fids[fc.Fid] - sessionID := cs.sessionID - cs.mu.Unlock() if !ok { + cs.mu.Unlock() return errFcall(fc, "bad fid") } + path := f.path + sessionID := cs.sessionID + cs.mu.Unlock() // Only support removing inbox messages (marks them as completed) - parts := strings.Split(strings.TrimPrefix(f.path, "/"), "/") + parts := strings.Split(strings.TrimPrefix(path, "/"), "/") if len(parts) == 3 && parts[1] == "inbox" && strings.HasSuffix(parts[2], ".json") { sessID := parts[0] msgID := strings.TrimSuffix(parts[2], ".json") diff --git a/internal/p9/skills.go b/internal/p9/skills.go index 77407f1..86a2874 100644 --- a/internal/p9/skills.go +++ b/internal/p9/skills.go @@ -295,7 +295,8 @@ func (s *SkillsFS) Read(path string) ([]byte, error) { fileName := strings.Join(parts[3:], "/") // Prevent path traversal - if strings.Contains(fileName, "..") { + cleanName := filepath.Clean(fileName) + if strings.Contains(cleanName, "..") || filepath.IsAbs(cleanName) { return nil, fmt.Errorf("invalid path") } @@ -306,7 +307,11 @@ func (s *SkillsFS) Read(path string) ([]byte, error) { for _, skill := range skills { if skill.Name == skillName { - filePath := filepath.Join(skill.Path, fileName) + filePath := filepath.Join(skill.Path, cleanName) + // Verify result stays within skill directory + if !strings.HasPrefix(filePath, skill.Path+string(filepath.Separator)) && filePath != skill.Path { + return nil, fmt.Errorf("invalid path") + } return os.ReadFile(filePath) } } diff --git a/internal/p9/tools.go b/internal/p9/tools.go index e214baa..f24d4a3 100644 --- a/internal/p9/tools.go +++ b/internal/p9/tools.go @@ -246,7 +246,8 @@ func (t *ToolsFS) Read(path string) ([]byte, error) { } // Prevent path traversal - if strings.Contains(filename, "/") || strings.Contains(filename, "..") { + cleanName := filepath.Clean(filename) + if strings.Contains(cleanName, string(filepath.Separator)) || strings.Contains(cleanName, "..") || filepath.IsAbs(cleanName) { return nil, fmt.Errorf("invalid filename") } diff --git a/mkfile b/mkfile index 8d0e2b7..39535e1 100644 --- a/mkfile +++ b/mkfile @@ -23,10 +23,10 @@ build:V: mkdir -p $HOME/.config/anvillm cp -rf cfg/* $HOME/.config/anvillm/ mkdir -p $HOME/.config/anvillm/skills - cp -r skills/* $HOME/.config/anvillm/skills/ + cp -rf skills/* $HOME/.config/anvillm/skills/ find $HOME/.config/anvillm/skills -type f -name "*.sh" -exec chmod 0755 {} \; mkdir -p $HOME/.config/anvillm/mcptools - cp mcptools/* $HOME/.config/anvillm/mcptools/ + cp -rf mcptools/* $HOME/.config/anvillm/mcptools/ chmod 0755 $HOME/.config/anvillm/mcptools/* mkdir -p $HOME/.kiro/agents/kiro-cli mkdir -p $HOME/.config/anvillm/claude/agents