Fixed crash when decrypting bogus input#40168
Fixed crash when decrypting bogus input#40168Enmk wants to merge 1 commit intoClickHouse:masterfrom Enmk:fixed_Nullable_getDataAt_and_decrypt
decrypting bogus input#40168Conversation
Some assumptions of ColumnArray::getDataAt() were broken by ColumnNullable::getDataAt(). Also made sure that users call decrypt only on String or FixedString
robot-clickhouse
added
the
pr-bugfix
| */ | ||
| StringRef getDataAt(size_t n) const override | ||
| { | ||
| auto nestedData = getNestedColumn().getDataAt(n); |
| if (isNullAt(n)) | ||
| return EMPTY_STRING_REF; | ||
| { | ||
| // data for NULL-items must have valid data pointer, otherwise some assumtions down the road are broken. |
There was a problem hiding this comment.
The comment is unclear.
| { | ||
| auto nestedData = getNestedColumn().getDataAt(n); | ||
| if (isNullAt(n)) | ||
| return EMPTY_STRING_REF; |
There was a problem hiding this comment.
EMPTY_STRING_REF is unneeded and should be wiped from the codebase entirely.
| FunctionArgumentDescriptors{ | ||
| {"mode", &isStringOrFixedString<IDataType>, isColumnConst, "decryption mode string"}, | ||
| {"input", nullptr, nullptr, "ciphertext"}, | ||
| {"input", &isStringOrFixedString<IDataType>, nullptr, "ciphertext"}, |
There was a problem hiding this comment.
Why the validator for the third argument remains "nullptr"?
There was a problem hiding this comment.
Because we don't impose any limitations on column type, it can be either const, nullable or regular.
| UInt64 index = 0; | ||
|
|
||
| if (elem != EMPTY_STRING_REF) | ||
| if (elem.size == 0 && col_arg_cloned->isNullAt(0)) |
There was a problem hiding this comment.
What this branch of code should actually check?
| SELECT decrypt('aes-128-ecb', 'text', 'key', 'IV', 1213); --{serverError 43} bad AAD type | ||
| SELECT decrypt('aes-128-gcm', 'text', 'key', 'IV', 1213); --{serverError 43} bad AAD type | ||
|
|
||
| -- Invalid ciphertext type |
There was a problem hiding this comment.
Let's add a new test instead of editing the old test.
| auto nestedData = getNestedColumn().getDataAt(n); | ||
| if (isNullAt(n)) | ||
| return EMPTY_STRING_REF; | ||
| { |
There was a problem hiding this comment.
I think this branch can be removed.
There was a problem hiding this comment.
In that case we'll get non-zero size StringRef. E.g. 4 for UInt32, and IMO that would lead to matching hashes for UInt32(0) and null item, which might break something else.
|
Tests have failed. |
|
Superseded by #40194. |
3 participants
Changelog category
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Do not allow anything but String or FixedString as
ciphertextparameter ofdecrypt....
Closes: #39987