Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: database/sql: provide google/go-safeweb/safesql like api #64314

Open
empijei opened this issue Nov 21, 2023 · 4 comments
Open

proposal: database/sql: provide google/go-safeweb/safesql like api #64314

empijei opened this issue Nov 21, 2023 · 4 comments
Labels
Milestone

Comments

@empijei
Copy link
Contributor

empijei commented Nov 21, 2023

Currently the database/sql package does very little to prevent accidental SQL injection.

Both of these calls are valid and accepted:

db.QueryRow("SELECT id from db where name = ?", name) // Safe
db.QueryRow("SELECT id from db where name = '"+name+"'") // SQL injection vulnerable

There are ways to make sure that only constants or trusted strings end up being used as query sources, like the safesql package does.

Consider that the safesql package is a very tiny wrapper around the standard package, most code looks like this:

func (c Conn) ExecContext(ctx context.Context, query TrustedSQLString, args ...interface{}) (Result, error) {
	return c.c.ExecContext(ctx, query.s, args...)
}

And the only real addition is the trusted string manipulation and construction library provided by the fifty lines of code in safesql.go.

Maintaining this package is trivial as it only changes when the sql API package changes, and it contains very little logic.

The advantage is that this makes it significantly harder for programmers to accidentally pass user input as query source, without almost any hinderance on code writing, and migration is trivial (see more in the doc).

Code like the following is trivial to migrate from sql to safesql:
db.Query("SELECT ...", args...)
The only change required would be to promote the string literal to a trusted string:
db.Query(safesql.New("SELECT ..."), args...)

This approach is the one used and suggested by Google (the safesql package is itself owned by Google) and it has been talked about for a while now (2016 talk by Christoph Kern). It's a very battle tested and effective way to prevent code injection.

The standard library has packages like html/template that already follow similar approaches, so this is not new to the Go stdlib.

Sadly, this is not known and most people end up using the barebone, unprotected SQL package.

My proposal would be to provide a v2 for database/sql, a database/safesql or even just a separate package outside of the stdlib that the doc for database/sql points to.

I think having users default to the less safe option is a suboptimal situation, we should at least provide a warning of the issue and point users to a solution.

@gopherbot gopherbot added this to the Proposal milestone Nov 21, 2023
@empijei
Copy link
Contributor Author

empijei commented Nov 21, 2023

@seankhliao
Copy link
Member

see also #22697 for general sql v2 wishlist

@rsc
Copy link
Contributor

rsc commented Dec 12, 2023

The standard library has packages like html/template that already follow similar approaches

I'm not sure what "similar" means here in the context of html/template. I don't believe it has APIs that insist on TrustedFoo.

@empijei
Copy link
Contributor Author

empijei commented Dec 13, 2023

What I meant is that html/template doesn't just take string. Strings are by default escaped and treated as untrusted, for a string to be rendered verbatim in an action it requires to be promoted to HTML, for example.

But it does have some issues with accepting bare strings as template sources, in fact potentially accepting userg-generated content as template source, but that's a much more complex issue that would require a big change to be addressed (e.g. going the safehtml way).

Consider that an acceptable middle-ground would be to use a type alias and rely on vet to make sure that the type is only ever constructed from string manipulation of the safe type or from constants. This would not require an API change but it would surface issues when people vet their code (which, hopefully, happens early in the dev cycle).

@seankhliao seankhliao changed the title proposal: database/sql: Provide a safer API proposal: database/sql: provide google/go-safeweb/safesql like api Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants