MDEV-39274: Extra hardening of the parser for the ClientHello packet#4889
MDEV-39274: Extra hardening of the parser for the ClientHello packet#4889jmestwa-coder wants to merge 2 commits intoMariaDB:mainfrom
Conversation
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
2 times, most recently
from
9fbba8d to
79b17c9
Compare
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
Would you consider describing the problem and a sequence of actions that lead to it?
Preferably there should be a regression test for it.
sql/sql_acl.cc
Outdated
| } | ||
| else if (!(thd->client_capabilities & CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA)) | ||
| { | ||
| if (passwd >= (char*) net->read_pos + pkt_len) |
There was a problem hiding this comment.
This check, if it needs to be done, needs to be done after assigning passwd. Or, maybe, instead of doing this check, how about constraining strend to never go out of buffer? Well, IMHO you do not even need to do that: higher level code ensures that all packages are actually null-terminated iirc.
Which brings me to my next question: do you have a test to demonstrate the problem?
If you can't hit this by fuzzing the network data (as I suppose you wouldn't be able to), then convert this into an assert to make sure the packet is properly terminated.
There was a problem hiding this comment.
You’re right that the packet buffer is guaranteed to be null-terminated by higher-level code so it’s hard to demonstrate this as a real runtime issue.
I’ve updated the change to use a DBUG_ASSERT instead, to document and enforce the assumption that passwd stays within the packet bounds.
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
2 times, most recently
from
e878335 to
b2cbd8c
Compare
Replace runtime check with DBUG_ASSERT to document the invariant that passwd remains within the packet buffer, as guaranteed by higher-level code.
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
from
b2cbd8c to
f15f504
Compare
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please stand by for the final review.
|
please also update the description of the PR |
gkodinov
changed the title
gkodinov
changed the title
gkodinov
changed the title
|
Updated the PR description accordingly. |
There was a problem hiding this comment.
I' not sure this can be qualified "hardening"
Is there an possible out-of-bound?
If yes, please add a test case (without client, using just pure socket send). Then you'd need to return with an error, in the place where out of bounds would otherwise be read.
If out of bounds is impossible, a comment will be enough why it is impossible would be good enough.
a DBUG_ASSERT only "hardens" a debug build, something for the CI mostly. It is quite a weak "hardening".
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
from
30624e7 to
65d2fde
Compare
|
I’ve updated the change to document the invariant with a comment instead, clarifying why passwd remains within the packet bounds based on the null-termination guarantee. let me know if this looks appropriate.. |
3 participants
Summary
Add a debug assertion in
parse_client_handshake_packetto ensure thatpasswdstays within the packet buffer.Background
In the secure-connection branch,
passwdis derived from the username field and then dereferenced to read the password length.The parser relies on the assumption that
passwdremains within the packet bounds, as ensured by the surrounding parsing logic.Change
DBUG_ASSERTto validate thatpasswddoes not exceed the packet boundary