Address a set of LDAP issues.

CHANGELOG.md

@@ -1,6 +1,14 @@
 # cap CHANGELOG
 
 Canonical reference for changes, improvements, and bugfixes for cap.
+## 0.3.2
+
+### Bug fixes:
+* Address a set of LDAP issues ([**PR**](https://github.com/hashicorp/cap/pull/77)):
+  * Properly escape user filters when using UPN domains
+  * Increase max tls to 1.3
+  * Improve `EscapeValue(...)`
+  * Use text template for rendering filters
 
 ## 0.3.1
 

ldap/client.go

@@ -10,11 +10,11 @@ import (
 	"crypto/x509"
 	"encoding/binary"
 	"fmt"
-	"html/template"
 	"math"
 	"net"
 	"net/url"
 	"strings"
+	"text/template"
 	"time"
 
 	"github.com/go-ldap/ldap/v3"
@@ -753,7 +753,11 @@ func (c *Client) renderUserSearchFilter(username string) (string, error) {
 	}
 	if c.conf.UPNDomain != "" {
 		context.UserAttr = "userPrincipalName"
-		context.Username = fmt.Sprintf("%s@%s", EscapeValue(username), c.conf.UPNDomain)
+		// Intentionally, calling EscapeFilter(...) (vs EscapeValue) since the
+		// username is being injected into a search filter.
+		// As an untrusted string, the username must be escaped according to RFC
+		// 4515, in order to prevent attackers from injecting characters that could modify the filter
+		context.Username = fmt.Sprintf("%s@%s", EscapeFilter(username), c.conf.UPNDomain)
 	}
 
 	var renderedFilter bytes.Buffer
@@ -826,10 +830,14 @@ func EscapeValue(input string) string {
 	// - trailing space
 	// - special characters '"', '+', ',', ';', '<', '>', '\\'
 	// - null
-	for i := 0; i < len(input); i++ {
+	inputLen := len(input)
+	for i := 0; i < inputLen; i++ {
 		escaped := false
 		if input[i] == '\\' {
 			i++
+			if i > inputLen-1 {
+				break
+			}
 			escaped = true
 		}
 		switch input[i] {
@@ -839,6 +847,9 @@ func EscapeValue(input string) string {
 				i++
 			}
 			continue
+		case '\000':
+			input = input[0:i] + `\00` + input[i+1:]
+			continue
 		}
 		if escaped {
 			input = input[0:i] + "\\" + input[i:]

ldap/client_test.go

@@ -16,6 +16,72 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestClient_renderUserSearchFilter(t *testing.T) {
+	t.Parallel()
+	// just ensure that rendered filters are properly escaped
+	testCtx := context.Background()
+	tests := []struct {
+		name        string
+		conf        *ClientConfig
+		userName    string
+		want        string
+		errContains string
+	}{
+		{
+			name:     "valid-default",
+			userName: "alice",
+			conf: &ClientConfig{
+				URLs: []string{"localhost"},
+			},
+			want: "(cn=alice)",
+		},
+		{
+			name:     "escaped-malicious-filter",
+			userName: "[email protected])((((((((((((((((((((((((((((((((((((((userPrincipalName=foo",
+			conf: &ClientConfig{
+				URLs:       []string{"localhost"},
+				UPNDomain:  "example.com",
+				UserFilter: "(&({{.UserAttr}}={{.Username}})({{.UserAttr}}[email protected]))",
+			},
+			want: "(&([email protected]\\29\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\28\\[email protected])([email protected]))",
+		},
+		{
+			name:     "bad-filter-unclosed-action",
+			userName: "alice",
+			conf: &ClientConfig{
+				URLs:       []string{"localhost"},
+				UserFilter: "hello{{range",
+			},
+			errContains: "search failed due to template compilation error",
+		},
+		{
+			name: "missing-username",
+			conf: &ClientConfig{
+				URLs: []string{"localhost"},
+			},
+			errContains: "missing username",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			assert, require := assert.New(t), require.New(t)
+			c, err := NewClient(testCtx, tc.conf)
+			require.NoError(err)
+
+			f, err := c.renderUserSearchFilter(tc.userName)
+			if tc.errContains != "" {
+				require.Error(err)
+				assert.ErrorContains(err, tc.errContains)
+				return
+			}
+			require.NoError(err)
+			assert.NotEmpty(f)
+			assert.Equal(tc.want, f)
+		})
+	}
+
+}
+
 func TestClient_NewClient(t *testing.T) {
 	t.Parallel()
 	testCtx := context.Background()
@@ -64,6 +130,12 @@ func TestClient_NewClient(t *testing.T) {
 			wantErrIs:       ErrInvalidParameter,
 			wantErrContains: "invalid 'tls_min_version' in config",
 		},
+		{
+			name: "valid-tls-max",
+			conf: &ClientConfig{
+				TLSMaxVersion: "tls13",
+			},
+		},
 		{
 			name: "invalid-tls-max",
 			conf: &ClientConfig{

ldap/config.go

@@ -35,7 +35,7 @@ const (
 	DefaultTLSMinVersion = "tls12"
 
 	// DefaultTLSMaxVersion for the ClientConfig.TLSMaxVersion
-	DefaultTLSMaxVersion = "tls12"
+	DefaultTLSMaxVersion = "tls13"
 
 	// DefaultOpenLDAPUserPasswordAttribute defines the attribute name for the
 	// openLDAP default password attribute which will always be excluded

ldap/conn_test.go

@@ -13,12 +13,16 @@ func Test_EscapeValue(t *testing.T) {
 		"test\\hello": "test\\\\hello",
 		"  test  ":    "\\  test \\ ",
 		"":            "",
+		`\`:           `\`,
+		"golang\000":  `golang\00`,
+		"go\000lang":  `go\00lang`,
+		"\000":        `\00`,
 	}
 
 	for test, answer := range testcases {
 		res := EscapeValue(test)
 		if res != answer {
-			t.Errorf("Failed to escape %s: %s != %s\n", test, res, answer)
+			t.Errorf("Failed to escape %q: %q != %q\n", test, res, answer)
 		}
 	}
 }