Harden default challenge code storage and name lookups

libstns/owner_other.go

@@ -0,0 +1,9 @@
+//go:build !unix
+
+package libstns
+
+import "os"
+
+func isOwnedByCurrentUser(fi os.FileInfo) bool {
+	return true
+}

libstns/owner_unix.go

@@ -0,0 +1,16 @@
+//go:build unix
+
+package libstns
+
+import (
+	"os"
+	"syscall"
+)
+
+func isOwnedByCurrentUser(fi os.FileInfo) bool {
+	st, ok := fi.Sys().(*syscall.Stat_t)
+	if !ok {
+		return false
+	}
+	return int(st.Uid) == os.Geteuid()
+}

libstns/stns.go

@@ -6,9 +6,10 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"net/url"
 	"os"
 	"os/user"
-	"path"
+	"path/filepath"
 	"strings"
 
 	"github.com/STNS/STNS/v2/model"
@@ -24,19 +25,47 @@ type STNS struct {
 	popChallengeCode   func(string) ([]byte, error)
 }
 
+// user must be a single path element so the code stays directly under os.TempDir().
+func challengeCodePath(user string) (string, error) {
+	if user == "" || user == "." || user == ".." || strings.ContainsAny(user, "/\\\x00") {
+		return "", errors.New("invalid user name")
+	}
+	return filepath.Join(os.TempDir(), user), nil
+}
+
 func DefaultStoreChallengeCode(user string, code []byte) error {
-	fmt.Sprint(path.Join(os.TempDir(), user))
-	err := ioutil.WriteFile(path.Join(os.TempDir(), user), code, 0600)
+	p, err := challengeCodePath(user)
 	if err != nil {
 		return err
 	}
-	return nil
+	if err := os.Remove(p); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	f, err := os.OpenFile(p, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600)
+	if err != nil {
+		return err
+	}
+	if _, err := f.Write(code); err != nil {
+		f.Close()
+		return err
+	}
+	return f.Close()
 }
 
 func DefaultPopChallengeCode(user string) ([]byte, error) {
-	p := path.Join(os.TempDir(), user)
+	p, err := challengeCodePath(user)
+	if err != nil {
+		return nil, err
+	}
+	fi, err := os.Lstat(p)
+	if err != nil {
+		return nil, err
+	}
+	if !fi.Mode().IsRegular() || !isOwnedByCurrentUser(fi) {
+		return nil, errors.New("invalid challenge code file")
+	}
 	defer os.Remove(p)
-	return ioutil.ReadFile(p)
+	return os.ReadFile(p)
 }
 
 func DefaultMakeChallengeCode() ([]byte, error) {
@@ -120,7 +149,7 @@ func (s *STNS) ListUser() ([]*model.User, error) {
 }
 
 func (s *STNS) GetUserByName(name string) (*model.User, error) {
-	r, err := s.client.Request(usersEndpoint, fmt.Sprintf("name=%s", name))
+	r, err := s.client.Request(usersEndpoint, url.Values{"name": {name}}.Encode())
 	if err != nil {
 		return nil, err
 	}
@@ -167,7 +196,7 @@ func (s *STNS) ListGroup() ([]*model.Group, error) {
 }
 
 func (s *STNS) GetGroupByName(name string) (*model.Group, error) {
-	r, err := s.client.Request(groupsEndpoint, fmt.Sprintf("name=%s", name))
+	r, err := s.client.Request(groupsEndpoint, url.Values{"name": {name}}.Encode())
 	if err != nil {
 		return nil, err
 	}

libstns/stns_test.go

@@ -1,10 +1,13 @@
 package libstns
 
 import (
+	"bytes"
 	"encoding/json"
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"reflect"
 	"testing"
 
@@ -122,6 +125,19 @@ func TestSTNS_GetUserByName(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "special characters",
+			args: args{
+				name: "foo&id=1#bar",
+			},
+			want: &model.User{
+				Base: model.Base{
+					ID:   3,
+					Name: "foo&id=1#bar",
+				},
+			},
+			wantErr: false,
+		},
 		{
 			name: "notfound",
 			args: args{
@@ -131,15 +147,18 @@ func TestSTNS_GetUserByName(t *testing.T) {
 			wantErr: true,
 		},
 	}
+	ids := map[string]int{"example1": 1, "foo&id=1#bar": 3}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				if r.FormValue("name") == "example1" && r.URL.String() == "/users?name=example1" {
+				q := r.URL.Query()
+				name := q.Get("name")
+				if id, ok := ids[name]; ok && r.URL.Path == "/users" && len(q) == 1 {
 					rp, err := json.Marshal([]*model.User{
 						&model.User{
 							Base: model.Base{
-								ID:   1,
-								Name: "example1",
+								ID:   id,
+								Name: name,
 							},
 						}})
 
@@ -367,6 +386,19 @@ func TestSTNS_GetGroupByName(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "special characters",
+			args: args{
+				name: "foo&id=1#bar",
+			},
+			want: &model.Group{
+				Base: model.Base{
+					ID:   3,
+					Name: "foo&id=1#bar",
+				},
+			},
+			wantErr: false,
+		},
 		{
 			name: "notfound",
 			args: args{
@@ -376,15 +408,18 @@ func TestSTNS_GetGroupByName(t *testing.T) {
 			wantErr: true,
 		},
 	}
+	ids := map[string]int{"example1": 1, "foo&id=1#bar": 3}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				if r.FormValue("name") == "example1" && r.URL.String() == "/groups?name=example1" {
+				q := r.URL.Query()
+				name := q.Get("name")
+				if id, ok := ids[name]; ok && r.URL.Path == "/groups" && len(q) == 1 {
 					rp, err := json.Marshal([]*model.Group{
 						&model.Group{
 							Base: model.Base{
-								ID:   1,
-								Name: "example1",
+								ID:   id,
+								Name: name,
 							},
 						}})
 
@@ -686,3 +721,133 @@ func TestSTNS_Verify(t *testing.T) {
 		})
 	}
 }
+
+func TestDefaultStoreAndPopChallengeCode(t *testing.T) {
+	tests := []struct {
+		name     string
+		user     string
+		existing []byte
+	}{
+		{
+			name: "ok",
+			user: "example1",
+		},
+		{
+			name:     "overwrite existing file",
+			user:     "example1",
+			existing: []byte("old"),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			t.Setenv("TMPDIR", dir)
+			p := filepath.Join(dir, tt.user)
+			if tt.existing != nil {
+				if err := os.WriteFile(p, tt.existing, 0644); err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			code := []byte("challenge")
+			if err := DefaultStoreChallengeCode(tt.user, code); err != nil {
+				t.Fatalf("DefaultStoreChallengeCode() error = %v", err)
+			}
+			fi, err := os.Stat(p)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if fi.Mode().Perm() != 0600 {
+				t.Errorf("perm = %v, want 0600", fi.Mode().Perm())
+			}
+
+			got, err := DefaultPopChallengeCode(tt.user)
+			if err != nil {
+				t.Fatalf("DefaultPopChallengeCode() error = %v", err)
+			}
+			if !bytes.Equal(got, code) {
+				t.Errorf("DefaultPopChallengeCode() = %s, want %s", got, code)
+			}
+			if _, err := os.Lstat(p); !os.IsNotExist(err) {
+				t.Errorf("file still exists: %v", err)
+			}
+		})
+	}
+}
+
+func TestDefaultChallengeCode_InvalidUser(t *testing.T) {
+	tests := []struct {
+		name string
+		user string
+	}{
+		{name: "empty", user: ""},
+		{name: "dot", user: "."},
+		{name: "dotdot", user: ".."},
+		{name: "parent", user: "../x"},
+		{name: "separator", user: "a/b"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv("TMPDIR", t.TempDir())
+			if err := DefaultStoreChallengeCode(tt.user, []byte("challenge")); err == nil {
+				t.Error("DefaultStoreChallengeCode() error = nil, want error")
+			}
+			if _, err := DefaultPopChallengeCode(tt.user); err == nil {
+				t.Error("DefaultPopChallengeCode() error = nil, want error")
+			}
+		})
+	}
+}
+
+func TestDefaultPopChallengeCode_NotRegularFile(t *testing.T) {
+	tests := []struct {
+		name  string
+		setup func(t *testing.T, p string)
+	}{
+		{
+			name: "symlink",
+			setup: func(t *testing.T, p string) {
+				target := filepath.Join(t.TempDir(), "target")
+				if err := os.WriteFile(target, []byte("secret"), 0600); err != nil {
+					t.Fatal(err)
+				}
+				if err := os.Symlink(target, p); err != nil {
+					t.Fatal(err)
+				}
+			},
+		},
+		{
+			name: "directory",
+			setup: func(t *testing.T, p string) {
+				if err := os.Mkdir(p, 0700); err != nil {
+					t.Fatal(err)
+				}
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			t.Setenv("TMPDIR", dir)
+			p := filepath.Join(dir, "example1")
+			tt.setup(t, p)
+
+			got, err := DefaultPopChallengeCode("example1")
+			if err == nil {
+				t.Errorf("DefaultPopChallengeCode() error = nil, want error")
+			}
+			if got != nil {
+				t.Errorf("DefaultPopChallengeCode() = %s, want nil", got)
+			}
+			if _, err := os.Lstat(p); err != nil {
+				t.Errorf("entry was removed: %v", err)
+			}
+			if target, err := os.Readlink(p); err == nil {
+				b, err := os.ReadFile(target)
+				if err != nil || string(b) != "secret" {
+					t.Errorf("link target changed: %s, %v", b, err)
+				}
+			}
+		})
+	}
+}