security(cdc): fix some security problems #3700

Ehco1996 · 2021-12-02T07:09:36Z

What problem does this PR solve?

fix https://github.com/pingcap/ticdc/issues/3696

preview the fixed results here

What is changed and how it works?

Check List

Tests

Unit test
Integration test

Code changes

Side effects

Related changes

Release note

 `None`.

ti-chi-bot · 2021-12-02T07:09:38Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

asddongmen
okJiang

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.

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

Ehco1996 · 2021-12-02T07:10:50Z

/run-all-tests

Ehco1996 · 2021-12-02T07:41:36Z

/run-integration-test
/run-kafka-integration-test

codecov-commenter · 2021-12-03T01:46:47Z

Codecov Report

Merging #3700 (a142ea3) into master (7702989) will increase coverage by 0.5424%.
The diff coverage is 80.0630%.

Flag	Coverage Δ
cdc	`58.7564% <80.1105%> (+1.1368%)`	⬆️
dm	`56.3627% <50.0000%> (-0.0023%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #3700        +/-   ##
================================================
+ Coverage   56.9451%   57.4876%   +0.5424%     
================================================
  Files           455        460         +5     
  Lines         54110      54824       +714     
================================================
+ Hits          30813      31517       +704     
+ Misses        20085      20041        -44     
- Partials       3212       3266        +54

Ehco1996 · 2021-12-03T02:35:26Z

/run-all-tests

Ehco1996 · 2021-12-03T02:48:31Z

/run-integration-test

Ehco1996 · 2021-12-03T05:54:43Z

could you please take another look for this pr ? thanks 🧡🧡🧡 @hi-rustin @asddongmen

lance6716 · 2021-12-03T07:50:23Z

cdc/sink/producer/kafka/config.go

@@ -72,7 +72,7 @@ func (c *Config) CompleteByOpts(sinkURI *url.URL, replicaConfig *config.ReplicaC
 	params := sinkURI.Query()
 	s := params.Get("partition-num")
 	if s != "" {
-		a, err := strconv.Atoi(s)


why this is a security problem 😂

becasue in L79 c.PartitionNum = int32(a) try convert int to int32 😂

okJiang · 2021-12-03T09:13:42Z

tests/integration_tests/bank/case.go

 	var line dataRow
-	row := db.QueryRow(query)
+	row := db.QueryRow(query, tableName)


What's the difference in this change?

the old one may casue sql jnjection see more here

see details https://cwe.mitre.org/data/definitions/89.html

Rustin170506

rest LGTM

cdc/sink/producer/kafka/config.go

Rustin170506 · 2021-12-03T13:52:03Z

cdc/sorter/unified/file_backend.go

@@ -65,7 +65,7 @@ func newFileBackEnd(fileName string, serde encoding.SerializerDeserializer) (*fi
 }

 func (f *fileBackEnd) reader() (backEndReader, error) {
-	fd, err := os.OpenFile(f.fileName, os.O_RDWR, 0o644)
+	fd, err := os.OpenFile(f.fileName, os.O_RDWR, 0o600)


/cc @liuzix

Could you please take a look? Thanks! 💚 💙 💜 💛 ❤️

asddongmen · 2021-12-06T03:26:05Z

/run-integration-tests

amyangfei · 2021-12-06T03:47:16Z

/merge

ti-chi-bot · 2021-12-06T03:47:19Z

This pull request has been accepted and is ready to merge.

Commit hash: a142ea3

Ehco1996 · 2021-12-06T05:37:04Z

/run-dm-integration-tests

enable test

2a47875

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 2, 2021

fix test

8fef8e5

Ehco1996 mentioned this pull request Dec 2, 2021

ticdc: add gosec PingCAP-QE/devops-config#64

Merged

fix more

9926882

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2021

Ehco1996 added 2 commits December 3, 2021 10:31

Merge branch 'master' into cdc-gosec

2943f66

fix

b5072f8

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2021

Ehco1996 added the area/ticdc Issues or PRs related to TiCDC. label Dec 3, 2021

Ehco1996 marked this pull request as ready for review December 3, 2021 05:53

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2021

Ehco1996 added the status/ptal Could you please take a look? label Dec 3, 2021

lance6716 reviewed Dec 3, 2021

View reviewed changes

okJiang reviewed Dec 3, 2021

View reviewed changes

okJiang approved these changes Dec 3, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 3, 2021

Rustin170506 reviewed Dec 3, 2021

View reviewed changes

ti-chi-bot requested a review from liuzix December 3, 2021 13:52

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2021

Merge branch 'master' into cdc-gosec

153ad19

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2021

Merge branch 'master' into cdc-gosec

a142ea3

asddongmen self-requested a review December 6, 2021 03:02

asddongmen approved these changes Dec 6, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 6, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 6, 2021

Merge branch 'master' into cdc-gosec

f35f8a7

ti-chi-bot merged commit eed6f9b into pingcap:master Dec 6, 2021

okJiang pushed a commit to okJiang/tiflow that referenced this pull request Dec 8, 2021

security(cdc): fix some security problems (pingcap#3700)

ea3bd14

Ehco1996 deleted the cdc-gosec branch December 20, 2021 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security(cdc): fix some security problems #3700

security(cdc): fix some security problems #3700

Ehco1996 commented Dec 2, 2021 •

edited

Loading

ti-chi-bot commented Dec 2, 2021 •

edited

Loading

Ehco1996 commented Dec 2, 2021

Ehco1996 commented Dec 2, 2021

codecov-commenter commented Dec 3, 2021 •

edited

Loading

Ehco1996 commented Dec 3, 2021

Ehco1996 commented Dec 3, 2021

Ehco1996 commented Dec 3, 2021

lance6716 Dec 3, 2021

Ehco1996 Dec 3, 2021

okJiang Dec 3, 2021

Ehco1996 Dec 3, 2021

okJiang Dec 3, 2021

Rustin170506 left a comment

Rustin170506 Dec 3, 2021

asddongmen commented Dec 6, 2021

amyangfei commented Dec 6, 2021

ti-chi-bot commented Dec 6, 2021

Ehco1996 commented Dec 6, 2021

security(cdc): fix some security problems #3700

security(cdc): fix some security problems #3700

Conversation

Ehco1996 commented Dec 2, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 2, 2021 • edited Loading

Ehco1996 commented Dec 2, 2021

Ehco1996 commented Dec 2, 2021

codecov-commenter commented Dec 3, 2021 • edited Loading

Codecov Report

Ehco1996 commented Dec 3, 2021

Ehco1996 commented Dec 3, 2021

Ehco1996 commented Dec 3, 2021

lance6716 Dec 3, 2021

Choose a reason for hiding this comment

Ehco1996 Dec 3, 2021

Choose a reason for hiding this comment

okJiang Dec 3, 2021

Choose a reason for hiding this comment

Ehco1996 Dec 3, 2021

Choose a reason for hiding this comment

okJiang Dec 3, 2021

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Dec 3, 2021

Choose a reason for hiding this comment

asddongmen commented Dec 6, 2021

amyangfei commented Dec 6, 2021

ti-chi-bot commented Dec 6, 2021

Ehco1996 commented Dec 6, 2021

Ehco1996 commented Dec 2, 2021 •

edited

Loading

ti-chi-bot commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 3, 2021 •

edited

Loading