Hello Francesco, hello all, > On 25 December 2018 at 21:04 Francesco Pretto <cez...@gmail.com> wrote: > > According to OpenSSL 1.1.0 documentation[1], "the decrypted data buffer out > passed > to the EVP_DecryptUpdate() should have sufficient room for (inl + > cipher_block_size) > bytes". In TestEncrypt(), pDecryptedBuffer has the exactly the size of the > known clear > text, which sounds correct but it's currently violating the contract of > EVP_DecryptUpdate() > used in PdfEncryptAESBase::BaseDecrypt() and causing a buffer overflow > detected by > MSVC when running the the test in Debug build. Fix TestEncrypt() so the out > data buffer > will end having exactly inl + cipher_block_size. > > [1] https://www.openssl.org/docs/man1.1.0/crypto/EVP_DecryptUpdate.html
thanks for your patch, I'm sorry to have overlooked it when no one else reviewed it, I'm no expert in encryption. The quote about the contract of EVP_DecryptUpdate() is also mentioned in the OpenSSL 1.0.2 documentation [2], and your patch adds as much room in the output buffer for the decryption as the input got (padding to next higher multiple of 16 bytes, and AES_IV_LENGTH = 16 bytes), so because inl, the length of the encrypted data input to PdfEncrypt::Decrypt(), already includes the padding, your patch adds AES_IV_LENGTH but the documentation calls for cipher_block_size. So it can only work if both are the same number (or the IV length is larger, but that's unrealistic, and also contradicts what you've written). The 1.0.2 documentation says: "The constant EVP_MAX_IV_LENGTH is the maximum IV length for all ciphers. EVP_CIPHER_block_size() and EVP_CIPHER_CTX_block_size() return the block size of a cipher when passed an EVP_CIPHER or EVP_CIPHER_CTX structure. The constant EVP_MAX_IV_LENGTH is also the maximum block length for all ciphers." The 1.1.0 documentation doesn't have this last sentence anymore, but says instead EVP_MAX_BLOCK_LENGTH is the maximum block length. Already in 1.0.2 (I don't have 1.1.* yet) the latter constant is 32, so if it is that in 1.1.* too, your patch could break with 32 extra bytes required instead of 16 (what you're giving). This is because I don't think AES-256 (32 bytes key length) would smaller-than-key cipher blocks (I'm no expert in encryption, but could those who know more please speak up?). So to decide on this issue, I need your (I mean the list) help: who thinks they can, please do, because I'm not sure enough to reject the patch on my own, but also not to accept it. Best regards, Matthew [2] https://www.openssl.org/docs/man1.0.2/crypto/EVP_EncryptInit.html _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
