From 38e421d9c1ecc41f6161b0121a55aa3a59889c7f Mon Sep 17 00:00:00 2001 From: Felix Geyer Date: Tue, 8 May 2012 21:33:21 +0200 Subject: Fix padding handling in SymmetricCipherStream. The implementation had two issues: - It didn't add a block full of padding when the input size was a multiple of the block size. - It didn't strip the padding when reading data. --- src/streams/SymmetricCipherStream.cpp | 58 +++++++++++++++++++++++++++-------- src/streams/SymmetricCipherStream.h | 2 +- tests/TestSymmetricCipher.cpp | 37 +++++++++++++++++++--- tests/TestSymmetricCipher.h | 1 + 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/src/streams/SymmetricCipherStream.cpp b/src/streams/SymmetricCipherStream.cpp index 365a282a3..fc364e83c 100644 --- a/src/streams/SymmetricCipherStream.cpp +++ b/src/streams/SymmetricCipherStream.cpp @@ -36,7 +36,7 @@ SymmetricCipherStream::~SymmetricCipherStream() bool SymmetricCipherStream::reset() { if (isWritable()) { - if (!writeBlock()) { + if (!writeBlock(true)) { return false; } } @@ -53,7 +53,7 @@ bool SymmetricCipherStream::reset() void SymmetricCipherStream::close() { if (isWritable()) { - writeBlock(); + writeBlock(true); } LayeredStream::close(); @@ -63,13 +63,22 @@ qint64 SymmetricCipherStream::readData(char* data, qint64 maxSize) { Q_ASSERT(maxSize >= 0); + if (m_error) { + return -1; + } + qint64 bytesRemaining = maxSize; qint64 offset = 0; while (bytesRemaining > 0) { if ((m_bufferPos == m_buffer.size()) || m_bufferFilling) { if (!readBlock()) { - return maxSize - bytesRemaining; + if (m_error) { + return -1; + } + else { + return maxSize - bytesRemaining; + } } } @@ -102,7 +111,32 @@ bool SymmetricCipherStream::readBlock() m_cipher->processInPlace(m_buffer); m_bufferPos = 0; m_bufferFilling = false; - return true; + + if (m_baseDevice->atEnd()) { + // PKCS7 padding + quint8 padLength = m_buffer.at(m_buffer.size() - 1); + + if (padLength == m_cipher->blockSize()) { + Q_ASSERT(m_buffer == QByteArray(m_cipher->blockSize(), m_cipher->blockSize())); + // full block with just padding: discard + m_buffer.clear(); + return false; + } + else if (padLength > m_cipher->blockSize()) { + // invalid padding + m_error = true; + return false; + } + else { + Q_ASSERT(m_buffer.right(padLength) == QByteArray(padLength, padLength)); + // resize buffer to strip padding + m_buffer.resize(m_cipher->blockSize() - padLength); + return true; + } + } + else { + return true; + } } } @@ -111,7 +145,7 @@ qint64 SymmetricCipherStream::writeData(const char* data, qint64 maxSize) Q_ASSERT(maxSize >= 0); if (m_error) { - return 0; + return -1; } qint64 bytesRemaining = maxSize; @@ -126,7 +160,7 @@ qint64 SymmetricCipherStream::writeData(const char* data, qint64 maxSize) bytesRemaining -= bytesToCopy; if (m_buffer.size() == m_cipher->blockSize()) { - if (!writeBlock()) { + if (!writeBlock(false)) { if (m_error) { return -1; } @@ -140,18 +174,18 @@ qint64 SymmetricCipherStream::writeData(const char* data, qint64 maxSize) return maxSize; } -bool SymmetricCipherStream::writeBlock() +bool SymmetricCipherStream::writeBlock(bool lastBlock) { - if (m_buffer.isEmpty()) { - return true; - } - else if (m_buffer.size() != m_cipher->blockSize()) { + if (lastBlock) { // PKCS7 padding int padLen = m_cipher->blockSize() - m_buffer.size(); - for (int i = m_buffer.size(); i < m_cipher->blockSize(); i++) { + for (int i = 0; i < padLen; i++) { m_buffer.append(static_cast(padLen)); } } + else if (m_buffer.isEmpty()) { + return true; + } m_cipher->processInPlace(m_buffer); diff --git a/src/streams/SymmetricCipherStream.h b/src/streams/SymmetricCipherStream.h index b236cf66e..6a4c9fdee 100644 --- a/src/streams/SymmetricCipherStream.h +++ b/src/streams/SymmetricCipherStream.h @@ -41,7 +41,7 @@ protected: private: bool readBlock(); - bool writeBlock(); + bool writeBlock(bool lastBlock); const QScopedPointer m_cipher; QByteArray m_buffer; diff --git a/tests/TestSymmetricCipher.cpp b/tests/TestSymmetricCipher.cpp index 7560a6765..fb786efde 100644 --- a/tests/TestSymmetricCipher.cpp +++ b/tests/TestSymmetricCipher.cpp @@ -53,18 +53,20 @@ void TestSymmetricCipher::testAes256CbcEncryption() SymmetricCipher::Encrypt, key, iv); buffer.open(QIODevice::WriteOnly); stream.open(QIODevice::WriteOnly); - QVERIFY(stream.reset()); + buffer.reset(); buffer.buffer().clear(); stream.write(plainText.left(16)); QCOMPARE(buffer.data(), cipherText.left(16)); - QVERIFY(stream.reset()); + // make sure padding is written + QCOMPARE(buffer.data().size(), 32); + buffer.reset(); buffer.buffer().clear(); stream.write(plainText.left(10)); - QCOMPARE(buffer.data(), QByteArray()); + QVERIFY(buffer.data().isEmpty()); QVERIFY(stream.reset()); buffer.reset(); @@ -89,7 +91,9 @@ void TestSymmetricCipher::testAes256CbcDecryption() QCOMPARE(cipher.process(cipherText), plainText); - QBuffer buffer(&cipherText); + // padded with 16 0x16 bytes + QByteArray cipherTextPadded = cipherText + QByteArray::fromHex("3a3aa5e0213db1a9901f9036cf5102d2"); + QBuffer buffer(&cipherTextPadded); SymmetricCipherStream stream(&buffer, SymmetricCipher::Aes256, SymmetricCipher::Cbc, SymmetricCipher::Decrypt, key, iv); buffer.open(QIODevice::ReadOnly); @@ -164,4 +168,29 @@ void TestSymmetricCipher::testSalsa20() QCOMPARE(cipherTextB.mid(448, 64), expectedCipherText4); } +void TestSymmetricCipher::testPadding() +{ + QByteArray key = QByteArray::fromHex("603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"); + QByteArray iv = QByteArray::fromHex("000102030405060708090a0b0c0d0e0f"); + QByteArray plainText = QByteArray::fromHex("6bc1bee22e409f96e93d"); + + QBuffer buffer; + buffer.open(QIODevice::ReadWrite); + + SymmetricCipherStream streamEnc(&buffer, SymmetricCipher::Aes256, SymmetricCipher::Cbc, + SymmetricCipher::Encrypt, key, iv); + streamEnc.open(QIODevice::WriteOnly); + streamEnc.write(plainText); + streamEnc.close(); + buffer.reset(); + // make sure padding is written + QCOMPARE(buffer.buffer().size(), 16); + + SymmetricCipherStream streamDec(&buffer, SymmetricCipher::Aes256, SymmetricCipher::Cbc, + SymmetricCipher::Decrypt, key, iv); + streamDec.open(QIODevice::ReadOnly); + QByteArray decrypted = streamDec.readAll(); + QCOMPARE(decrypted, plainText); +} + KEEPASSX_QTEST_CORE_MAIN(TestSymmetricCipher) diff --git a/tests/TestSymmetricCipher.h b/tests/TestSymmetricCipher.h index 081010d82..2d8f00bea 100644 --- a/tests/TestSymmetricCipher.h +++ b/tests/TestSymmetricCipher.h @@ -29,6 +29,7 @@ private Q_SLOTS: void testAes256CbcEncryption(); void testAes256CbcDecryption(); void testSalsa20(); + void testPadding(); }; #endif // KEEPASSX_TESTSYMMETRICCIPHER_H -- cgit v1.2.3