Skip to content

Fix Auth Resnponse packet when cleartext is used #887

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.

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 4 commits into from
Nov 13, 2018

Conversation

methane
Copy link
Member

@methane methane commented Oct 30, 2018

Fixes #884

Description

NUL shouldn't be after string[n] auth-response. It breaks following string[NUL] database.

https://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::HandshakeResponse41

So n should contain trailing NUL byte.

I confirmed cleartext is used in sha256_password. I can't confirm cleartext password.
Especially, there are no way to enable fast cleartext password in MySQL 5.7 and 8.0.
(default-authenticate-plugin can't be "cleartext_password".)

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@methane methane changed the title [wip] Fix Auth Resnponse packet when addNUL is true Fix Auth Resnponse packet when cleartext is used Oct 31, 2018
@methane
Copy link
Member Author

methane commented Oct 31, 2018

One problem is what should be returned for empty password: empty string or one byte NUL.

Whan I tested with sha256_password (which returns cleartext when secure connection), one byte NUL is required. I suppose cleartext password plugin is same.

Copy link
Member

@julienschmidt julienschmidt left a comment

Choose a reason for hiding this comment

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

Why did you remove the addNUL?
It's there to avoid an allocation for a return value which is just copied again afterwards anyway.

@julienschmidt julienschmidt added this to the v1.4.1 milestone Oct 31, 2018
@methane
Copy link
Member Author

methane commented Nov 1, 2018

Why did you remove the addNUL?
It's there to avoid an allocation for a return value which is just copied again afterwards anyway.

Because I thought it's there to add NUL after string[n]. And appending it after n bytes was the bug.

I think both of []bytes(mc.cfg.Passwd) and append([]bytes(mc.cfg.Passwd), 0) are allocate once.

Anyway, code around addNUL looks ugly, and this code is used only for authentication, not query.
So performance difference should be ignorable.

@methane methane merged commit 369b5d6 into go-sql-driver:master Nov 13, 2018
@methane methane deleted the fix-884 branch November 13, 2018 02:38
@jpeirson
Copy link

I can confirm that the fixes in 369b5d6 fix the issue I was having.

julienschmidt pushed a commit that referenced this pull request Nov 14, 2018
Trailing NUL char should be in `string[n] auth-response`.
But NUL was after auth-response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants