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.

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<char>(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<SymmetricCipher> 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