Remove fmt strings and replace with inline queries (#13799) (#13821)

* removed fmt strings and replaced with inline SQL | added unit tests

* changelog++

Co-authored-by: Gary Frederick <gary.frederick@hashicorp.com>

changelog/13799.txt

@@ -0,0 +1,3 @@
+```release-note:security
+database/mssql: Removed string interpolation on internal queries and replaced them with inline queries using named parameters.
+```

plugins/database/mssql/mssql.go

@@ -202,12 +202,16 @@ func (m *MSSQL) revokeUserDefault(ctx context.Context, username string) error {
 	}
 
 	// First disable server login
-	disableStmt, err := db.PrepareContext(ctx, fmt.Sprintf("ALTER LOGIN [%s] DISABLE;", username))
+	disableQuery :=
+		`DECLARE @stmt nvarchar(max);
+		SET @stmt = 'ALTER LOGIN ' + QuoteName(@username) + ' DISABLE';
+		EXEC(@stmt);`
+	disableStmt, err := db.PrepareContext(ctx, disableQuery)
 	if err != nil {
 		return err
 	}
 	defer disableStmt.Close()
-	if _, err := disableStmt.ExecContext(ctx); err != nil {
+	if _, err := disableStmt.ExecContext(ctx, sql.Named("username", username)); err != nil {
 		return err
 	}
 
@@ -285,12 +289,12 @@ func (m *MSSQL) revokeUserDefault(ctx context.Context, username string) error {
 	}
 
 	// Drop this login
-	stmt, err = db.PrepareContext(ctx, fmt.Sprintf(dropLoginSQL, username, username))
+	stmt, err = db.PrepareContext(ctx, dropLoginSQL)
 	if err != nil {
 		return err
 	}
 	defer stmt.Close()
-	if _, err := stmt.ExecContext(ctx); err != nil {
+	if _, err := stmt.ExecContext(ctx, sql.Named("username", username)); err != nil {
 		return err
 	}
 
@@ -383,14 +387,12 @@ END
 `
 
 const dropLoginSQL = `
-IF EXISTS
-  (SELECT name
-   FROM master.sys.server_principals
-   WHERE name = N'%s')
-BEGIN
-  DROP LOGIN [%s]
-END
-`
+DECLARE @stmt nvarchar(max)
+SET @stmt = 'IF EXISTS (SELECT name FROM [master].[sys].[server_principals] WHERE [name] = ' + QuoteName(@username, '''') + ') ' +
+	'BEGIN ' +
+		'DROP LOGIN ' + QuoteName(@username) + ' ' +
+	'END'
+EXEC (@stmt)`
 
 const alterLoginSQL = `
 ALTER LOGIN [{{username}}] WITH PASSWORD = '{{password}}' 

plugins/database/mssql/mssql_test.go

@@ -14,6 +14,7 @@ import (
 	dbplugin "github.com/hashicorp/vault/sdk/database/dbplugin/v5"
 	dbtesting "github.com/hashicorp/vault/sdk/database/dbplugin/v5/testing"
 	"github.com/hashicorp/vault/sdk/helper/dbtxn"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestInitialize(t *testing.T) {
@@ -244,7 +245,10 @@ func TestUpdateUser_password(t *testing.T) {
 			dbtesting.AssertInitialize(t, db, initReq)
 			defer dbtesting.AssertClose(t, db)
 
-			createTestMSSQLUser(t, connURL, dbUser, initPassword, testMSSQLLogin)
+			err := createTestMSSQLUser(connURL, dbUser, initPassword, testMSSQLLogin)
+			if err != nil {
+				t.Fatalf("Failed to create user: %s", err)
+			}
 
 			assertCredsExist(t, connURL, dbUser, initPassword)
 
@@ -287,7 +291,10 @@ func TestDeleteUser(t *testing.T) {
 	dbtesting.AssertInitialize(t, db, initReq)
 	defer dbtesting.AssertClose(t, db)
 
-	createTestMSSQLUser(t, connURL, dbUser, initPassword, testMSSQLLogin)
+	err := createTestMSSQLUser(connURL, dbUser, initPassword, testMSSQLLogin)
+	if err != nil {
+		t.Fatalf("Failed to create user: %s", err)
+	}
 
 	assertCredsExist(t, connURL, dbUser, initPassword)
 
@@ -311,6 +318,44 @@ func TestDeleteUser(t *testing.T) {
 	assertCredsDoNotExist(t, connURL, dbUser, initPassword)
 }
 
+func TestSQLSanitization(t *testing.T) {
+	cleanup, connURL := mssqlhelper.PrepareMSSQLTestContainer(t)
+	defer cleanup()
+
+	injectionString := "vaultuser]"
+	dbUser := "vaultuser"
+	initPassword := "p4$sw0rd"
+
+	initReq := dbplugin.InitializeRequest{
+		Config: map[string]interface{}{
+			"connection_url": connURL,
+		},
+		VerifyConnection: true,
+	}
+
+	db := new()
+
+	dbtesting.AssertInitialize(t, db, initReq)
+	defer dbtesting.AssertClose(t, db)
+
+	err := createTestMSSQLUser(connURL, dbUser, initPassword, testMSSQLLogin)
+	if err != nil {
+		t.Fatalf("Failed to create user: %s", err)
+	}
+
+	assertCredsExist(t, connURL, dbUser, initPassword)
+
+	deleteReq := dbplugin.DeleteUserRequest{
+		Username: injectionString,
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+	_, err = db.DeleteUser(ctx, deleteReq)
+
+	assert.EqualError(t, err, "mssql: Cannot alter the login 'vaultuser]', because it does not exist or you do not have permission.")
+}
+
 func assertCredsExist(t testing.TB, connURL, username, password string) {
 	t.Helper()
 	err := testCredsExist(connURL, username, password)
@@ -339,18 +384,18 @@ func testCredsExist(connURL, username, password string) error {
 	return db.Ping()
 }
 
-func createTestMSSQLUser(t *testing.T, connURL string, username, password, query string) {
+func createTestMSSQLUser(connURL string, username, password, query string) error {
 	db, err := sql.Open("mssql", connURL)
 	defer db.Close()
 	if err != nil {
-		t.Fatal(err)
+		return err
 	}
 
 	// Start a transaction
 	ctx := context.Background()
 	tx, err := db.BeginTx(ctx, nil)
 	if err != nil {
-		t.Fatal(err)
+		return err
 	}
 	defer func() {
 		_ = tx.Rollback()
@@ -361,24 +406,20 @@ func createTestMSSQLUser(t *testing.T, connURL string, username, password, query
 		"password": password,
 	}
 	if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil {
-		t.Fatal(err)
+		return err
 	}
 	// Commit the transaction
 	if err := tx.Commit(); err != nil {
-		t.Fatal(err)
+		return err
 	}
+	return nil
 }
 
 const testMSSQLRole = `
 CREATE LOGIN [{{name}}] WITH PASSWORD = '{{password}}';
 CREATE USER [{{name}}] FOR LOGIN [{{name}}];
 GRANT SELECT, INSERT, UPDATE, DELETE ON SCHEMA::dbo TO [{{name}}];`
 
-const testMSSQLDrop = `
-DROP USER [{{name}}];
-DROP LOGIN [{{name}}];
-`
-
 const testMSSQLLogin = `
 CREATE LOGIN [{{name}}] WITH PASSWORD = '{{password}}';
 `