Skip to content

fix(dialect): escape driver.Valuer error messages in AppendError - #1416

Open
kyungseopk1m wants to merge 1 commit into
uptrace:masterfrom
kyungseopk1m:fix/append-error-escape
Open

fix(dialect): escape driver.Valuer error messages in AppendError#1416
kyungseopk1m wants to merge 1 commit into
uptrace:masterfrom
kyungseopk1m:fix/append-error-escape

Conversation

@kyungseopk1m

Copy link
Copy Markdown
Contributor

What

dialect.AppendError writes err.Error() into the ?!(...) marker without escaping. That message is untrusted, so a crafted error can close the marker and inject SQL.

Closes #1307.

Why

AppendError is the shared failure path for value formatting. The error text reaching it can be attacker controlled in several places:

Before this change:

type badValuer struct{}

func (badValuer) Value() (driver.Value, error) {
	return nil, errors.New("pwned'); INSERT INTO pwned VALUES (999); --")
}

formats as

?!(pwned'); INSERT INTO pwned VALUES (999); --)

The quote terminates the surrounding literal and the rest is parsed as SQL.

Fix

AppendError now takes a StringEscaper and runs the message through AppendString, which already quotes and escapes exhaustively. This follows the direction suggested in the review of #1369.

The interface is declared in dialect instead of reusing schema.QueryGen, because schema already imports dialect and taking QueryGen there creates an import cycle. It is a single method, and schema.Dialect satisfies it structurally with no changes:

type StringEscaper interface {
	AppendString(b []byte, s string) []byte
}

func AppendError(b []byte, escaper StringEscaper, err error) []byte {
	b = append(b, "?!("...)
	b = escaper.AppendString(b, err.Error())
	b = append(b, ')')
	return b
}

All 13 call sites pass gen.Dialect().

Two helpers in pgdialect had no dialect in scope and now take the escaper as a parameter:

  • appendElem is a plain []byte helper, so the escaper is threaded through its three callers.
  • appendRange is reached from Range.AppendQuery and MultiRange.AppendQuery, which already received a schema.QueryGen and discarded it as _. Naming that parameter was enough, so their signatures are unchanged.

One note on schema/dialect.go. BaseDialect.AppendString reports a NUL byte through AppendError, so it now passes itself as the escaper. The message there is a fixed constant with no quotes or NUL bytes, so the recursion terminates on the first pass. Leaving that one call site on the old raw path would have kept an unescaped route into the marker, which is what this change is trying to remove.

Breaking change

dialect.AppendError gains a parameter, and dialect.StringEscaper is new. appendElem and appendRange are unexported. The issue carries the v2 label, so the signature change looked acceptable; happy to gate it differently if you prefer.

Tests

New regression tests:

  • dialect/append_test.go covers a plain message, an embedded quote, and a marker breakout attempt.
  • schema/append_value_test.go covers appendDriverValue with a driver.Valuer returning an injecting error.
  • dialect/pgdialect/elem_test.go covers appendElem and Range.AppendQuery on the same input.

Reverting the AppendError body to the old raw append fails all four, and the failure output shows the unescaped payload. go build, go test, and go vet are clean on both the root module and dialect/pgdialect. internal/dbtest builds; its suite needs a live database and was not run.

Related

Same class as the escaping fixes in #1396, #1405, and #1406.

AppendError wrote err.Error() into the "?!(...)" marker verbatim. The
message is untrusted: it can come from a driver.Valuer, a JSON or msgpack
encoder, or a pgdialect element appender, so a crafted message could close
the marker and inject SQL.

Take a StringEscaper and run the message through AppendString, which
already quotes and escapes exhaustively. The interface is declared in
dialect rather than reusing schema.QueryGen because schema already imports
dialect; schema.Dialect satisfies it structurally.

appendElem and appendRange in pgdialect had no access to a dialect, so
they now take the escaper as a parameter. Range.AppendQuery and
MultiRange.AppendQuery already received a schema.QueryGen and discarded
it, so their signatures are unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible SQL Injection via driver.Valuer Error Messages

1 participant