MDEV-39274: Document invariant ensuring passwd stays within packet bounds#4889
MDEV-39274: Document invariant ensuring passwd stays within packet bounds#4889jmestwa-coder wants to merge 1 commit 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
3 times, most recently
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.. |
|
Comment looks appropriate, thanks! The only remaining thing is to change the title of the pull request, and the comment. Could you maybe also squash 2 commits into 1. |
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
from
75e3ec4 to
73f5429
Compare
Document that the packet buffer is null-terminated by the network layer, ensuring strend(user) remains within bounds and passwd stays within the packet.
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
from
73f5429 to
bf1d7ae
Compare
jmestwa-coder
changed the title
|
Thanks for the review. I’ve updated the title and comment, and squashed the commits into a single one. Please let me know if everything looks good now. |
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