Skip to content

bound datetime/time stmt param reads to the declared length#5164

Open
jmestwa-coder wants to merge 1 commit into
MariaDB:10.6from
jmestwa-coder:stmt-param-time-overread
Open

bound datetime/time stmt param reads to the declared length#5164
jmestwa-coder wants to merge 1 commit into
MariaDB:10.6from
jmestwa-coder:stmt-param-time-overread

Conversation

@jmestwa-coder
Copy link
Copy Markdown
Contributor

get_param_length() validates only length bytes for a parameter value, but the binary-protocol datetime reader reads to[4..6] when length>4 (needs 7) and to[7..10] when length>7 (needs 11), and the time reader reads to[8..11] when length>8 (needs 12). A client sending an off-spec datetime length of 5/6/8/9/10 or a time length of 9/10/11 makes the server read past the value, out of bounds when it ends at the packet boundary. Gate the optional time and microsecond reads on the canonical encoding lengths (7/11 for datetime, 12 for time).

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the length validation checks when parsing TIME and DATETIME parameters in sql/sql_prepare.cc to ensure proper bounds checking before reading time components and fractional seconds. There are no review comments, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jun 2, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. This is a preliminary review!

This needs:

  • a MDEV describing the problem, complete with steps to reproduce
  • a commit message compliant with the MariaDB coding standards
  • a regression test
  • a rebase to the lowest affected version (this is a bug fix and not a feature).

…ength

get_param_length() guarantees only `length` bytes for a prepared-statement
parameter value sent over the binary protocol. set_param_datetime() reads
to[4..6] when length > 4 (needs 7 bytes) and sint4korr(to+7) when length > 7
(needs 11); set_param_time() reads sint4korr(to+8) when length > 8 (needs 12).

A client that sends an off-spec datetime length of 5, 6, 8, 9 or 10, or a
time length of 9, 10 or 11, makes the server read up to 3 bytes past the
validated value, an out-of-bounds read when the value ends at the packet
buffer boundary.

Gate the optional time and microsecond reads on the canonical encoding
lengths (>= 7 and >= 11 for datetime, >= 12 for time) so a short value falls
back to the next-shorter form instead of over-reading. Well-formed values
(datetime 4/7/11, time 8/12) parse exactly as before.
@jmestwa-coder jmestwa-coder force-pushed the stmt-param-time-overread branch from c737116 to 3eec106 Compare June 3, 2026 11:12
@jmestwa-coder jmestwa-coder changed the base branch from main to 10.6 June 3, 2026 11:13
@jmestwa-coder
Copy link
Copy Markdown
Contributor Author

Pushed an update:

  • Rebased onto 10.6, the lowest maintained branch that carries this code, and retargeted the PR base to 10.6.
  • Rewrote the commit message to describe the problem and the fix.

How it triggers: get_param_length() only validates length bytes, but the datetime reader touches to[4..6] once length > 4 (needs 7) and to[7..10] once length > 7 (needs 11), and the time reader touches to[8..11] once length > 8 (needs 12). So a client sending a non-canonical temporal length (datetime 5/6/8/9/10, time 9/10/11) reads up to 3 bytes past the value, observable as an OOB read when the value sits at the end of the packet buffer.

On the regression test: the off-spec length isn't reachable through the client library, since it always encodes the canonical lengths (4/7/11 for datetime, 8/12 for time), so mysql_client_test can't drive a short length. I confirmed the bug with a small standalone harness that runs get_param_length() plus the read logic with the value placed flush against a PROT_NONE guard page: the off-spec lengths fault before the patch and exit clean after, while the valid lengths are unchanged. I can attach that harness, or wire up a crafted-packet test if there's a spot you'd prefer for one.

I'll open the MDEV with the details above and prefix the commit subject with the id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

2 participants