sahvx655-wq opened a new pull request, #3717:
URL: https://github.com/apache/brpc/pull/3717
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
`MysqlReply::Field::ParseMicrosecs()` formats the 4-byte microseconds field
of a binary-protocol `DATETIME`/`TIMESTAMP` (length 11) or `TIME` (length 12)
value. It splits the value into three `uint8_t` groups (`p1 = us / 10000`, then
`p2`, `p3`) and uses them to index `digits10`/`digits01`, which are 101-byte
string literals. `ParseBinaryDataTime()` and `ParseBinaryTime()` range-check
year, month, day, hour, minute and second before the same lookups, and the
comment there says that is what keeps the indices inside the tables, but the
microseconds value is used exactly as the server sent it. Anything above 999999
gives a group between 100 and 255, so the parser reads up to 154 bytes past the
end of the literal and copies them into the string returned by `field.string()`.
The value comes straight from the result row of a prepared-statement
execute, so a malicious or compromised MySQL server, or anyone on the plaintext
connection, can trigger it with a `DATETIME(6)` or `TIME(6)` column and a row
carrying microseconds = 1010000. With the new test on master under ASan:
```
ERROR: AddressSanitizer: global-buffer-overflow ... READ of size 1
#0 brpc::MysqlReply::Field::ParseMicrosecs(...) mysql_reply.cpp
#1 brpc::MysqlReply::Field::ParseBinaryDataTime(...) mysql_reply.cpp:1387
#2 brpc::MysqlReply::Field::Parse(...) mysql_reply.cpp:1115
#3 brpc::MysqlReply::Row::Parse(...) mysql_reply.cpp:886
#4 brpc::MysqlReply::ResultSet::Parse(...) mysql_reply.cpp:1498
#5 brpc::MysqlReply::ConsumePartialIOBuf(...) mysql_reply.cpp:371
0x... is located 0 bytes after global variable '.str.120' ... of size 101
'.str.120' is ascii string '0000000000111111111122222222223333333333...'
```
Without ASan the read goes unnoticed and a few bytes of neighbouring
read-only data end up in the formatted value. Values from 1000000 to 1009999
stay inside the literal but hit its terminating NUL, so the returned string
carries NUL bytes where the first two fraction digits should be.
### What is changed and the side effects?
Changed:
- `ParseMicrosecs()` rejects a microseconds value above 999999 with
`PARSE_ERROR_ABSOLUTELY_WRONG` right after reading it, the same way the other
out-of-range components are rejected. The check sits in the shared helper, so
it covers both `ParseBinaryDataTime()` and `ParseBinaryTime()`, whatever the
column's declared decimals.
- Added `MysqlReplyParseTest.RejectOutOfRangeBinaryMicroseconds` (DATETIME
and TIME with 1000000, 1010000 and 0xFFFFFFFF), which fails on master, and
`MysqlReplyParseTest.AcceptMaxBinaryMicroseconds`, which checks that 999999
still formats as `.999999`.
Side effects:
- Performance effects: none; one comparison per temporal value that carries
microseconds.
- Breaking backward compatibility: no. MySQL never sends a microseconds
value above 999999, so only malformed rows are affected.
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
Tests run (CMake Debug + ASan build on macOS with clang):
`brpc_mysql_reply_parse_unittest` (36 tests),
`brpc_mysql_auth_packet_unittest`, `brpc_mysql_auth_scramble_unittest` and
`brpc_mysql_auth_handshake_unittest`, all passing. The two touched files also
compile cleanly with `-Wall -W -Werror`. The mysqld-backed integration tests
were skipped here because no server was available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
