Re: bd616a3733c: Auth: set thd->scramble in all cases during writing Initial Handshake Packet
Hi, Nikita, On Sep 09, Nikita Malyavin wrote:
revision-id: bd616a3733c (mariadb-11.6.1-13-gbd616a3733c) parent(s): bbbb429a1eb author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2024-09-07 20:46:19 +0200 message:
Auth: set thd->scramble in all cases during writing Initial Handshake Packet
--- sql/sql_acl.cc | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 93efc5806cc..7845af55413 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -13504,17 +13504,21 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, mpvio->cached_server_packet.pkt_len= data_len; }
- if (data_len < SCRAMBLE_LENGTH) + if (thd->scramble[SCRAMBLE_LENGTH] != 0)
old code was obviously wrong but the new one makes no sense either. How can thd->scramble[SCRAMBLE_LENGTH] be not zero at this point? I'd think you need to execute the code below unconditionally.
{ + DBUG_ASSERT(thd->scramble[SCRAMBLE_LENGTH] == 1); // Sanity + if (data_len) { /* the first packet *must* have at least 20 bytes of a scramble. - if a plugin provided less, we pad it to 20 with zeros + if a plugin provided less, we pad it to 20 with zeros, + plus extra zero termination sign is put in thd->scramble. + If more is provided, we'll use only 20 bytes as a handshake scramble.
Not sure it's a good idea. Why not to send the complete scramble? True, it won't fit on the client side into mysql->scramble[]. Two ways to fix it: * Write the first 20 bytes. The server will only store the first 20 bytes too, and for COM_CHANGE_USER they'll use the first 20 bytes only (for the login auth they can use the complete scramble). That's a rather hackish fix, a proper fix would be * store the complete scramble in MYSQL. The first 20 bytes in MYSQL::scramble_buff, the rest in MySQL::extension.scramble_suffix
*/ - memcpy(scramble_buf, data, data_len); - bzero(scramble_buf + data_len, SCRAMBLE_LENGTH - data_len); - data= scramble_buf; + size_t fill_size= MY_MIN(SCRAMBLE_LENGTH, data_len); + memcpy(thd->scramble, data, fill_size); + bzero(thd->scramble + fill_size, SCRAMBLE_LENGTH - fill_size + 1); }
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei! This patch has a server-side only effect. You see, the old code was sending *either* the internally-generated scramble, or the plugin-provided payload. But never both. The protocol would interpret both as a scramble. You wrote:
How can thd->scramble[SCRAMBLE_LENGTH] be not zero at this point?
thd->scramble[SCRAMBLE_LENGTH] is set to 1 in acl_authenticate(): if (command == COM_CHANGE_USER) { ... } else { /* mark the thd as having no scramble yet */ thd->scramble[SCRAMBLE_LENGTH]= 1; So when THD is just connecting, it has 1 in the (SCRAMBLE_LENGTH+1)-th byte. This is true only for the server: libmariadb makes no such marking, so it's harder (I know no way) to distinguish the initial handshake from an extra roundtrip. Overall, my solution is to use only the first 20 bytes as a nonce used to generate the SSL secret. Quick'n'dirty, maybe, but I think that supporting arbitrary length scrambles can be done on top of it, if necessary. It won't make it into 11.7, though. Best regards, Nikita
Hi, Nikita, On Sep 09, Nikita Malyavin wrote:
Hello Sergei!
This patch has a server-side only effect. You see, the old code was sending *either* the internally-generated scramble, or the plugin-provided payload. But never both.
which is correct, I don't see why it would need to send both
The protocol would interpret both as a scramble.
You wrote:
How can thd->scramble[SCRAMBLE_LENGTH] be not zero at this point?
thd->scramble[SCRAMBLE_LENGTH] is set to 1 in acl_authenticate():
if (command == COM_CHANGE_USER) { ... } else { /* mark the thd as having no scramble yet */ thd->scramble[SCRAMBLE_LENGTH]= 1;
So when THD is just connecting, it has 1 in the (SCRAMBLE_LENGTH+1)-th byte.
yes, sorry, I didn't phrase the question correctly. thd->scramble[SCRAMBLE_LENGTH]= 1; was needed to tell the plugin to generate the scramble. A plugin can be called in the middle of the authentication or in the COM_CHANGE_USER. It doesn't know what happens before it and need to know if the scramble was already generated. The function where you added your condition if (thd->scramble[SCRAMBLE_LENGTH] != 0) it's send_server_handshake_packet(), it can only happen as the first packet during authentication, you know with certainty that the scramble was not yet generated, you don't need a condition for that.
This is true only for the server: libmariadb makes no such marking, so it's harder (I know no way) to distinguish the initial handshake from an extra roundtrip.
Overall, my solution is to use only the first 20 bytes as a nonce used to generate the SSL secret. Quick'n'dirty, maybe, but I think that supporting arbitrary length scrambles can be done on top of it, if necessary.
it won't be easy to add later without breaking the compatibility with existing clients, I suspect. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
On Mon, 9 Sept 2024 at 21:58, Sergei Golubchik <serg@mariadb.org> wrote:
How can thd->scramble[SCRAMBLE_LENGTH] be not zero at this point? *[...]* thd->scramble[SCRAMBLE_LENGTH]= 1;
was needed to tell the plugin to generate the scramble. A plugin can be called in the middle of the authentication or in the COM_CHANGE_USER. It doesn't know what happens before it and need to know if the scramble was already generated.
Only two plugins were setting thd->scramble, and only them could be default. Now the situation changes, and any plugin could be tried as default. In this case, thd->scramble will be found unset. BTW, that doesn't help to distinguish being executed as a non-default plugin during handshake from COM_CHANGE_USER. Does this sort out the issue? it won't be easy to add later without breaking the compatibility with
existing clients, I suspect.
Indeed. I would skip that part completely. It seems to me that scramble is not important in the certificate validation and can be safely omitted. Do I miss something? -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik