[MDEV-30389] Calls to `encryption_crypt` currently pass uninitialized `dlen`, should pass correct buffer size Created: 2023-01-12 Updated: 2023-07-02 Resolved: 2023-06-26 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Encryption, Plugins |
| Fix Version/s: | 11.0.3, 11.1.2, 11.2.1 |
| Type: | Task | Priority: | Major |
| Reporter: | Trevor Gross | Assignee: | Sergei Golubchik |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | encryption, plugins | ||
| Description |
|
Calls to `encryption_crypt` (service_encryption.h) seem to generally receive an uninitialized `*dlen`, which it passes on to the encryption service calls. This is an output value that receives the number of bytes written to the `dst` buffer. This should be updated to pass the correct length of the `dst` buffer instead of an uninitialized value. There is no problem for encryption schemes where it can be assumed that `dlen == slen`. However, in cases where the two may not be equal, it would be good to allow for debug assertions validating that the destination buffer is large enough. In general, the buffer should be large enough, but it is currently impossible to verify this is true in code. Finding the information in source is also difficult, so plugin writers have no way to verify they aren't creating buffer overwrites. |
| Comments |
| Comment by Marko Mäkelä [ 2023-01-12 ] |
|
Which users of encryption would support asymmetric encryption, or one where the length of the encrypted data is not identical with the unencrypted data? I know that InnoDB assumes that the lengths will be the same. Changing that would be a huge task. |
| Comment by Trevor Gross [ 2023-01-12 ] |
|
Some schemes may have a CRC, and tags could be in the middle of the data depending on how it is chunked, but I don't know of any specific use cases. From the comment on update though (https://mariadb.com/kb/en/encryption-plugin-api/): > Writes the output to th dst buffer. note that it might write So it definitely seems like `dst` may be larger than `src` - it's currently not possible to know by how much though. It seems like maybe this should come from `encrypted_length`, but I don't see any calls to its wrapper `encryption_encrypted_length_func` in the source, so I'm not positive what the intent is. |
| Comment by Trevor Gross [ 2023-01-12 ] |
|
To be clear- I wasn't suggesting any change to the behavior for how big the buffers actually are, just that `*dlen` passed to the plugin contains the actual length of the buffer instead of being uninitialized (to allow checked writes on the plugin side). I don't think this would be a huge change, but I'm looking into it. |
| Comment by Sergei Golubchik [ 2023-05-24 ] |
|
marko, reassigned to you, because as far as I'm concerned the PR is fine (was, last time I've checked). |
| Comment by Marko Mäkelä [ 2023-06-26 ] |
|
For some reason, the GitHub option to rebase&merge PR#2438 was greyed out, so I can’t merge this. |