Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

On Sat, 4 Jan 2020 at 15:11, cary huang <hcary328(at)gmail(dot)com> wrote:
>
> Hello Sawada and all
>
> I would like to elaborate more on Sehrope and Sawada's discussion on passing NULL IV in "pg_cipher_encrypt/decrypt" functions during kmgr_wrap_key and kmgr_unwrap_key routines in kmgr_utils.c. Openssl implements key wrap according to RFC3394 as Sawada mentioned and passing NULL will make openssl to use default IV, which equals to A6A6A6A6A6A6A6A6. I have confirmed this on my side; A key wrapped with "NULL" IV can be unwrapped successfully with IV=A6A6A6A6A6A6A6A6, and unwrap will fail if IV is set to anything else other than NULL or A6A6A6A6A6A6A6A6.
>

Sehrope also suggested me not to use the fixed IV in order to avoid
getting the same result from the same value. I'm researching it now.
Also, currently it's using key wrap algorithm[1] but it accepts only
multiple of 8 bytes as input. Since it's not good for some cases it's
better to use key wrap with padding algorithm[2] instead, which seems
available in OpenSSL 1.1.0 or later.

> I would like to provide some comments on the encryption and decryption routines provided by cipher_openssl.c in which cipher.c and kmgr_utils.c are using. I see that "ossl_cipher_encrypt" calls "EVP_EncryptInit_ex" and "EVP_EncryptUpdate" only to complete the encryption. Same thing applies to decryption routines. According to my past experience with openssl and the usages online, it is highly recommended to use "init-update-final" cycle to complete the encryption and I see that the "final" part (EVP_EncryptFinal) is missing. This call will properly handle the last block of data especially when padding is taken into account. The functions still works now because the input is encryption key and its size is multiple of each cipher block and no padding is used. I think it will be safer to use the proper "init-update-final" cycle for encryption/decryption

Agreed.

>
> According to openssl EVP documentation, "EVP_EncryptUpdate" can be called multiple times at different offset to the input data to be encrypted. I see that "pg_cipher_encrypt" only calls "EVP_EncryptUpdate" once, which makes me assume that the application should invoke "pg_cipher_encrypt" multiple times until the entire data block is encrypted? I am asking because if we were to use "EVP_EncryptFinal" to complete the encryption cycle, then it is better to let "pg_cipher_encrypt" to figure out how many times "EVP_EncryptUpdate" should be called and finalize it with "EVP_EncryptFinal" at last block.

IIUC EVP_EncryptUpdate can encrypt the entire data block.
EVP_EncryptFinal_ex encrypts any data that remains in a partial block.

>
> Lastly, I think we are missing a cleanup routine that calls "EVP_CIPHER_CTX_free()" to free up the EVP_CIPHER_CTX when encryption is done.

Right.

While reading pgcrypto code I thought that it's better to change the
cryptographic code (cipher.c) so that pgcrypto can use them instead of
having duplicated code. I'm trying to change it so.

[1] https://tools.ietf.org/html/rfc3394
[2] https://tools.ietf.org/html/rfc5649

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services