From b4337e257a2927ac7211e45bbd52c700ad689997 Mon Sep 17 00:00:00 2001 From: Robert Chiniquy Date: Wed, 20 May 2026 12:20:35 -0700 Subject: [PATCH 1/3] fix: RevokePermissionOnDatabase issues REVOKE, not GRANT RevokePermissionOnDatabase was building a GRANT...TO statement instead of REVOKE...FROM, so calling the revoke path re-granted the permission instead of removing it. The neighbouring GrantPermissionOnDatabase already builds the correct GRANT statement, so this was an inconsistency, not a missing helper. While here, drop the trailing positional args (fullPermission, db, user) from the ExecContext call -- the query string is built with fmt.Sprintf and contains no ? placeholders, so the args were inert. How found: occult-go-analysis c1 profile, sql_query_via_sprintf detector flagged the function for review. The detector finding itself is a false positive (input is validated against [\]"';]), but reading the function in detail surfaced the GRANT-vs-REVOKE swap. Origin: https://github.com/conductorone/occult-go-analysis --- pkg/mssqldb/databases.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/mssqldb/databases.go b/pkg/mssqldb/databases.go index c65e63e1..7383eaed 100644 --- a/pkg/mssqldb/databases.go +++ b/pkg/mssqldb/databases.go @@ -144,13 +144,13 @@ func (c *Client) RevokePermissionOnDatabase(ctx context.Context, permission, db, } command := fmt.Sprintf( - "GRANT %s ON DATABASE::[%s] TO [%s];", + "REVOKE %s ON DATABASE::[%s] FROM [%s];", fullPermission, db, user, ) - _, err := c.db.ExecContext(ctx, command, fullPermission, db, user) + _, err := c.db.ExecContext(ctx, command) if err != nil { return err } From c64c590b4f55bf2f559c4f347cf2b8a9694b567a Mon Sep 17 00:00:00 2001 From: Robert Chiniquy Date: Wed, 20 May 2026 12:22:21 -0700 Subject: [PATCH 2/3] fix: escape ' in passwords passed to CREATE LOGIN CreateLogin builds CREATE LOGIN ... WITH PASSWORD = '%s' via fmt.Sprintf, interpolating the caller-supplied password into a single-quoted SQL Server string literal without escaping. A password containing ' breaks the literal -- syntactically invalid for benign input, a SQL injection vector if any caller passes user-controlled text through. SQL Server escapes ' inside string literals by doubling it (''), so this replaces single quotes with doubled single quotes before interpolation. The stored password byte-string still contains ' as authored. How found: occult-go-analysis c1 profile, sql_query_via_sprintf detector flagged CreateLogin. Other sprintf sites in this package validate inputs against ["';] before interpolation, but CreateLogin did not, leaving the password path exposed. Origin: https://github.com/conductorone/occult-go-analysis --- pkg/mssqldb/users.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/mssqldb/users.go b/pkg/mssqldb/users.go index 95862684..8664758d 100644 --- a/pkg/mssqldb/users.go +++ b/pkg/mssqldb/users.go @@ -399,7 +399,12 @@ func (c *Client) CreateLogin(ctx context.Context, loginType LoginType, username, // For SQL Server authentication, only username and password are used loginName := fmt.Sprintf("[%s]", username) l.Debug("creating SQL login", zap.String("login", loginName)) - query = fmt.Sprintf("CREATE LOGIN %s WITH PASSWORD = '%s';", loginName, password) + // SQL Server string literals escape ' by doubling it. Without this, + // a password containing ' breaks out of the literal -- a SQL + // injection vector and at minimum a syntax error for legitimate + // users whose passwords contain '. + escapedPassword := strings.ReplaceAll(password, "'", "''") + query = fmt.Sprintf("CREATE LOGIN %s WITH PASSWORD = '%s';", loginName, escapedPassword) case LoginTypeAzureAD, LoginTypeEntraID: // Azure AD and Entra ID use external provider loginName := fmt.Sprintf("[%s]", username) From 2cf2612fb435b5ce55427ed81078e12e72f9cd33 Mon Sep 17 00:00:00 2001 From: Robert Chiniquy Date: Wed, 20 May 2026 13:14:21 -0700 Subject: [PATCH 3/3] fix: address bot suggestions on #38 Two additional fixes flagged by the bot review: - pkg/mssqldb/databases.go: drop the inert positional args from GrantPermissionOnDatabase's ExecContext call. The command string is built via fmt.Sprintf with no ? placeholders, so the args were never used. Mirrors the cleanup the Revoke fix already applied to its sibling. - pkg/mssqldb/users.go: reject [\]"'; in the username passed to CreateLogin. The function interpolates username into bracket- quoted SQL Server identifiers; a username containing ] would escape the brackets the same way an unescaped ' in the password would. Matches the validation pattern used by the other identifier-interpolating functions in this package. --- pkg/mssqldb/databases.go | 2 +- pkg/mssqldb/users.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/mssqldb/databases.go b/pkg/mssqldb/databases.go index 7383eaed..6504ba6b 100644 --- a/pkg/mssqldb/databases.go +++ b/pkg/mssqldb/databases.go @@ -117,7 +117,7 @@ func (c *Client) GrantPermissionOnDatabase(ctx context.Context, permission, db, l.Debug("SQL QUERY", zap.String("q", command)) - _, err := c.db.ExecContext(ctx, command, permission, db, user) + _, err := c.db.ExecContext(ctx, command) if err != nil { return err } diff --git a/pkg/mssqldb/users.go b/pkg/mssqldb/users.go index 8664758d..b30bb0d3 100644 --- a/pkg/mssqldb/users.go +++ b/pkg/mssqldb/users.go @@ -386,6 +386,14 @@ const ( func (c *Client) CreateLogin(ctx context.Context, loginType LoginType, username, password string) error { l := ctxzap.Extract(ctx) + // Username is interpolated into bracket-quoted SQL Server identifiers + // below. Reject characters that would let the value break out of the + // brackets (this is the same guard the rest of this package uses on + // identifier-interpolated inputs). + if strings.ContainsAny(username, "[]\"';") { + return fmt.Errorf("invalid characters in username") + } + var query string switch loginType { case LoginTypeWindows: