From 1d50d6dad8a9be45d67a448a0cc9a0910bd40720 Mon Sep 17 00:00:00 2001
From: Conrad Hoffmann <ch@bitfehler.net>
Date: Thu, 10 Mar 2022 16:46:56 +0100
Subject: [PATCH] Harden mapping from request path to FS path

Put strict checks in place to avoid authenticated users accessing files
outside of their actual storage directory. These checks will need
updating if multiple address books are to be supported.
---
 storage/filesystem.go | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/storage/filesystem.go b/storage/filesystem.go
index b8a3e0d..3ad71fd 100644
--- a/storage/filesystem.go
+++ b/storage/filesystem.go
@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 
 	"github.com/emersion/go-vcard"
 	"github.com/emersion/go-webdav/carddav"
@@ -38,7 +39,7 @@ func NewFilesystem(path string) (carddav.Backend, error) {
 func (b *filesystemBackend) pathForContext(ctx context.Context) (string, error) {
 	raw := ctx.Value(auth.AuthCtxKey)
 	if raw == nil {
-		return b.path, nil
+		return "", fmt.Errorf("unauthenticated requests are not supported")
 	}
 	authCtx, ok := raw.(*auth.AuthContext)
 	if !ok {
@@ -57,6 +58,20 @@ func (b *filesystemBackend) pathForContext(ctx context.Context) (string, error)
 	return path, nil
 }
 
+func (b *filesystemBackend) safePath(ctx context.Context, path string) (string, error) {
+	basePath, err := b.pathForContext(ctx)
+	if err != nil {
+		return "", err
+	}
+	// We are mapping to local filesystem path, so be conservative about what to accept
+	// TODO this changes once multiple addess books are supported
+	var valid = regexp.MustCompile(`^/[A-Za-z0-9_-]+(.[a-zA-Z]+)?$`)
+	if !valid.MatchString(path) {
+		return "", fmt.Errorf("invalid request path")
+	}
+	return filepath.Join(basePath, path), nil
+}
+
 func etagForFile(path string) (string, error) {
 	data, err := ioutil.ReadFile(path)
 	if err != nil {
@@ -153,13 +168,12 @@ func (b *filesystemBackend) AddressBook(ctx context.Context) (*carddav.AddressBo
 }
 
 func (b *filesystemBackend) GetAddressObject(ctx context.Context, path string, req *carddav.AddressDataRequest) (*carddav.AddressObject, error) {
-	basePath, err := b.pathForContext(ctx)
+	localPath, err := b.safePath(ctx, path)
 	if err != nil {
 		return nil, err
 	}
-	path = filepath.Join(basePath, path)
 
-	info, err := os.Stat(path)
+	info, err := os.Stat(localPath)
 	if err != nil {
 		return nil, err
 	}
@@ -169,18 +183,18 @@ func (b *filesystemBackend) GetAddressObject(ctx context.Context, path string, r
 		propFilter = req.Props
 	}
 
-	card, err := vcardFromFile(path, propFilter)
+	card, err := vcardFromFile(localPath, propFilter)
 	if err != nil {
 		return nil, err
 	}
 
-	etag, err := etagForFile(path)
+	etag, err := etagForFile(localPath)
 	if err != nil {
 		return nil, err
 	}
 
 	obj := carddav.AddressObject{
-		Path: "/" + filepath.Base(path),
+		Path: path,
 		ModTime: info.ModTime(),
 		ETag: etag,
 		Card: *card,
@@ -249,12 +263,11 @@ func (b *filesystemBackend) QueryAddressObjects(ctx context.Context, query *card
 }
 
 func (b *filesystemBackend) PutAddressObject(ctx context.Context, path string, card vcard.Card) (loc string, err error) {
-	basePath, err := b.pathForContext(ctx)
+	localPath, err := b.safePath(ctx, path)
 	if err != nil {
 		return "", err
 	}
-	internalPath := filepath.Join(basePath, path)
-	f, err := os.OpenFile(internalPath, os.O_RDWR|os.O_CREATE, 0644)
+	f, err := os.OpenFile(localPath, os.O_RDWR|os.O_CREATE, 0644)
 	if err != nil {
 		return "", err
 	}
@@ -270,13 +283,11 @@ func (b *filesystemBackend) PutAddressObject(ctx context.Context, path string, c
 }
 
 func (b *filesystemBackend) DeleteAddressObject(ctx context.Context, path string) error {
-	basePath, err := b.pathForContext(ctx)
+	localPath, err := b.safePath(ctx, path)
 	if err != nil {
 		return err
 	}
-	path = filepath.Join(basePath, path)
-	//TODO does this need more security/sanity checks?
-	err = os.Remove(path)
+	err = os.Remove(localPath)
 	if err != nil {
 		return err
 	}