From ff7a08c3647b5a65f21953fecc4b48cbf2d73672 Mon Sep 17 00:00:00 2001 From: Gary Frederick Date: Thu, 27 Jan 2022 15:20:13 -0800 Subject: [PATCH] Remove fmt strings and replace with inline queries (#13799) * removed fmt strings and replaced with inline SQL | added unit tests * changelog++ --- changelog/13799.txt | 3 ++ plugins/database/mssql/mssql.go | 36 ++++++++------- plugins/database/mssql/mssql_test.go | 65 +++++++++++++++++++++++----- 3 files changed, 77 insertions(+), 27 deletions(-) create mode 100644 changelog/13799.txt diff --git a/changelog/13799.txt b/changelog/13799.txt new file mode 100644 index 000000000..27e15d8a3 --- /dev/null +++ b/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. +``` \ No newline at end of file diff --git a/plugins/database/mssql/mssql.go b/plugins/database/mssql/mssql.go index 094fa7dbb..ef627fb04 100644 --- a/plugins/database/mssql/mssql.go +++ b/plugins/database/mssql/mssql.go @@ -216,24 +216,32 @@ func (m *MSSQL) revokeUserDefault(ctx context.Context, username string) error { // Check if DB is contained if m.containedDB { - revokeStmt, err := db.PrepareContext(ctx, fmt.Sprintf("DROP USER IF EXISTS [%s]", username)) + revokeQuery := + `DECLARE @stmt nvarchar(max); + SET @stmt = 'DROP USER IF EXISTS ' + QuoteName(@username); + EXEC(@stmt);` + revokeStmt, err := db.PrepareContext(ctx, revokeQuery) if err != nil { return err } defer revokeStmt.Close() - if _, err := revokeStmt.ExecContext(ctx); err != nil { + if _, err := revokeStmt.ExecContext(ctx, sql.Named("username", username)); err != nil { return err } return nil } // First disable server login - disableStmt, err := db.PrepareContext(ctx, fmt.Sprintf("ALTER LOGIN [%s] DISABLE;", username)) - if err != nil { + 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 } @@ -311,12 +319,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 } @@ -413,14 +421,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}}' diff --git a/plugins/database/mssql/mssql_test.go b/plugins/database/mssql/mssql_test.go index 42f15ca19..67d7e30e0 100644 --- a/plugins/database/mssql/mssql_test.go +++ b/plugins/database/mssql/mssql_test.go @@ -14,6 +14,7 @@ import ( "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) { @@ -262,7 +263,10 @@ func TestUpdateUser_password(t *testing.T) { dbtesting.AssertInitializeCircleCiTest(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) @@ -326,7 +330,10 @@ func TestDeleteUser(t *testing.T) { dbtesting.AssertInitializeCircleCiTest(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) @@ -350,6 +357,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.AssertInitializeCircleCiTest(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) @@ -378,18 +423,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() @@ -400,12 +445,13 @@ 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 = ` @@ -413,11 +459,6 @@ 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}}'; `