-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-30214 Use openssl aes encrypt/decrypt functions #18033
HPCC-30214 Use openssl aes encrypt/decrypt functions #18033
Conversation
https://track.hpccsystems.com/browse/HPCC-30214 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments
static void encryptError(const char *what) | ||
{ | ||
VStringBuffer s("openssl::aesEncrypt: %s", what); | ||
throw makeStringException(0, s.str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Shouldn't really use 0 for the error code.
system/jlib/jencrypt.cpp
Outdated
|
||
try | ||
{ | ||
if (outlen < ciphertext_len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is too strict. For instance I changed the unit test to the following, and it fails:
unsigned char key[] = { 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,
0x38, 0x39, 0x30, 0x31, 0x32, 0x33, 0x34, 0x35,
0x36, 0x37, 0x38, 0x39, 0x30, 0x31, 0x32, 0x33,
0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x31
};
/* Message to be encrypted */
constexpr const char * text = "The quick brown fox jumps over the lazy dog";
constexpr unsigned lenText = strlen(text);
const unsigned char *plaintext = (const unsigned char *)text;
MemoryBuffer ciphertext1, ciphertext2, decrypted;
openssl::aesEncrypt(key, 32, plaintext, strlen ((char *)plaintext), ciphertext1);
jlib::aesEncrypt(key, 32, plaintext, strlen ((char *)plaintext), ciphertext2);
CPPUNIT_ASSERT(ciphertext1.length()==ciphertext2.length());
CPPUNIT_ASSERT(memcmp(ciphertext1.bytes(), ciphertext2.bytes(), ciphertext1.length()) == 0);
/* Decrypt the ciphertext */
openssl::aesDecrypt(key, 32, ciphertext1.bytes(), ciphertext1.length(), decrypted);
char decryptedBuffer[lenText+1];
openssl::aesDecrypt(key, 32, ciphertext1.bytes(), ciphertext1.length(), decryptedBuffer, lenText);
decryptedBuffer[lenText] = 0;
/* Add a NULL terminator. We are expecting printable text */
decrypted.append('\0');
/* Show the decrypted text */
DBGLOG("Decrypted text is: %s/%s", (const char *) decrypted.bytes(), decryptedBuffer);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to retain this test, then we need to check that everywhere that calls this function has allocated the correct amount of memory - with any last block padding necessary.
ffcabfb
to
9521d7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardkchapman looks good, especially the test cases. Please squash.
Signed-off-by: Richard Chapman <[email protected]>
9521d7a
to
8f381ba
Compare
@ghalliday Squashed |
Type of change:
Checklist:
Smoketest:
Testing: