From 1d638ddf393c33b674f2b4f9b249a7013cab604b Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 14 Jul 2023 14:23:40 -0400 Subject: [PATCH 1/5] fix (ldap): properly escape user filters with UPN domains --- ldap/client.go | 6 ++++- ldap/client_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/ldap/client.go b/ldap/client.go index 7b51f43..ddb8c15 100644 --- a/ldap/client.go +++ b/ldap/client.go @@ -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 diff --git a/ldap/client_test.go b/ldap/client_test.go index 9e698be..d191223 100644 --- a/ldap/client_test.go +++ b/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: "foo@example.com)((((((((((((((((((((((((((((((((((((((userPrincipalName=foo", + conf: &ClientConfig{ + URLs: []string{"localhost"}, + UPNDomain: "example.com", + UserFilter: "(&({{.UserAttr}}={{.Username}})({{.UserAttr}}=admin@example.com))", + }, + want: "(&(userPrincipalName=foo@example.com\\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\\28userPrincipalName=foo@example.com)(userPrincipalName=admin@example.com))", + }, + { + 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() From 822334cca7e6209d4ed206cb36f528da2e9926cb Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 14 Jul 2023 14:25:59 -0400 Subject: [PATCH 2/5] fix (ldap): increase max tls to 1.3 --- ldap/client_test.go | 6 ++++++ ldap/config.go | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/ldap/client_test.go b/ldap/client_test.go index d191223..09424c1 100644 --- a/ldap/client_test.go +++ b/ldap/client_test.go @@ -130,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{ diff --git a/ldap/config.go b/ldap/config.go index a861694..1038f68 100644 --- a/ldap/config.go +++ b/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 From dadacf7ea25e01ca7e52f07d3c444bd562d513d9 Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 14 Jul 2023 14:27:03 -0400 Subject: [PATCH 3/5] fix (ldap): improve EscapeValue(...) --- ldap/client.go | 9 ++++++++- ldap/conn_test.go | 6 +++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/ldap/client.go b/ldap/client.go index ddb8c15..2c4227f 100644 --- a/ldap/client.go +++ b/ldap/client.go @@ -830,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] { @@ -843,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:] diff --git a/ldap/conn_test.go b/ldap/conn_test.go index 3206376..abaae1c 100644 --- a/ldap/conn_test.go +++ b/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) } } } From f30bbc56e5cea51d581987bcfc37a5af3d982b3a Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 14 Jul 2023 14:27:26 -0400 Subject: [PATCH 4/5] fix (ldap): use text template for rendering filters --- ldap/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ldap/client.go b/ldap/client.go index 2c4227f..5a6cfb4 100644 --- a/ldap/client.go +++ b/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" From f28ec041b24bf50a3ce40185f5ee31053d7cb240 Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 14 Jul 2023 15:10:29 -0400 Subject: [PATCH 5/5] chore: changelog entry --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9bd1ec..510dabd 100644 --- a/CHANGELOG.md +++ b/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