MDEV-41094 KDF() aliases large iteration/width to weak 32-bit values - #5731
Conversation
|
|
1f1d5b3 to
22f2553
Compare
|
Please, not so blatantly AI-generated, rewrite the commit comment to be readable. |
22f2553 to
fb384bf
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Key-width validation still narrows the value before checking it on supported 32-bit builds.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Hardens KDF() validation against oversized iteration counts and key widths, with regression coverage.
Changes:
- Validates arguments before narrowing.
- Rejects oversized PBKDF2 iterations and key widths.
- Adds regression tests and expected results.
| File | Description |
|---|---|
sql/item_strfunc.cc |
Adds KDF argument validation. |
mysql-test/main/func_kdf.test |
Adds oversized-argument regression cases. |
mysql-test/main/func_kdf.result |
Records expected NULL results and warnings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -544,6 +548,18 @@ String *Item_func_kdf::val_str(String *buf) | |||
| } | |||
| klen/= 8; | |||
There was a problem hiding this comment.
I wondered why this comment was apparently ignored (not replied to). The comment does appear to be redundant; right before the assignment we are actually ensuring that width will fit in 17 bits, which is less than the minimum size_t width of 32 bits that we support:
if (width <= 0 || width > UINT_MAX16 + 1 || width % 8)Side note: if we didn’t have to reject width == 0 or accept width == 0x10000, the condition could be rewritten as width & ~longlong{UINT_MAX16 - 8} or just width & ~0xFFF7LL.
fb384bf to
25fc0ae
Compare
KDF() narrowed its iteration-count and key-width arguments without checking they fit, so values differing by 2^32 aliased to the same small value and silently derived a much weaker key. Added checks that the key width fits a 16-bit unsigned value and the iteration count fits a 32-bit value before narrowing them, rejecting out-of-range values with an error instead of aliasing.
25fc0ae to
f80dc75
Compare
Item_func_kdf::val_str() read the PBKDF2 iteration count as a signed 64-bit longlong but only rejected values <= 0 before narrowing it to the 32-bit int expected by PKCS5_PBKDF2_HMAC(). Iteration counts that differ by 2^32 therefore aliased to the same 32-bit value and derived identical keys: e.g. 4294968296 silently did the work of 1000. Since the value is reproducible as (iter mod 2^32), any key derived this way was already only as strong as the aliased low iteration count, so no previously-derived ciphertext is orphaned by rejecting the alias now; it simply reports the weak request instead of silently honouring it.
Item_func_kdf::fix_length_and_dec() had the identical bug for the key width argument:
key_length= (uint)args[4]->val_int()narrows before the range check, and because the result is cached as a constant, the runtime guard in val_str() (which uses a wider type and is otherwise safe) is never reached for a constant width argument. This let a width like 4294967552 silently alias to 256, and let negative widths alias to a plausible positive one, both without warning.Both call sites now validate the argument's true 64-bit value before narrowing, reusing the existing invalid_argument_error() and NULL result already used for other invalid KDF() arguments.