*: [security] neutralizes externally-controlled format DSN strings #37489
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note
|
|
|
Welcome @dwisiswant0! |
dwisiswant0
force-pushed
the
fix/security-120f1346-e958-49d0-b66c-0f889a469540
branch
from
03092fa
to
ee22a94
Compare
|
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/c5f296b775fb7f01364b7e67591cb02de4ea5234 |
dwisiswant0
force-pushed
the
fix/security-120f1346-e958-49d0-b66c-0f889a469540
branch
2 times, most recently
from
f25c40a
to
96719bd
Compare
dwisiswant0
force-pushed
the
fix/security-120f1346-e958-49d0-b66c-0f889a469540
branch
from
96719bd
to
c5f296b
Compare
|
/run-br-integration-tests |
|
/run-integration-br-tests |
| @@ -208,7 +209,7 @@ func (conf *Config) GetDSN(db string) string { | |||
| // https://github.com/go-sql-driver/mysql#maxallowedpacket | |||
| hostPort := net.JoinHostPort(conf.Host, strconv.Itoa(conf.Port)) | |||
| dsn := fmt.Sprintf("%s:%s@tcp(%s)/%s?collation=utf8mb4_general_ci&readTimeout=%s&writeTimeout=30s&interpolateParams=true&maxAllowedPacket=0", | |||
| conf.User, conf.Password, hostPort, db, conf.ReadTimeout) | |||
| conf.User, conf.Password, hostPort, url.QueryEscape(db), conf.ReadTimeout) | |||
There was a problem hiding this comment.
seems dbName does not support url.QueryEscape. So in order to connect to databases like test!a I prefer we don't escape it
There was a problem hiding this comment.
Hi @lance6716 - It must be escaped due to a security vulnerability matter, see the release note of this PR summary. Or do you have a better way to validate the database name?
There was a problem hiding this comment.
I guess the vulnerability lies in parsing DSN? If so, maybe we can
- use fields of
Configto set database, read timeout, etc. https://pkg.go.dev/github.com/go-sql-driver/mysql#Config - use
NewConnectorto get aConnectorfromConfighttps://pkg.go.dev/github.com/go-sql-driver/mysql#NewConnector - use
OpenDBto get aDBfromConnectorhttps://pkg.go.dev/database/sql#OpenDB
/cc @hawkingrei @gozssky
ti-chi-bot
added
the
needs-rebase
|
@dwisiswant0: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
ping @dwisiswant0 , do you need some help? |
Since I didn't think about the complexity at first, I'm going to close this PR unfortunately. |
We can fix it in our daily work. Can you verify that my comments in #37489 (comment) is safe enough? |
In the context of verifying the patch, I'd prefer to test it directly for more flexibility from my side (something related e.g. bypass, etc). So feel free to ping me (in the future associated PR or 120f1346-e958-49d0-b66c-0f889a469540) whenever it's in a ready for review state. :) |
What problem does this PR solve?
Issue Number: close 120f1346-e958-49d0-b66c-0f889a469540 (external)
Problem Summary:
TiDB uses Go MySQL Driver for connecting to MySQL servers. The Data Source Name (DSN) strings for describing database connections is not neutralized so they can escape and add new parameters to use as data source name.
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.