From 2758c75cae2896b7f1327fe00f60a1c017c0e0d1 Mon Sep 17 00:00:00 2001
From: Jakob Unterwurzacher <jakobunt@gmail.com>
Date: Sat, 10 Dec 2016 12:59:54 +0100
Subject: [PATCH] ctlsock: sanitize paths before passing them to the backend

You used to be able to crash gocryptfs by passing "/foo"
of "foo/" to the ctlsock.

Fixes https://github.com/rfjakob/gocryptfs/issues/66
---
 internal/ctlsock/ctlsock_serve.go | 18 ++++++++++++++++--
 internal/ctlsock/sanitize.go      | 20 ++++++++++++++++++++
 internal/ctlsock/sanitize_test.go | 25 +++++++++++++++++++++++++
 tests/defaults/main_test.go       |  7 +++++++
 4 files changed, 68 insertions(+), 2 deletions(-)
 create mode 100644 internal/ctlsock/sanitize.go
 create mode 100644 internal/ctlsock/sanitize_test.go

diff --git a/internal/ctlsock/ctlsock_serve.go b/internal/ctlsock/ctlsock_serve.go
index 571260d..63515c0 100644
--- a/internal/ctlsock/ctlsock_serve.go
+++ b/internal/ctlsock/ctlsock_serve.go
@@ -5,6 +5,7 @@ package ctlsock
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net"
 	"os"
@@ -35,6 +36,9 @@ type ResponseStruct struct {
 	ErrNo int32
 	// ErrText is a detailed error message.
 	ErrText string
+	// WarnText contains warnings that may have been encountered while
+	// processing the message.
+	WarnText string
 }
 
 type ctlSockHandler struct {
@@ -102,14 +106,19 @@ func (ch *ctlSockHandler) handleConnection(conn *net.UnixConn) {
 func (ch *ctlSockHandler) handleRequest(in *RequestStruct, conn *net.UnixConn) {
 	var err error
 	var out ResponseStruct
+	var inPath, clean string
 	if in.DecryptPath != "" && in.EncryptPath != "" {
 		err = errors.New("Ambigous")
 	} else if in.DecryptPath == "" && in.EncryptPath == "" {
 		err = errors.New("No operation")
 	} else if in.DecryptPath != "" {
-		out.Result, err = ch.fs.DecryptPath(in.DecryptPath)
+		inPath = in.DecryptPath
+		clean = SanitizePath(inPath)
+		out.Result, err = ch.fs.DecryptPath(clean)
 	} else if in.EncryptPath != "" {
-		out.Result, err = ch.fs.EncryptPath(in.EncryptPath)
+		inPath = in.EncryptPath
+		clean = SanitizePath(inPath)
+		out.Result, err = ch.fs.EncryptPath(clean)
 	}
 	if err != nil {
 		out.ErrText = err.Error()
@@ -121,6 +130,9 @@ func (ch *ctlSockHandler) handleRequest(in *RequestStruct, conn *net.UnixConn) {
 			}
 		}
 	}
+	if inPath != clean {
+		out.WarnText = fmt.Sprintf("Non-canonical input path %q has been interpreted as %q", inPath, clean)
+	}
 	sendResponse(&out, conn)
 }
 
@@ -130,6 +142,8 @@ func sendResponse(msg *ResponseStruct, conn *net.UnixConn) {
 		tlog.Warn.Printf("ctlsock: Marshal failed: %v", err)
 		return
 	}
+	// For convenience for the user, add a newline at the end.
+	jsonMsg = append(jsonMsg, '\n')
 	_, err = conn.Write(jsonMsg)
 	if err != nil {
 		tlog.Warn.Printf("ctlsock: Write failed: %v", err)
diff --git a/internal/ctlsock/sanitize.go b/internal/ctlsock/sanitize.go
new file mode 100644
index 0000000..5bc3706
--- /dev/null
+++ b/internal/ctlsock/sanitize.go
@@ -0,0 +1,20 @@
+package ctlsock
+
+import (
+	"path/filepath"
+)
+
+// SanitizePath adapts filepath.Clean for FUSE paths.
+// 1) It always returns a relative path
+// 2) It returns "" instead of "."
+// See the TestSanitizePath testcases for examples.
+func SanitizePath(path string) string {
+	clean := filepath.Clean(path)
+	if clean == "." || clean == "/" {
+		return ""
+	}
+	if clean[0] == '/' {
+		clean = clean[1:]
+	}
+	return clean
+}
diff --git a/internal/ctlsock/sanitize_test.go b/internal/ctlsock/sanitize_test.go
new file mode 100644
index 0000000..dfcb62c
--- /dev/null
+++ b/internal/ctlsock/sanitize_test.go
@@ -0,0 +1,25 @@
+package ctlsock
+
+import (
+	"testing"
+)
+
+func TestSanitizePath(t *testing.T) {
+	testCases := [][]string{
+		{"", ""},
+		{".", ""},
+		{"/", ""},
+		{"foo", "foo"},
+		{"/foo", "foo"},
+		{"foo/", "foo"},
+		{"/foo/", "foo"},
+		{"/foo/./foo", "foo/foo"},
+		{"./", ""},
+	}
+	for _, tc := range testCases {
+		res := SanitizePath(tc[0])
+		if res != tc[1] {
+			t.Errorf("%q: got %q, want %q", tc[0], res, tc[1])
+		}
+	}
+}
diff --git a/tests/defaults/main_test.go b/tests/defaults/main_test.go
index 1ad5f55..089f940 100644
--- a/tests/defaults/main_test.go
+++ b/tests/defaults/main_test.go
@@ -57,6 +57,13 @@ func TestCtlSock(t *testing.T) {
 	if response.ErrNo != int32(syscall.ENOENT) || response.Result != "" {
 		t.Errorf("incorrect error handling: %+v", response)
 	}
+	// Strange paths should not cause a crash
+	crashers := []string{"/foo", "foo/", "/foo/", ".", "/////", "/../../."}
+	for _, c := range crashers {
+		req.EncryptPath = c
+		// QueryCtlSock calls t.Fatal if it gets EOF when gocryptfs panics
+		test_helpers.QueryCtlSock(t, sock, req)
+	}
 }
 
 // In gocryptfs before v1.2, the file header was only read once for each