Fix vulnerability in authenticated secrets API

This patch fixes a vulnerability in the secrets API, however
it is important to stress that the user must be authenticated
as the admin user on the REST API before they can attempt this.

Reported by Appsecco via email. @lucasroesler, Appsecco and
myself believe this to be of low severity.

The fix prevents directory traversal characters from being
used in secret names. If a secret name such as:
../../root/.ssh/authorized_keys were to be used, an attacker
could remove the value and write their own.

Tested with unit tests and tests are now made to run
via the CI and a new Makefile target.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>

.travis.yml

@@ -10,6 +10,7 @@ addons:
       - runc
 
 script:
+- make test
 - make dist
 - make prepare-test
 - make test-e2e

Makefile

@@ -11,6 +11,10 @@ all: local
 local:
 	CGO_ENABLED=0 GOOS=linux go build -o bin/faasd
 
+.PHONY: test
+test:
+	CGO_ENABLED=0 GOOS=linux go test -ldflags $(LDFLAGS) ./...
+
 .PHONY: dist
 dist:
 	CGO_ENABLED=0 GOOS=linux go build -ldflags $(LDFLAGS) -a -installsuffix cgo -o bin/faasd

pkg/provider/handlers/secret.go

@@ -2,11 +2,13 @@ package handlers
 
 import (
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"log"
 	"net/http"
 	"os"
 	"path"
+	"strings"
 
 	"github.com/containerd/containerd"
 	"github.com/openfaas/faas-provider/types"
@@ -76,17 +78,6 @@ func createSecret(c *containerd.Client, w http.ResponseWriter, r *http.Request,
 	}
 }
 
-func parseSecret(r *http.Request) (types.Secret, error) {
-	secret := types.Secret{}
-	bytesOut, err := ioutil.ReadAll(r.Body)
-	if err != nil {
-		return secret, err
-	}
-
-	err = json.Unmarshal(bytesOut, &secret)
-	return secret, err
-}
-
 func deleteSecret(c *containerd.Client, w http.ResponseWriter, r *http.Request, mountPath string) {
 	secret, err := parseSecret(r)
 	if err != nil {
@@ -103,3 +94,29 @@ func deleteSecret(c *containerd.Client, w http.ResponseWriter, r *http.Request,
 		return
 	}
 }
+
+func parseSecret(r *http.Request) (types.Secret, error) {
+	secret := types.Secret{}
+	bytesOut, err := ioutil.ReadAll(r.Body)
+	if err != nil {
+		return secret, err
+	}
+
+	err = json.Unmarshal(bytesOut, &secret)
+	if err != nil {
+		return secret, err
+	}
+
+	if isTraversal(secret.Name) {
+		return secret, fmt.Errorf(traverseErrorSt)
+	}
+
+	return secret, err
+}
+
+const traverseErrorSt = "directory traversal found in name"
+
+func isTraversal(name string) bool {
+	return strings.Contains(name, fmt.Sprintf("%s", string(os.PathSeparator))) ||
+		strings.Contains(name, "..")
+}

pkg/provider/handlers/secret_test.go

@@ -0,0 +1,63 @@
+package handlers
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/openfaas/faas-provider/types"
+)
+
+func Test_parseSecretValidName(t *testing.T) {
+
+	s := types.Secret{Name: "authorized_keys"}
+	body, _ := json.Marshal(s)
+	reader := bytes.NewReader(body)
+	r := httptest.NewRequest(http.MethodPost, "/", reader)
+	_, err := parseSecret(r)
+
+	if err != nil {
+		t.Fatalf("secret name is valid with no traversal characters")
+	}
+}
+
+func Test_parseSecretValidNameWithDot(t *testing.T) {
+
+	s := types.Secret{Name: "authorized.keys"}
+	body, _ := json.Marshal(s)
+	reader := bytes.NewReader(body)
+	r := httptest.NewRequest(http.MethodPost, "/", reader)
+	_, err := parseSecret(r)
+
+	if err != nil {
+		t.Fatalf("secret name is valid with no traversal characters")
+	}
+}
+
+func Test_parseSecretWithTraversalWithSlash(t *testing.T) {
+
+	s := types.Secret{Name: "/root/.ssh/authorized_keys"}
+	body, _ := json.Marshal(s)
+	reader := bytes.NewReader(body)
+	r := httptest.NewRequest(http.MethodPost, "/", reader)
+	_, err := parseSecret(r)
+
+	if err == nil {
+		t.Fatalf("secret name should fail due to path traversal")
+	}
+}
+
+func Test_parseSecretWithTraversalWithDoubleDot(t *testing.T) {
+
+	s := types.Secret{Name: ".."}
+	body, _ := json.Marshal(s)
+	reader := bytes.NewReader(body)
+	r := httptest.NewRequest(http.MethodPost, "/", reader)
+	_, err := parseSecret(r)
+
+	if err == nil {
+		t.Fatalf("secret name should fail due to path traversal")
+	}
+}