Skip to content

Fix mysql_clear_password plugin on auth switch panic. #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 21, 2017
Merged

Fix mysql_clear_password plugin on auth switch panic. #646

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,9 @@ func (mc *mysqlConn) writeAuthPacket(cipher []byte) error {
// http://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::AuthSwitchResponse
func (mc *mysqlConn) writeOldAuthPacket(cipher []byte) error {
// User password
scrambleBuff := scrambleOldPassword(cipher, []byte(mc.cfg.Passwd))
// https://dev.mysql.com/doc/internals/en/old-password-authentication.html
// Old password authentication only need and will need 8-byte challenge.
scrambleBuff := scrambleOldPassword(cipher[:8], []byte(mc.cfg.Passwd))

// Calculate the packet length and add a tailing 0
pktLen := len(scrambleBuff) + 1
Expand Down Expand Up @@ -392,7 +394,9 @@ func (mc *mysqlConn) writeClearAuthPacket() error {
// Native password authentication method
// http://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::AuthSwitchResponse
func (mc *mysqlConn) writeNativeAuthPacket(cipher []byte) error {
scrambleBuff := scramblePassword(cipher, []byte(mc.cfg.Passwd))
// https://dev.mysql.com/doc/internals/en/secure-password-authentication.html
// Native password authentication only need and will need 20-byte challenge.
scrambleBuff := scramblePassword(cipher[0:20], []byte(mc.cfg.Passwd))

// Calculate the packet length and add a tailing 0
pktLen := len(scrambleBuff)
Expand Down Expand Up @@ -495,7 +499,7 @@ func (mc *mysqlConn) readResultOK() ([]byte, error) {
if len(data) > 1 {
pluginEndIndex := bytes.IndexByte(data, 0x00)
plugin := string(data[1:pluginEndIndex])
cipher := data[pluginEndIndex+1 : len(data)-1]
cipher := data[pluginEndIndex+1:]
Copy link
Member

Choose a reason for hiding this comment

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

does this still work if the mysql server appends 0x00?

Copy link

Choose a reason for hiding this comment

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

@julienschmidt with all due respect to @elemount but with an eye on expediency (we've got a bunch devs hung up on this), would it be acceptable for me to re-submit this merge request with my changes which answers the question of the appended 0x00?

Copy link
Contributor Author

@elemount elemount Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@julienschmidt , it depends how the protocol define. In protocol, the cipher is a string[EOF], and it must be string[EOF], string[NUL] is impossible, for the auth data(many times is a random salt) may contains the 0x00 inside, . So 0x00 must be the data which the auth plugin pass, so depends on the auth plugin itself to decide how to deal with it.

Copy link

Choose a reason for hiding this comment

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

I tested it :D

https://play.golang.org/p/f2oGHOwj8H

@elemount fixes it, turns out my if is overkill :D

Copy link
Member

Choose a reason for hiding this comment

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

My concern was if it breaks native password auth, since it changes the behavior introduced in #494.
But this is actually addressed in this PR by slicing the data to the specified size, so everything should be fine.


switch plugin {
case "mysql_old_password":
Expand Down