Data Source Name Injection in pingcap/tidb
Reported on
Dec 26th 2021
Description
TiDB Importer uses Go MySQL Driver for connecting to MySQL servers. This driver utilizes Data Source Name (DSN) strings for describing database connections with the following format:
username:password@protocol(address)/dbname?param=value
The driver has a built-in protection against LOCAL INFILE requests. To access the requested file, set the allowAllFiles parameter to true in the DSN string when connecting to the MySQL server.
Steps to Reproduce
To test the vulnerability, I need to (go) build TiDB importer command first in it.
$ git clone https://github.com/pingcap/tidb
$ cd tidb/cmd/importer
$ go build .
Next, let's set up the software which we will use as a rogue MySQL server: the Rogue-MySql-Server script by allyshka. It's a fork of the original rogue server by Gifts with additions to support modern MySQL servers.
$ wget -q https://github.com/allyshka/Rogue-MySql-Server/raw/master/roguemysql.php
Launch the roguemysql.php script and specify the path to the file you are interested in.
The last step is to specify our server address in the config.toml file. Fill the required fields and don't forget to put the ?allowAllFiles=true& string after the database name.
And finally, execute the importer command with the previous config file, you will get your file's content
$ ./importer -config config.toml
The TiDB importer has connected to the rogue MySQL server, which has requested the /etc/passwd file to be read, and the TiDB importer has transferred this file's contents to us!
Impact
This issue may lead to arbitrary file read.
Occurrences
References
The vulnerability you submitted has been successfully reproduced. Based on our assessment, we believe that this issue may lead to potential security risks and should be treated as a vulnerability. Thanks for your research work.
And we still have a few questions about this vulnerability. The first is about the scoring of the vulnerability. We use the CVSS 3.1 standard to score vulnerabilities. But the result is different from the score in the report. The scoring vector we use is as follows: CVSS:3.1/AV:L/AC:L/PR:H/UI:R/S:U/C:H/I:N/A:N (see this link for details), and the final score we get is 4.2 (6.8 in the report). If we have any misunderstanding about the impact of the vulnerability, please let us know. Another question is how to fix this vulnerability? Do you have any suggestions?
Hi,
Sorry for the delay. I don't understand why this doesn't ping me the notification.
Regarding how to mitigate this issue, it's actually quite simple. Because this vulnerability inherits from improper input validation, so any untrusted input must be validated before being concatenated into a DSN string.
In this case, validating the database name with the pattern as ^[0-9a-zA-Z$_]+$ should suffice, or practically use url.QueryEscape.
@maintainter - Pretty sure that you should be able to confirm this vulnerability by acknowledging it.
@admin - I don't understand that the maintainer has replied to this report, but there is still a "follow-up" status. Can you please inform the @maintainer(s)?
The maintainer will get an e-mail ping because of the messages you and I am creating here 👍
Hi @Dwi, thanks for your reply. Can you update the CVSS score and select a more accurate vulnerability type? This issue doesn't seem to be a SQL injection.
Hi @maintainer, sorted as CWE-134.
I list occurrences based on cases that have been tested and I'm afraid it will be a relic of future discoveries, so I believe that vulnerable codes are not the only ones I listed - see https://cs.github.com/?scopeName=All+repos&scope=&q=repo%3Apingcap%2Ftidb+%22%40tcp%22. Thus, I changed the availability score to low.
Let me know what you think.
Hi @Dwi, thanks for your update. Since a successful exploit can only result in arbitrary file read, in my opinion, this issue does not affect the availability of the server. (I find the definition of availability in section 2.3.3 in this document: https://www.first.org/cvss/v3.1/specification-document)
Sorted, if you think you've reviewed the presence of similar occurrences.
@Dwi, thanks for your quick reply, I've confirmed this issue.
Hi there, I just submitted a patch for this issue.
Before
[mysql] 2022/08/30 14:09:15 packets.go:37: unexpected EOF
[2022/08/30 14:09:15.770 +07:00] [FATAL] [main.go:80] ["invalid connection"] [stack="main.main\n\t/tmp/tidb/cmd/importer/main.go:80\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"]
After (03092fa481e94d3c0f626f5953c92f344dfbd553)
[mysql] 2022/08/30 14:09:38 packets.go:37: unexpected EOF
[2022/08/30 14:09:38.530 +07:00] [FATAL] [main.go:80] ["local file '/etc/passwd' is not registered"] [stack="main.main\n\t/tmp/tidb/cmd/importer/main.go:80\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"]
That's great. Can you send a Pull Request? So that we can review and merge this fix into master.
Opened at #37489. Also, can you please test them by any chance? :)
Good job, I will contact the feature owner to review these fixes.
Hi there, seems like the patch d0376379d615cc8f263a0b17c031ce403c8dcbfb already rolled out. Can you please give an update on this? Thanks.
Hi @dwisiswant0, I'm just new to here, not sure how to choose the right action. Currently the fixing patch is included in the version v6.4.0, but v6.4.0 is planned to be release some weeks later. Maybe we should click "Publish" when v6.4.0 is released?
Oh, I thought it's been released. If so, let's schedule the advisory to be released 24 hours after the fix is released (for people to start to upgrade, mainly to verify the new release hasn't caused any other problems). Thanks, @lance6716.
Hi there, since this was rolled out in version 6.4.0, I believe that there are release notes left behind for it. It's no less important to notify tidb-server users to be aware of that, can you please update the release changelog for that version? I initiated it in this #37489 PR (check the release note section) and feel free to adapt.
Changelog aside, can we agree to disclose this now? Since it's been released 4 days ago, which might help boost the awareness I said.
Thanks. (: