Skip to content

*: [security] neutralizes externally-controlled format DSN strings#37489

Closed
dwisiswant0 wants to merge 1 commit intopingcap:masterfrom
dwisiswant0:fix/security-120f1346-e958-49d0-b66c-0f889a469540
Closed

*: [security] neutralizes externally-controlled format DSN strings#37489
dwisiswant0 wants to merge 1 commit intopingcap:masterfrom
dwisiswant0:fix/security-120f1346-e958-49d0-b66c-0f889a469540

Conversation

@dwisiswant0
Copy link

@dwisiswant0 dwisiswant0 commented Aug 31, 2022

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?

  • Escape any untrusted input to connect by DSN strings.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the issue of arbitrary file read via data source name injection (CVE-2022-3023).

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 31, 2022

[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 /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 31, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Aug 31, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 31, 2022

Welcome @dwisiswant0!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to pingcap/tidb. 😃

@dwisiswant0 dwisiswant0 force-pushed the fix/security-120f1346-e958-49d0-b66c-0f889a469540 branch from 03092fa to ee22a94 Compare August 31, 2022 03:49
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 31, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/c5f296b775fb7f01364b7e67591cb02de4ea5234

@dwisiswant0 dwisiswant0 force-pushed the fix/security-120f1346-e958-49d0-b66c-0f889a469540 branch 2 times, most recently from f25c40a to 96719bd Compare August 31, 2022 05:41
@dwisiswant0 dwisiswant0 force-pushed the fix/security-120f1346-e958-49d0-b66c-0f889a469540 branch from 96719bd to c5f296b Compare August 31, 2022 06:38
@lance6716
Copy link
Contributor

lance6716 commented Sep 1, 2022

/run-br-integration-tests

@lance6716
Copy link
Contributor

lance6716 commented Sep 1, 2022

/run-integration-br-tests

lance6716
lance6716 reviewed Sep 1, 2022
View reviewed changes
dumpling/export/config.go
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)
Copy link
Contributor

@lance6716 lance6716 Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems dbName does not support url.QueryEscape. So in order to connect to databases like test!a I prefer we don't escape it

Copy link
Author

@dwisiswant0 dwisiswant0 Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@lance6716 lance6716 Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the vulnerability lies in parsing DSN? If so, maybe we can

/cc @hawkingrei @gozssky

Copy link
Contributor

@lance6716 lance6716 Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @dwisiswant0

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2022
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 10, 2022

@dwisiswant0: PR needs rebase.

Details

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.

@lance6716
Copy link
Contributor

lance6716 commented Oct 8, 2022

ping @dwisiswant0 , do you need some help?

@dwisiswant0
Copy link
Author

dwisiswant0 commented Oct 9, 2022

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.

@dwisiswant0 dwisiswant0 closed this Oct 9, 2022
@lance6716
Copy link
Contributor

lance6716 commented Oct 9, 2022 •
edited
Loading

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?

@dwisiswant0
Copy link
Author

dwisiswant0 commented Oct 9, 2022

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. :)

@lance6716 lance6716 mentioned this pull request Oct 9, 2022
Merged
12 tasks
This was referenced Oct 19, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lance6716 lance6716 lance6716 left review comments

@sleepymole sleepymole Awaiting requested review from sleepymole

@hawkingrei hawkingrei Awaiting requested review from hawkingrei

Assignees

No one assigned

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dwisiswant0 @ti-chi-bot @sre-bot @lance6716