Skip to content

auth: fix CheckScrambledPassword() panic for invalid input#1197

Merged
kennytm merged 3 commits intopingcap:masterfrom
tiancaiamao:fix-panic
Mar 24, 2021
Merged

auth: fix CheckScrambledPassword() panic for invalid input#1197
kennytm merged 3 commits intopingcap:masterfrom
tiancaiamao:fix-panic

Conversation

@tiancaiamao
Copy link
Copy Markdown
Collaborator

@tiancaiamao tiancaiamao commented Mar 24, 2021

What problem does this PR solve?

image

What is changed and how it works?

The input data may be invalid, but calling CheckScrambledPassword() on malformed data should not make TiDB server panic.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@tiancaiamao
Copy link
Copy Markdown
Collaborator Author

tiancaiamao commented Mar 24, 2021

The malformed data maybe related to pingcap/tidb#19603, maybe we are not resolving the protocol correctly, but I have no clue.

PTAL @djshow832 @morgo

@kennytm
Copy link
Copy Markdown
Contributor

kennytm commented Mar 24, 2021

/lgtm

@ti-srebot ti-srebot added the status/LGT1 LGT1 label Mar 24, 2021
djshow832
djshow832 reviewed Mar 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

Add some test cases?

@tiancaiamao
Copy link
Copy Markdown
Collaborator Author

tiancaiamao commented Mar 24, 2021

Add some test cases?

https://github.com/pingcap/parser/pull/1197/files#diff-b06746870e62f95017714121be6134cffdc4bd1d46dd21f8cae8c484e3e2829dR47-R49 That's all for it.

djshow832
djshow832 reviewed Mar 24, 2021
View reviewed changes
auth/auth.go
terror.Log(errors.Trace(err))
hash := crypt.Sum(nil)
// token = scrambleHash XOR stage1Hash
if len(auth) != len(hash) {
Copy link
Copy Markdown
Contributor

@djshow832 djshow832 Mar 24, 2021

Choose a reason for hiding this comment

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

Is it possible that len(auth) > len(hash)?

Copy link
Copy Markdown
Collaborator Author

@tiancaiamao tiancaiamao Mar 24, 2021

Choose a reason for hiding this comment

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

It doesn't matter, as long as len(auth) != len(hash) the input is invalid.

@djshow832
Copy link
Copy Markdown
Contributor

djshow832 commented Mar 24, 2021

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 LGT1 label Mar 24, 2021
@ti-srebot ti-srebot added the status/LGT2 LGT2 label Mar 24, 2021
@djshow832
Copy link
Copy Markdown
Contributor

djshow832 commented Mar 24, 2021

/merge

@ti-srebot
Copy link
Copy Markdown
Contributor

ti-srebot commented Mar 24, 2021

/run-all-tests

@ti-srebot
Copy link
Copy Markdown
Contributor

ti-srebot commented Mar 24, 2021

@tiancaiamao merge failed.

@morgo
Copy link
Copy Markdown
Contributor

morgo commented Mar 24, 2021

The malformed data maybe related to pingcap/tidb#19603, maybe we are not resolving the protocol correctly, but I have no clue.

PTAL @djshow832 @morgo

I took a look. The authSwitchRequest is relatively self contained unless the resp.AuthPlugin is caching_sha2_password (it doesn't attempt to authSwitch for any other plugins, even though it could). Do you know if it is a client attempting to use MySQL 8.0 auth?

There are other authentication plugins besides caching_sha2_password OR mysql_native_password that clients could attempt to use too, and it's also entirely possible it is a connector that doesn't speak the protocol correctly.

@morgo
Copy link
Copy Markdown
Contributor

morgo commented Mar 24, 2021

/run-all-tests

@kennytm
Copy link
Copy Markdown
Contributor

kennytm commented Mar 24, 2021

I think we should just get rid of the integration test. The TiDB is always lagging behind making it almost always failing and impossible to use "/merge".

@kennytm kennytm merged commit ab6d0f2 into pingcap:master Mar 24, 2021
@ti-srebot
Copy link
Copy Markdown
Contributor

ti-srebot commented Mar 25, 2021

cherry pick to release-5.0 failed

@tiancaiamao tiancaiamao deleted the fix-panic branch March 25, 2021 03:45
@tiancaiamao
Copy link
Copy Markdown
Collaborator Author

tiancaiamao commented Mar 25, 2021

I took a look. The authSwitchRequest is relatively self contained unless the resp.AuthPlugin is caching_sha2_password (it doesn't attempt to authSwitch for any other plugins, even though it could). Do you know if it is a client attempting to use MySQL 8.0 auth?

There are other authentication plugins besides caching_sha2_password OR mysql_native_password that clients could attempt to use too, and it's also entirely possible it is a connector that doesn't speak the protocol correctly.

Yup, I've said I have no clue. I'm not sure how that happen and what trigger the malformed input.
We need more feedback from the user.

@tiancaiamao tiancaiamao mentioned this pull request Mar 25, 2021
Merged
tiancaiamao added a commit to tiancaiamao/parser that referenced this pull request Mar 25, 2021
@tiancaiamao @ti-srebot
auth: fix CheckScrambledPassword() panic for invalid input (pingcap#1197
b878c27 
)

* auth: fix CheckScrambledPassword() panic for invalid input

* fix CI

Co-authored-by: ti-srebot <[email protected]>
tiancaiamao added a commit to tiancaiamao/parser that referenced this pull request Mar 25, 2021
@tiancaiamao @ti-srebot
auth: fix CheckScrambledPassword() panic for invalid input (pingcap#1197
5c1b163 
)

* auth: fix CheckScrambledPassword() panic for invalid input

* fix CI

Co-authored-by: ti-srebot <[email protected]>
This was referenced Mar 25, 2021
Merged
Merged
kennytm pushed a commit that referenced this pull request Mar 25, 2021
@tiancaiamao @ti-srebot
auth: fix CheckScrambledPassword() panic for invalid input (#1197) (#…
52ddeee 
…1199)

* auth: fix CheckScrambledPassword() panic for invalid input

* fix CI

Co-authored-by: ti-srebot <[email protected]>

Co-authored-by: ti-srebot <[email protected]>
kennytm pushed a commit that referenced this pull request Mar 25, 2021
@tiancaiamao @ti-srebot
auth: fix CheckScrambledPassword() panic for invalid input (#1197) (#…
0d17053 
…1200)

* auth: fix CheckScrambledPassword() panic for invalid input

* fix CI

Co-authored-by: ti-srebot <[email protected]>

Co-authored-by: ti-srebot <[email protected]>
tiancaiamao added a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
@tiancaiamao @ti-srebot
auth: fix CheckScrambledPassword() panic for invalid input (pingcap#1197
b0397a9 
)

* auth: fix CheckScrambledPassword() panic for invalid input

* fix CI

Co-authored-by: ti-srebot <[email protected]>
@tiancaiamao tiancaiamao mentioned this pull request Jun 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djshow832 djshow832 djshow832 left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees

No one assigned

Labels

needs-cherry-pick-release-5.0 status/can-merge status/LGT2 LGT2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tiancaiamao @kennytm @djshow832 @ti-srebot @morgo @bb7133