fix(coredeno): harden security and fix review issues
- Path traversal: CheckPath now requires separator after prefix match - Store namespace: block reserved '_' prefixed groups - StoreGet: distinguish ErrNotFound from real DB errors via sentinel - Store: add rows.Err() checks in GetAll and Render - gRPC leak: cleanupGRPC on all early-return error paths in OnStartup - DenoClient: fix fmt.Sprint(nil) → type assertions - Socket permissions: 0700 dirs, 0600 sockets (owner-only) - Marketplace: persist SignKey, re-verify manifest on Update - io/local: resolve symlinks in New() (macOS /var → /private/var) - Tests: fix sun_path length overflow on macOS Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
parent
9899398153
commit
19e3fd3af7
11 changed files with 90 additions and 18 deletions
|
|
@ -88,9 +88,10 @@ func (c *DenoClient) LoadModule(code, entryPoint string, perms ModulePermissions
|
|||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
errStr, _ := resp["error"].(string)
|
||||
return &LoadModuleResponse{
|
||||
Ok: resp["ok"] == true,
|
||||
Error: fmt.Sprint(resp["error"]),
|
||||
Error: errStr,
|
||||
}, nil
|
||||
}
|
||||
|
||||
|
|
@ -128,8 +129,10 @@ func (c *DenoClient) ModuleStatus(code string) (*ModuleStatusResponse, error) {
|
|||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
respCode, _ := resp["code"].(string)
|
||||
sts, _ := resp["status"].(string)
|
||||
return &ModuleStatusResponse{
|
||||
Code: fmt.Sprint(resp["code"]),
|
||||
Status: fmt.Sprint(resp["status"]),
|
||||
Code: respCode,
|
||||
Status: sts,
|
||||
}, nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -17,9 +17,9 @@ func (s *Sidecar) Start(ctx context.Context, args ...string) error {
|
|||
return fmt.Errorf("coredeno: already running")
|
||||
}
|
||||
|
||||
// Ensure socket directory exists
|
||||
// Ensure socket directory exists with owner-only permissions
|
||||
sockDir := filepath.Dir(s.opts.SocketPath)
|
||||
if err := os.MkdirAll(sockDir, 0755); err != nil {
|
||||
if err := os.MkdirAll(sockDir, 0700); err != nil {
|
||||
return fmt.Errorf("coredeno: mkdir %s: %w", sockDir, err)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ package coredeno
|
|||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"net"
|
||||
"os"
|
||||
|
||||
|
|
@ -21,6 +22,11 @@ func ListenGRPC(ctx context.Context, socketPath string, srv *Server) error {
|
|||
if err != nil {
|
||||
return err
|
||||
}
|
||||
// Restrict socket to owner only — prevents other users from sending gRPC commands.
|
||||
if err := os.Chmod(socketPath, 0600); err != nil {
|
||||
listener.Close()
|
||||
return fmt.Errorf("chmod socket: %w", err)
|
||||
}
|
||||
defer func() {
|
||||
_ = listener.Close()
|
||||
_ = os.Remove(socketPath)
|
||||
|
|
|
|||
|
|
@ -76,10 +76,14 @@ func TestListenGRPC_Good(t *testing.T) {
|
|||
}
|
||||
|
||||
func TestListenGRPC_Bad_StaleSocket(t *testing.T) {
|
||||
sockDir := t.TempDir()
|
||||
sockPath := filepath.Join(sockDir, "stale.sock")
|
||||
// Use a short temp dir — macOS limits Unix socket paths to 104 bytes (sun_path)
|
||||
// and t.TempDir() + this test's long name can exceed that.
|
||||
sockDir, err := os.MkdirTemp("", "grpc")
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(func() { os.RemoveAll(sockDir) })
|
||||
sockPath := filepath.Join(sockDir, "s.sock")
|
||||
|
||||
// Create a stale socket file
|
||||
// Create a stale regular file where the socket should go
|
||||
require.NoError(t, os.WriteFile(sockPath, []byte("stale"), 0644))
|
||||
|
||||
medium := io.NewMockMedium()
|
||||
|
|
@ -97,13 +101,19 @@ func TestListenGRPC_Bad_StaleSocket(t *testing.T) {
|
|||
errCh <- ListenGRPC(ctx, sockPath, srv)
|
||||
}()
|
||||
|
||||
// Should replace stale file and start listening
|
||||
// Should replace stale file and start listening.
|
||||
// Also watch errCh — if ListenGRPC returns early, fail with the actual error.
|
||||
require.Eventually(t, func() bool {
|
||||
select {
|
||||
case err := <-errCh:
|
||||
t.Fatalf("ListenGRPC returned early: %v", err)
|
||||
return false
|
||||
default:
|
||||
}
|
||||
info, err := os.Stat(sockPath)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
// Socket file type, not regular file
|
||||
return info.Mode()&os.ModeSocket != 0
|
||||
}, 2*time.Second, 10*time.Millisecond, "socket should replace stale file")
|
||||
|
||||
|
|
|
|||
|
|
@ -14,7 +14,9 @@ func CheckPath(path string, allowed []string) bool {
|
|||
clean := filepath.Clean(path)
|
||||
for _, prefix := range allowed {
|
||||
cleanPrefix := filepath.Clean(prefix)
|
||||
if strings.HasPrefix(clean, cleanPrefix) {
|
||||
// Exact match or path is under the prefix directory.
|
||||
// The separator check prevents "data" matching "data-secrets".
|
||||
if clean == cleanPrefix || strings.HasPrefix(clean, cleanPrefix+string(filepath.Separator)) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,7 +2,9 @@ package coredeno
|
|||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"strings"
|
||||
|
||||
pb "forge.lthn.ai/core/go/pkg/coredeno/proto"
|
||||
"forge.lthn.ai/core/go/pkg/io"
|
||||
|
|
@ -133,17 +135,36 @@ func (s *Server) FileDelete(_ context.Context, req *pb.FileDeleteRequest) (*pb.F
|
|||
return &pb.FileDeleteResponse{Ok: true}, nil
|
||||
}
|
||||
|
||||
// StoreGet implements CoreService.StoreGet.
|
||||
// storeGroupAllowed checks that the requested group is not a reserved system namespace.
|
||||
// Groups prefixed with "_" are reserved for internal use (e.g. _coredeno, _modules).
|
||||
// TODO: once the proto carries module_code on store requests, enforce per-module namespace isolation.
|
||||
func storeGroupAllowed(group string) error {
|
||||
if strings.HasPrefix(group, "_") {
|
||||
return status.Errorf(codes.PermissionDenied, "reserved store group: %s", group)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// StoreGet implements CoreService.StoreGet with reserved namespace protection.
|
||||
func (s *Server) StoreGet(_ context.Context, req *pb.StoreGetRequest) (*pb.StoreGetResponse, error) {
|
||||
if err := storeGroupAllowed(req.Group); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
val, err := s.store.Get(req.Group, req.Key)
|
||||
if err != nil {
|
||||
return &pb.StoreGetResponse{Found: false}, nil
|
||||
if errors.Is(err, store.ErrNotFound) {
|
||||
return &pb.StoreGetResponse{Found: false}, nil
|
||||
}
|
||||
return nil, status.Errorf(codes.Internal, "store: %v", err)
|
||||
}
|
||||
return &pb.StoreGetResponse{Value: val, Found: true}, nil
|
||||
}
|
||||
|
||||
// StoreSet implements CoreService.StoreSet.
|
||||
// StoreSet implements CoreService.StoreSet with reserved namespace protection.
|
||||
func (s *Server) StoreSet(_ context.Context, req *pb.StoreSetRequest) (*pb.StoreSetResponse, error) {
|
||||
if err := storeGroupAllowed(req.Group); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if err := s.store.Set(req.Group, req.Key, req.Value); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
|
|
|||
|
|
@ -95,24 +95,36 @@ func (s *Service) OnStartup(ctx context.Context) error {
|
|||
s.grpcDone <- ListenGRPC(grpcCtx, opts.SocketPath, s.grpcServer)
|
||||
}()
|
||||
|
||||
// cleanupGRPC tears down the listener on early-return errors.
|
||||
cleanupGRPC := func() {
|
||||
grpcCancel()
|
||||
<-s.grpcDone
|
||||
}
|
||||
|
||||
// 6. Start sidecar (if args provided)
|
||||
if len(opts.SidecarArgs) > 0 {
|
||||
// Wait for core socket so sidecar can connect to our gRPC server
|
||||
if err := waitForSocket(ctx, opts.SocketPath, 5*time.Second); err != nil {
|
||||
cleanupGRPC()
|
||||
return fmt.Errorf("coredeno: core socket: %w", err)
|
||||
}
|
||||
|
||||
if err := s.sidecar.Start(ctx, opts.SidecarArgs...); err != nil {
|
||||
cleanupGRPC()
|
||||
return fmt.Errorf("coredeno: sidecar: %w", err)
|
||||
}
|
||||
|
||||
// 7. Wait for Deno's server and connect as client
|
||||
if opts.DenoSocketPath != "" {
|
||||
if err := waitForSocket(ctx, opts.DenoSocketPath, 10*time.Second); err != nil {
|
||||
_ = s.sidecar.Stop()
|
||||
cleanupGRPC()
|
||||
return fmt.Errorf("coredeno: deno socket: %w", err)
|
||||
}
|
||||
dc, err := DialDeno(opts.DenoSocketPath)
|
||||
if err != nil {
|
||||
_ = s.sidecar.Stop()
|
||||
cleanupGRPC()
|
||||
return fmt.Errorf("coredeno: deno client: %w", err)
|
||||
}
|
||||
s.denoClient = dc
|
||||
|
|
|
|||
|
|
@ -24,6 +24,11 @@ func New(root string) (*Medium, error) {
|
|||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
// Resolve symlinks so sandbox checks compare like-for-like.
|
||||
// macOS /var → /private/var causes false sandbox escapes without this.
|
||||
if resolved, err := filepath.EvalSymlinks(abs); err == nil {
|
||||
abs = resolved
|
||||
}
|
||||
return &Medium{root: abs}, nil
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -14,7 +14,8 @@ func TestNew(t *testing.T) {
|
|||
root := t.TempDir()
|
||||
m, err := New(root)
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, root, m.root)
|
||||
resolved, _ := filepath.EvalSymlinks(root)
|
||||
assert.Equal(t, resolved, m.root)
|
||||
}
|
||||
|
||||
func TestPath(t *testing.T) {
|
||||
|
|
|
|||
|
|
@ -40,6 +40,7 @@ type InstalledModule struct {
|
|||
Repo string `json:"repo"`
|
||||
EntryPoint string `json:"entry_point"`
|
||||
Permissions manifest.Permissions `json:"permissions"`
|
||||
SignKey string `json:"sign_key,omitempty"`
|
||||
InstalledAt string `json:"installed_at"`
|
||||
}
|
||||
|
||||
|
|
@ -84,6 +85,7 @@ func (i *Installer) Install(ctx context.Context, mod Module) error {
|
|||
Repo: mod.Repo,
|
||||
EntryPoint: entryPoint,
|
||||
Permissions: m.Permissions,
|
||||
SignKey: mod.SignKey,
|
||||
InstalledAt: time.Now().UTC().Format(time.RFC3339),
|
||||
}
|
||||
|
||||
|
|
@ -131,12 +133,12 @@ func (i *Installer) Update(ctx context.Context, code string) error {
|
|||
return fmt.Errorf("marketplace: pull: %s: %w", strings.TrimSpace(string(output)), err)
|
||||
}
|
||||
|
||||
// Reload manifest
|
||||
// Reload and re-verify manifest with the same key used at install time
|
||||
medium, mErr := io.NewSandboxed(dest)
|
||||
if mErr != nil {
|
||||
return fmt.Errorf("marketplace: medium: %w", mErr)
|
||||
}
|
||||
m, mErr := manifest.Load(medium, ".")
|
||||
m, mErr := loadManifest(medium, installed.SignKey)
|
||||
if mErr != nil {
|
||||
return fmt.Errorf("marketplace: reload manifest: %w", mErr)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ package store
|
|||
|
||||
import (
|
||||
"database/sql"
|
||||
"errors"
|
||||
"fmt"
|
||||
"strings"
|
||||
"text/template"
|
||||
|
|
@ -9,6 +10,9 @@ import (
|
|||
_ "modernc.org/sqlite"
|
||||
)
|
||||
|
||||
// ErrNotFound is returned when a key does not exist in the store.
|
||||
var ErrNotFound = errors.New("store: not found")
|
||||
|
||||
// Store is a group-namespaced key-value store backed by SQLite.
|
||||
type Store struct {
|
||||
db *sql.DB
|
||||
|
|
@ -46,7 +50,7 @@ func (s *Store) Get(group, key string) (string, error) {
|
|||
var val string
|
||||
err := s.db.QueryRow("SELECT value FROM kv WHERE grp = ? AND key = ?", group, key).Scan(&val)
|
||||
if err == sql.ErrNoRows {
|
||||
return "", fmt.Errorf("store.Get: not found: %s/%s", group, key)
|
||||
return "", fmt.Errorf("store.Get: %s/%s: %w", group, key, ErrNotFound)
|
||||
}
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("store.Get: %w", err)
|
||||
|
|
@ -111,6 +115,9 @@ func (s *Store) GetAll(group string) (map[string]string, error) {
|
|||
}
|
||||
result[k] = v
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, fmt.Errorf("store.GetAll: rows: %w", err)
|
||||
}
|
||||
return result, nil
|
||||
}
|
||||
|
||||
|
|
@ -130,6 +137,9 @@ func (s *Store) Render(tmplStr, group string) (string, error) {
|
|||
}
|
||||
vars[k] = v
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return "", fmt.Errorf("store.Render: rows: %w", err)
|
||||
}
|
||||
|
||||
tmpl, err := template.New("render").Parse(tmplStr)
|
||||
if err != nil {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue