From 1714d43ba5182b91b194c0862a0e6b6228fc45b8 Mon Sep 17 00:00:00 2001 From: benrubson Date: Sat, 12 May 2018 16:13:30 +0200 Subject: [PATCH 01/14] Padding, backward compatibility boolean --- encfs/CipherFileIO.cpp | 33 ++++++++++++++++++++++++++++++++- encfs/CipherFileIO.h | 1 + encfs/SSL_Cipher.cpp | 6 +++--- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 3a0a2975..0310374e 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -48,9 +48,39 @@ static Interface CipherFileIO_iface("FileIO/Cipher", 2, 0, 1); const int HEADER_SIZE = 8; // 64 bit initialization vector.. +/* + * Check if we enable CBC padding (instead of using stream cipher for the last block). + * Here we check against the cipher interface version rather than our own FileIO/Cipher one, + * because this latter is not stored in the configuration file... + * Padding (of files > 0 bytes) follows the OneAndZeroes rule : + * - each data block to encode is at most blockSize -1 bytes in length ; + * - each data block is padded with a 0x80 byte ; + * - the last data block is padded with cipherBlockSize -1 additional 0x00 bytes. + * Some 0x00 padding bytes may then be written out of the ciphertext, at the end of the file, + * if the length of the last data block is not already a multiple of cipherBlockSize. + * This allows to compute files' length without having to read the last block, at a cost of : + * int((fileSize - 1) / (blockSize - 1)) + cipherBlockSize. + * This function returns : + * - 2 if we pad in reverse mode + * - 1 if we pad in normal mode + * - 0 if we don't pad + * The return value helps us in CipherFileIO initialization below to set the blockSize. + * blockSize = blockSize - 1 in normal mode. + */ +int checkCBCPadding(const FSConfigPtr &cfg) { + if (((cfg->config->cipherIface.current() == 3) && (cfg->config->cipherIface.revision() >= 1)) || + (cfg->config->cipherIface.current() > 3)) { + if (cfg->reverseEncryption) { + return 2; + } + return 1; + } + return 0; +} + CipherFileIO::CipherFileIO(std::shared_ptr _base, const FSConfigPtr &cfg) - : BlockFileIO(cfg->config->blockSize, cfg), + : BlockFileIO(cfg->config->blockSize - checkCBCPadding(cfg) % 2, cfg), base(std::move(_base)), haveHeader(cfg->config->uniqueIV), externalIV(0), @@ -59,6 +89,7 @@ CipherFileIO::CipherFileIO(std::shared_ptr _base, fsConfig = cfg; cipher = cfg->cipher; key = cfg->key; + haveCBCPadding = (checkCBCPadding(cfg) > 0); CHECK_EQ(fsConfig->config->blockSize % fsConfig->cipher->cipherBlockSize(), 0) << "FS block size must be multiple of cipher block size"; diff --git a/encfs/CipherFileIO.h b/encfs/CipherFileIO.h index b0ddb19d..a09d3281 100644 --- a/encfs/CipherFileIO.h +++ b/encfs/CipherFileIO.h @@ -90,6 +90,7 @@ class CipherFileIO : public BlockFileIO { std::shared_ptr cipher; CipherKey key; + bool haveCBCPadding; }; } // namespace encfs diff --git a/encfs/SSL_Cipher.cpp b/encfs/SSL_Cipher.cpp index d39a39cc..2635ccfb 100644 --- a/encfs/SSL_Cipher.cpp +++ b/encfs/SSL_Cipher.cpp @@ -159,9 +159,9 @@ int TimedPBKDF2(const char *pass, int passlen, const unsigned char *salt, // - Version 2:1 adds support for Message Digest function interface // - Version 2:2 adds PBKDF2 for password derivation // - Version 3:0 adds a new IV mechanism -static Interface BlowfishInterface("ssl/blowfish", 3, 0, 2); -static Interface AESInterface("ssl/aes", 3, 0, 2); -static Interface CAMELLIAInterface("ssl/camellia", 3, 0, 2); +static Interface BlowfishInterface("ssl/blowfish", 3, 1, 2); +static Interface AESInterface("ssl/aes", 3, 1, 2); +static Interface CAMELLIAInterface("ssl/camellia", 3, 1, 2); #ifndef OPENSSL_NO_CAMELLIA From 8e9e8b88c3620e36aaa551b73f27c37435550ea8 Mon Sep 17 00:00:00 2001 From: benrubson Date: Sat, 12 May 2018 16:13:49 +0200 Subject: [PATCH 02/14] Padding, size --- encfs/CipherFileIO.cpp | 46 +++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 0310374e..3ccc4fe4 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -168,18 +168,33 @@ int CipherFileIO::getAttr(struct stat *stbuf) const { // stat() the backing file int res = base->getAttr(stbuf); - // adjust size if we have a file header - if ((res == 0) && haveHeader && S_ISREG(stbuf->st_mode) && - (stbuf->st_size > 0)) { + // adjust size if we have a file header or padding + if ((res == 0) && S_ISREG(stbuf->st_mode)) { if (!fsConfig->reverseEncryption) { /* In normal mode, the upper file (plaintext) is smaller * than the backing ciphertext file */ - rAssert(stbuf->st_size >= HEADER_SIZE); - stbuf->st_size -= HEADER_SIZE; - } else { + if (haveHeader && (stbuf->st_size > 0)) { + /* Instead of using rAssert, we could also relax the rule and return 0. + * Think about a file which would have been partially written. + * Same for getSize() below. */ + rAssert(stbuf->st_size >= HEADER_SIZE); + stbuf->st_size -= HEADER_SIZE; + } + if (haveCBCPadding && (stbuf->st_size > 0)) { + rAssert(stbuf->st_size >= fsConfig->cipher->cipherBlockSize()); + stbuf->st_size -= fsConfig->cipher->cipherBlockSize(); + stbuf->st_size -= stbuf->st_size / (blockSize() + 1); + } + } else if (stbuf->st_size > 0) { + if (haveCBCPadding) { + stbuf->st_size += (stbuf->st_size - 1) / (blockSize() - 1); + stbuf->st_size += fsConfig->cipher->cipherBlockSize(); + } /* In reverse mode, the upper file (ciphertext) is larger than * the backing plaintext file */ - stbuf->st_size += HEADER_SIZE; + if (haveHeader) { + stbuf->st_size += HEADER_SIZE; + } } } @@ -194,11 +209,22 @@ off_t CipherFileIO::getSize() const { off_t size = base->getSize(); // No check on S_ISREG here -- don't call getSize over getAttr unless this // is a normal file! - if (haveHeader && size > 0) { - if (!fsConfig->reverseEncryption) { + if (!fsConfig->reverseEncryption) { + if (haveHeader && (size > 0)) { rAssert(size >= HEADER_SIZE); size -= HEADER_SIZE; - } else { + } + if (haveCBCPadding && (size > 0)) { + rAssert(size >= fsConfig->cipher->cipherBlockSize()); + size -= fsConfig->cipher->cipherBlockSize(); + size -= size / (blockSize() + 1); + } + } else if (size > 0) { + if (haveCBCPadding) { + size += (size - 1) / (blockSize() - 1); + size += fsConfig->cipher->cipherBlockSize(); + } + if (haveHeader) { size += HEADER_SIZE; } } From 3acb83b2f1ce52e6e0027780e78890ffd936c31b Mon Sep 17 00:00:00 2001 From: benrubson Date: Sat, 12 May 2018 16:14:07 +0200 Subject: [PATCH 03/14] Padding, read in normal mode --- encfs/BlockFileIO.cpp | 2 +- encfs/CipherFileIO.cpp | 73 +++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/encfs/BlockFileIO.cpp b/encfs/BlockFileIO.cpp index f78703d3..114bdea7 100644 --- a/encfs/BlockFileIO.cpp +++ b/encfs/BlockFileIO.cpp @@ -43,7 +43,7 @@ static void clearCache(IORequest &req, unsigned int blockSize) { BlockFileIO::BlockFileIO(unsigned int blockSize, const FSConfigPtr &cfg) : _blockSize(blockSize), _allowHoles(cfg->config->allowHoles) { CHECK(_blockSize > 1); - _cache.data = new unsigned char[_blockSize]; + _cache.data = new unsigned char[_blockSize + 1]; // we will need some room for padding _noCache = cfg->opts->noCache; } diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 3ccc4fe4..1c07a71c 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -383,26 +383,73 @@ int CipherFileIO::generateReverseHeader(unsigned char *headerBuf) { * Read block from backing plaintext file, then encrypt it (reverse mode) */ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const { + + int cbs = fsConfig->cipher->cipherBlockSize(); int bs = blockSize(); off_t blockNum = req.offset / bs; IORequest tmpReq = req; + // adjust offset and length if we pad + if (haveCBCPadding && !fsConfig->reverseEncryption) { + tmpReq.offset += tmpReq.offset / tmpReq.dataLen; + tmpReq.dataLen += 1; + } + // adjust offset if we have a file header if (haveHeader && !fsConfig->reverseEncryption) { tmpReq.offset += HEADER_SIZE; } + ssize_t readSize = base->read(tmpReq); - bool ok; - if (readSize > 0) { - if (haveHeader && fileIV == 0) { - int res = const_cast(this)->initHeader(); - if (res < 0) { - return res; + if (haveHeader && (fileIV == 0) && (readSize > 0)) { + int res = const_cast(this)->initHeader(); + if (res < 0) { + return res; + } + } + + bool ok = true; + + if (haveCBCPadding && (readSize >= 0)) { + if (!fsConfig->reverseEncryption) { + // remove the padding bytes which could have been added after the ciphertext + int padBytes = readSize % cbs; + readSize -= padBytes; + if (readSize > 0) { + ok = blockRead(tmpReq.data, (int)readSize, + blockNum ^ fileIV); // cast works because we work on a + // block and blocksize fit an int + if (ok) { + // we could have some padding bytes at the end of the plain data (X * 0x00) + padBytes = 0; + while ((padBytes < readSize) && (tmpReq.data[readSize - padBytes - 1] == 0x00)) { + padBytes++; + } + // there's the holes specific case + if (_allowHoles && (padBytes == readSize) && (readSize == (bs + 1))) { + readSize--; + // otherwise we should have at least one byte of data followed by the first padding byte (0x80) + } else if (((readSize - padBytes) > 1) && (tmpReq.data[readSize - padBytes - 1] == 0x80)) { + padBytes++; + readSize -= padBytes; + } else { + VLOG(1) << "readOneBlock failed (wrong padding) for block " << blockNum << ", size " + << readSize; + readSize = -EBADMSG; + } + } + } else { + VLOG(1) << "readSize zero (" << padBytes << " padBytes) for offset " << req.offset; } + } else { + // todo } + } + // no CBC padding, stream cipher instead + else if (readSize > 0) { if (readSize != bs) { VLOG(1) << "streamRead(data, " << readSize << ", IV)"; ok = streamRead(tmpReq.data, (int)readSize, @@ -413,16 +460,18 @@ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const { blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int } + } - if (!ok) { - VLOG(1) << "decodeBlock failed for block " << blockNum << ", size " - << readSize; - readSize = -EBADMSG; - } - } else if (readSize == 0) { + else if (readSize == 0) { VLOG(1) << "readSize zero for offset " << req.offset; } + if (!ok) { + VLOG(1) << "decodeBlock failed for block " << blockNum << ", size " + << readSize; + readSize = -EBADMSG; + } + return readSize; } From 22949fae526a1db0205c8b4d96cb298186f8741a Mon Sep 17 00:00:00 2001 From: benrubson Date: Sat, 12 May 2018 16:14:31 +0200 Subject: [PATCH 04/14] Padding, write in normal mode --- encfs/BlockFileIO.cpp | 2 +- encfs/CipherFileIO.cpp | 82 +++++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/encfs/BlockFileIO.cpp b/encfs/BlockFileIO.cpp index 114bdea7..b6b77621 100644 --- a/encfs/BlockFileIO.cpp +++ b/encfs/BlockFileIO.cpp @@ -43,7 +43,7 @@ static void clearCache(IORequest &req, unsigned int blockSize) { BlockFileIO::BlockFileIO(unsigned int blockSize, const FSConfigPtr &cfg) : _blockSize(blockSize), _allowHoles(cfg->config->allowHoles) { CHECK(_blockSize > 1); - _cache.data = new unsigned char[_blockSize + 1]; // we will need some room for padding + _cache.data = new unsigned char[_blockSize + 16]; // we will need some room for padding _noCache = cfg->opts->noCache; } diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 1c07a71c..b315e00b 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -483,10 +483,11 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { return -EPERM; } + int cbs = fsConfig->cipher->cipherBlockSize(); unsigned int bs = blockSize(); off_t blockNum = req.offset / bs; - if (haveHeader && fileIV == 0) { + if (haveHeader && (fileIV == 0)) { int res = initHeader(); if (res < 0) { return res; @@ -494,25 +495,80 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } bool ok; - if (req.dataLen != bs) { - ok = streamWrite(req.data, (int)req.dataLen, - blockNum ^ fileIV); // cast works because we work on a - // block and blocksize fit an int - } else { - ok = blockWrite(req.data, (int)req.dataLen, - blockNum ^ fileIV); // cast works because we work on a - // block and blocksize fit an int + + int padBytes = 0; + if (haveCBCPadding) { + if (!fsConfig->reverseEncryption) { + // we need padding bytes so that dataLen is a multiple of cipherBlockSize() + padBytes = cbs - (req.dataLen % cbs); + // we then add the first padding byte : 0x80 + req.data[req.dataLen] = 0x80; + // if needed, we add the other padding bytes : 0x00 + memset(req.data + req.dataLen + 1, 0x00, padBytes - 1); + ok = blockWrite(req.data, (int)req.dataLen + padBytes, + blockNum ^ fileIV); // cast works because we work on a + // block and blocksize fit an int + if (ok) { + /* here we use getSize() in write normal mode, we can safely assume that + * backing file has not been modified by others since we opened it + * (otherwise getSize() could return a wrong cached result). */ + if ((padBytes > 1) || (req.offset + bs >= getSize())) { + // this is the last block, we must pad it with cipherBlockSize() bytes + memset(req.data + req.dataLen + padBytes, 0x00, cbs - padBytes); + padBytes = cbs; + } + } + } else { + // todo + } + } + + else { // no CBC padding, stream cipher instead + if (req.dataLen != bs) { + ok = streamWrite(req.data, (int)req.dataLen, + blockNum ^ fileIV); // cast works because we work on a + // block and blocksize fit an int + } else { + ok = blockWrite(req.data, (int)req.dataLen, + blockNum ^ fileIV); // cast works because we work on a + // block and blocksize fit an int + } } ssize_t res = 0; if (ok) { + IORequest tmpReq = req; + if (haveCBCPadding) { + tmpReq.offset += tmpReq.offset / bs; + tmpReq.dataLen += padBytes; + } if (haveHeader) { - IORequest tmpReq = req; tmpReq.offset += HEADER_SIZE; - res = base->write(tmpReq); - } else { - res = base->write(req); } + res = base->write(tmpReq); + /* I first went with the following (interesting) commented block, but + * as base->write() returns -errno or dataLen, let's simplify it. */ + /* this was not the last block written (only one padding byte) + if (padBytes == 1) { + // padding byte has been written + if (res > req.dataLen) { + res--; + } + // this is the last block written (more than one padding byte) + } else if (padBytes > 0) { + // we wrote above the last previous block + padding + if ((req.offset + res) > (upperSize + cbs)) { + res -= cbs; + // we wrote up to somewhere inside the previous padding + } else if ((req.offset + res) > upperSize) { + res = upperSize - req.offset; + } + } */ + // Simple version : + if (res > 0) { + res -= padBytes; + } + } else { VLOG(1) << "encodeBlock failed for block " << blockNum << ", size " << req.dataLen; From e56bfa94b3a06ddcc24155f621b57a80e8d67c49 Mon Sep 17 00:00:00 2001 From: benrubson Date: Sat, 12 May 2018 16:14:45 +0200 Subject: [PATCH 05/14] Padding, truncate in normal mode --- encfs/CipherFileIO.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index b315e00b..01ab0a42 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -635,10 +635,10 @@ int CipherFileIO::truncate(off_t size) { } reopen = 1; } - if (!haveHeader) { + if (!haveHeader && !haveCBCPadding) { res = BlockFileIO::truncateBase(size, base.get()); } else { - if (0 == fileIV) { + if (haveHeader && (0 == fileIV)) { // empty file.. create the header.. res = initHeader(); } @@ -648,7 +648,13 @@ int CipherFileIO::truncate(off_t size) { res = BlockFileIO::truncateBase(size, nullptr); } if (res == 0) { - res = base->truncate(size + HEADER_SIZE); + if (haveCBCPadding && (size > 0)) { + size += (size - 1) / blockSize() + fsConfig->cipher->cipherBlockSize(); + } + if (haveHeader) { + size += HEADER_SIZE; + } + res = base->truncate(size); } } if (reopen == 1) { From 3c43cb03acedf4869dd0357eb99d4a88509ddda5 Mon Sep 17 00:00:00 2001 From: benrubson Date: Sun, 13 May 2018 09:36:07 +0200 Subject: [PATCH 06/14] Padding, read in reverse mode --- encfs/CipherFileIO.cpp | 50 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 01ab0a42..2d07c535 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -391,14 +391,23 @@ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const { IORequest tmpReq = req; // adjust offset and length if we pad - if (haveCBCPadding && !fsConfig->reverseEncryption) { - tmpReq.offset += tmpReq.offset / tmpReq.dataLen; - tmpReq.dataLen += 1; + if (haveCBCPadding) { + if (!fsConfig->reverseEncryption) { + tmpReq.offset += tmpReq.offset / tmpReq.dataLen; + tmpReq.dataLen += 1; + } else { + tmpReq.offset -= tmpReq.offset / tmpReq.dataLen; + tmpReq.dataLen -= 1; + } } // adjust offset if we have a file header - if (haveHeader && !fsConfig->reverseEncryption) { - tmpReq.offset += HEADER_SIZE; + if (haveHeader) { + if (!fsConfig->reverseEncryption) { + tmpReq.offset += HEADER_SIZE; + } else { + // done in CipherFileIO::read() + } } ssize_t readSize = base->read(tmpReq); @@ -443,8 +452,37 @@ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const { } else { VLOG(1) << "readSize zero (" << padBytes << " padBytes) for offset " << req.offset; } + } else if (readSize > 0) { + int padBytes = cbs - (readSize % cbs); + tmpReq.data[readSize] = 0x80; + memset(tmpReq.data + readSize + 1, 0x00, padBytes - 1); + readSize += padBytes; + ok = blockRead(tmpReq.data, (int)readSize, + blockNum ^ fileIV); // cast works because we work on a + // block and blocksize fit an int + if (ok) { + if (readSize < req.dataLen) { + // this is the last block, we must pad it with cipherBlockSize() bytes + memset(tmpReq.data + readSize, 0x00, cbs - padBytes); + readSize += cbs - padBytes; + } + } } else { - // todo + // This could be a request for the last padding bytes after the last ciphered block. + // we could use getSize() to find out, but as we reverse read, we can assume size + // could have changed since our last check, we would then get a wrong cached result. + // Let's simply try to read the previous block instead. + if (tmpReq.offset > 0) { + tmpReq.offset -= tmpReq.dataLen; + readSize = base->read(tmpReq); + } + if ((readSize > 0) && ((bs - readSize) < cbs)) { + readSize = cbs - (bs - readSize); + memset(tmpReq.data, 0x00, readSize); + } else { + readSize = 0; + VLOG(1) << "readSize zero for offset " << req.offset; + } } } From a76eae3b03fdec06c5f2841c27126e9d099fe47b Mon Sep 17 00:00:00 2001 From: benrubson Date: Sun, 13 May 2018 09:36:30 +0200 Subject: [PATCH 07/14] Padding, tests in reverse mode --- integration/reverse.t.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/reverse.t.pl b/integration/reverse.t.pl index f6df926a..d7f4e516 100755 --- a/integration/reverse.t.pl +++ b/integration/reverse.t.pl @@ -178,7 +178,7 @@ sub grow { # autoflush should make sure the write goes to the kernel # immediately. Just to be sure, check it here. sizeVerify($vfh, $i) or die("unexpected plain file size"); - sizeVerify($cfh, $i) or $ok = 0; + sizeVerify($cfh, $i + int(($i - 1) / 1023) + 16) or $ok = 0; sizeVerify($dfh, $i) or $ok = 0; if(md5fh($vfh) ne md5fh($dfh)) @@ -204,7 +204,7 @@ sub largeRead { my $cname = encName("largeRead"); # cfh ... ciphertext file handle ok(open(my $cfh, "<", "$ciphertext/$cname"), "open ciphertext largeRead file"); - ok(sizeVerify($cfh, 1024*1024), "1M file size"); + ok(sizeVerify($cfh, (1024 * 1024) + int((1024 * 1024 - 1) / 1023) + 16), "1M file size"); } # Check that the reverse mount is read-only From 1f67dc09c466faac86882d93543491159658a089 Mon Sep 17 00:00:00 2001 From: benrubson Date: Mon, 25 Jun 2018 09:33:03 +0200 Subject: [PATCH 08/14] Padding, use an enum to define padding types --- encfs/CipherFileIO.cpp | 18 +++++++----------- encfs/CipherFileIO.h | 2 ++ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 2d07c535..5d153ac3 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -60,27 +60,23 @@ const int HEADER_SIZE = 8; // 64 bit initialization vector.. * if the length of the last data block is not already a multiple of cipherBlockSize. * This allows to compute files' length without having to read the last block, at a cost of : * int((fileSize - 1) / (blockSize - 1)) + cipherBlockSize. - * This function returns : - * - 2 if we pad in reverse mode - * - 1 if we pad in normal mode - * - 0 if we don't pad - * The return value helps us in CipherFileIO initialization below to set the blockSize. + * The return value helps us in CipherFileIO initialization below to set the blockSize : * blockSize = blockSize - 1 in normal mode. */ -int checkCBCPadding(const FSConfigPtr &cfg) { +PaddingType checkCBCPadding(const FSConfigPtr &cfg) { if (((cfg->config->cipherIface.current() == 3) && (cfg->config->cipherIface.revision() >= 1)) || (cfg->config->cipherIface.current() > 3)) { if (cfg->reverseEncryption) { - return 2; + return Padding_Reverse; } - return 1; + return Padding_Normal; } - return 0; + return Padding_Disabled; } CipherFileIO::CipherFileIO(std::shared_ptr _base, const FSConfigPtr &cfg) - : BlockFileIO(cfg->config->blockSize - checkCBCPadding(cfg) % 2, cfg), + : BlockFileIO(cfg->config->blockSize - (checkCBCPadding(cfg) == Padding_Normal ? 1 : 0), cfg), base(std::move(_base)), haveHeader(cfg->config->uniqueIV), externalIV(0), @@ -89,7 +85,7 @@ CipherFileIO::CipherFileIO(std::shared_ptr _base, fsConfig = cfg; cipher = cfg->cipher; key = cfg->key; - haveCBCPadding = (checkCBCPadding(cfg) > 0); + haveCBCPadding = (checkCBCPadding(cfg) != Padding_Disabled); CHECK_EQ(fsConfig->config->blockSize % fsConfig->cipher->cipherBlockSize(), 0) << "FS block size must be multiple of cipher block size"; diff --git a/encfs/CipherFileIO.h b/encfs/CipherFileIO.h index a09d3281..22374185 100644 --- a/encfs/CipherFileIO.h +++ b/encfs/CipherFileIO.h @@ -38,6 +38,8 @@ class Cipher; class FileIO; struct IORequest; +enum PaddingType { Padding_Disabled, Padding_Normal, Padding_Reverse }; + /* Implement the FileIO interface encrypting data in blocks. From 189e27e5bf7f6c952a95a96d50f10495d8fc1690 Mon Sep 17 00:00:00 2001 From: benrubson Date: Mon, 25 Jun 2018 23:00:51 +0200 Subject: [PATCH 09/14] Padding, helpers to adjust sizes --- encfs/CipherFileIO.cpp | 106 ++++++++++++++++++++++------------------- encfs/CipherFileIO.h | 3 ++ 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 5d153ac3..25509de0 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -154,6 +154,49 @@ bool CipherFileIO::setIV(uint64_t iv) { return base->setIV(iv); } +/** + * Helper function to adjust plain size to cipher size + * according to configuration (file header, padding). + */ +void CipherFileIO::plainSizeToCipherSize(off_t *size) const { + if (*size > 0) { + if (haveCBCPadding) { + // for all the blocks but the last one, we add 1 padding byte to compute blockSize bytes blocks + // remember, in normal mode, blockSize is set to blockSize - 1 + *size += (*size - 1) / (blockSize() - (fsConfig->reverseEncryption ? 1 : 0)); + // for the last block we add cipherBlockSize padding bytes + *size += fsConfig->cipher->cipherBlockSize(); + } + /* In reverse mode, the upper file (ciphertext) is larger than + * the backing plaintext file */ + if (haveHeader) { + *size += HEADER_SIZE; + } + } +} + +/** + * Helper function to adjust cipher size to plain size + * according to configuration (file header, padding). + */ +void CipherFileIO::cipherSizeToPlainSize(off_t *size) const { + if (haveHeader && (*size > 0)) { + /* Instead of using rAssert, we could also relax the rule and return 0. + * Think about a file which could have been partially written. */ + rAssert(*size >= HEADER_SIZE); + *size -= HEADER_SIZE; + } + if (haveCBCPadding && (*size > 0)) { + // same note as above regarding rAssert + rAssert(*size >= fsConfig->cipher->cipherBlockSize()); + // we have cipherBlockSize bytes added to the last block, let's remove them + *size -= fsConfig->cipher->cipherBlockSize(); + // the remaining blocks have a 1 byte padding, let's remove them + // remember, in normal mode, blockSize is set to blockSize - 1 + *size -= *size / (blockSize() + (fsConfig->reverseEncryption ? 0 : 1)); + } +} + /** * Get file attributes (FUSE-speak for "stat()") for an upper file * Upper file = file we present to the user via FUSE @@ -164,36 +207,14 @@ int CipherFileIO::getAttr(struct stat *stbuf) const { // stat() the backing file int res = base->getAttr(stbuf); - // adjust size if we have a file header or padding + // let's adjust size if ((res == 0) && S_ISREG(stbuf->st_mode)) { if (!fsConfig->reverseEncryption) { - /* In normal mode, the upper file (plaintext) is smaller - * than the backing ciphertext file */ - if (haveHeader && (stbuf->st_size > 0)) { - /* Instead of using rAssert, we could also relax the rule and return 0. - * Think about a file which would have been partially written. - * Same for getSize() below. */ - rAssert(stbuf->st_size >= HEADER_SIZE); - stbuf->st_size -= HEADER_SIZE; - } - if (haveCBCPadding && (stbuf->st_size > 0)) { - rAssert(stbuf->st_size >= fsConfig->cipher->cipherBlockSize()); - stbuf->st_size -= fsConfig->cipher->cipherBlockSize(); - stbuf->st_size -= stbuf->st_size / (blockSize() + 1); - } - } else if (stbuf->st_size > 0) { - if (haveCBCPadding) { - stbuf->st_size += (stbuf->st_size - 1) / (blockSize() - 1); - stbuf->st_size += fsConfig->cipher->cipherBlockSize(); - } - /* In reverse mode, the upper file (ciphertext) is larger than - * the backing plaintext file */ - if (haveHeader) { - stbuf->st_size += HEADER_SIZE; - } + cipherSizeToPlainSize(&(stbuf->st_size)); + } else { + plainSizeToCipherSize(&(stbuf->st_size)); } } - return res; } @@ -205,23 +226,12 @@ off_t CipherFileIO::getSize() const { off_t size = base->getSize(); // No check on S_ISREG here -- don't call getSize over getAttr unless this // is a normal file! - if (!fsConfig->reverseEncryption) { - if (haveHeader && (size > 0)) { - rAssert(size >= HEADER_SIZE); - size -= HEADER_SIZE; - } - if (haveCBCPadding && (size > 0)) { - rAssert(size >= fsConfig->cipher->cipherBlockSize()); - size -= fsConfig->cipher->cipherBlockSize(); - size -= size / (blockSize() + 1); - } - } else if (size > 0) { - if (haveCBCPadding) { - size += (size - 1) / (blockSize() - 1); - size += fsConfig->cipher->cipherBlockSize(); - } - if (haveHeader) { - size += HEADER_SIZE; + // let's adjust size + if (size >= 0) { + if (!fsConfig->reverseEncryption) { + cipherSizeToPlainSize(&size); + } else { + plainSizeToCipherSize(&size); } } return size; @@ -682,11 +692,11 @@ int CipherFileIO::truncate(off_t size) { res = BlockFileIO::truncateBase(size, nullptr); } if (res == 0) { - if (haveCBCPadding && (size > 0)) { - size += (size - 1) / blockSize() + fsConfig->cipher->cipherBlockSize(); - } - if (haveHeader) { - size += HEADER_SIZE; + // let's adjust size + if (!fsConfig->reverseEncryption) { + plainSizeToCipherSize(&size); + } else { + cipherSizeToPlainSize(&size); } res = base->truncate(size); } diff --git a/encfs/CipherFileIO.h b/encfs/CipherFileIO.h index 22374185..a96396fd 100644 --- a/encfs/CipherFileIO.h +++ b/encfs/CipherFileIO.h @@ -77,6 +77,9 @@ class CipherFileIO : public BlockFileIO { bool blockWrite(unsigned char *buf, int size, uint64_t iv64) const; bool streamWrite(unsigned char *buf, int size, uint64_t iv64) const; + void plainSizeToCipherSize(off_t *size) const; + void cipherSizeToPlainSize(off_t *size) const; + ssize_t read(const IORequest &req) const; std::shared_ptr base; From d7373efc5b6d051acb09744701569216894de683 Mon Sep 17 00:00:00 2001 From: benrubson Date: Thu, 28 Jun 2018 22:56:23 +0200 Subject: [PATCH 10/14] Padding, configuration through expert mode --- encfs/CipherFileIO.cpp | 4 ++-- encfs/FSConfig.h | 3 +++ encfs/FileUtils.cpp | 28 ++++++++++++++++++++++------ encfs/encfs.pod | 32 ++++++++++++++++++++++---------- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 25509de0..03f69dcf 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -64,8 +64,8 @@ const int HEADER_SIZE = 8; // 64 bit initialization vector.. * blockSize = blockSize - 1 in normal mode. */ PaddingType checkCBCPadding(const FSConfigPtr &cfg) { - if (((cfg->config->cipherIface.current() == 3) && (cfg->config->cipherIface.revision() >= 1)) || - (cfg->config->cipherIface.current() > 3)) { + if ((((cfg->config->cipherIface.current() == 3) && (cfg->config->cipherIface.revision() >= 1)) || + (cfg->config->cipherIface.current() > 3)) && (cfg->config->padding)) { if (cfg->reverseEncryption) { return Padding_Reverse; } diff --git a/encfs/FSConfig.h b/encfs/FSConfig.h index 15b67b22..211dd92c 100644 --- a/encfs/FSConfig.h +++ b/encfs/FSConfig.h @@ -69,6 +69,8 @@ struct EncFSConfig { bool plainData; // do not encrypt file content + bool padding; // use CBC padding instead of cipher stream mode + int blockMACBytes; // MAC headers on blocks.. int blockMACRandBytes; // number of random bytes in the block header @@ -82,6 +84,7 @@ struct EncFSConfig { cfgType = Config_None; subVersion = 0; plainData = false; + padding = true; blockMACBytes = 0; blockMACRandBytes = 0; uniqueIV = false; diff --git a/encfs/FileUtils.cpp b/encfs/FileUtils.cpp index 0b5d43de..eda38259 100644 --- a/encfs/FileUtils.cpp +++ b/encfs/FileUtils.cpp @@ -320,6 +320,7 @@ bool readV6Config(const char *configFile, EncFSConfig *cfg, ConfigInfo *info) { config->read("blockSize", &cfg->blockSize); config->read("plainData", &cfg->plainData); + config->read("padding", &cfg->padding); config->read("uniqueIV", &cfg->uniqueIV); config->read("chainedNameIV", &cfg->chainedNameIV); config->read("externalIVChaining", &cfg->externalIVChaining); @@ -549,6 +550,7 @@ bool writeV6Config(const char *configFile, const EncFSConfig *cfg) { addEl(doc, config, "keySize", cfg->keySize); addEl(doc, config, "blockSize", cfg->blockSize); addEl(doc, config, "plainData", (int)cfg->plainData); + addEl(doc, config, "padding", (int)cfg->padding); addEl(doc, config, "uniqueIV", (int)cfg->uniqueIV); addEl(doc, config, "chainedNameIV", (int)cfg->chainedNameIV); addEl(doc, config, "externalIVChaining", (int)cfg->externalIVChaining); @@ -800,9 +802,9 @@ static int selectBlockSize(const Cipher::CipherAlgorithm &alg) { cout << autosprintf( // xgroup(setup) - _("Select a block size in bytes. The cipher you have chosen\n" - "supports sizes from %i to %i bytes in increments of %i.\n" - "Or just hit enter for the default (%i bytes)\n"), + _("Select a block size in bytes EncFS will internally deal with.\n" + "The cipher you have chosen supports sizes from %i to %i bytes\n" + "in increments of %i. Or just hit enter for the default (%i bytes)\n"), alg.blockSize.min(), alg.blockSize.max(), alg.blockSize.inc(), DefaultBlockSize); @@ -885,12 +887,23 @@ static bool selectPlainData(bool insecure) { if (insecure) { plainData = boolDefaultNo( _("You used --insecure, you can then disable file data encryption\n" - "which is of course abolutely discouraged.\n" - "Disable file data encryption?")); + "which is of course abolutely discouraged.\n" + "Disable file data encryption?")); } return plainData; } +/** + * Ask the user whether to use CBC padding or cipher stream mode for the last block + */ +static bool selectPadding() { + return ! boolDefaultNo( + _("Every block of data is padded and encrypted using CBC.\n" + "You can however, and at your own risk, use the old behavior which was\n" + "to use cipher stream mode for the last partial block, without any padding.\n" + "Use cipher stream mode?")); +} + /** * Ask the user whether to enable block MAC and random header bytes */ @@ -964,7 +977,7 @@ static bool selectChainedIV() { // xgroup(setup) return boolDefaultYes( _("Enable filename initialization vector chaining?\n" - "This makes filename encoding dependent on the complete path, \n" + "This makes filename encoding dependent on the complete path,\n" "rather then encoding each path element individually.")); } @@ -1037,6 +1050,7 @@ RootPtr createV6Config(EncFS_Context *ctx, int blockMACBytes = 0; // selectBlockMAC() int blockMACRandBytes = 0; // selectBlockMAC() bool plainData = false; // selectPlainData() + bool padding = true; // selectPadding() bool uniqueIV = true; // selectUniqueIV() bool chainedIV = true; // selectChainedIV() bool externalIV = false; // selectExternalChainedIV() @@ -1129,6 +1143,7 @@ RootPtr createV6Config(EncFS_Context *ctx, blockMACRandBytes = 0; } else { + padding = selectPadding(); if (reverseEncryption) { cout << _("reverse encryption - chained IV and MAC disabled") << "\n"; uniqueIV = selectUniqueIV(false); @@ -1173,6 +1188,7 @@ RootPtr createV6Config(EncFS_Context *ctx, config->keySize = keySize; config->blockSize = blockSize; config->plainData = plainData; + config->padding = padding; config->nameIface = nameIOIface; config->creator = "EncFS " VERSION; config->subVersion = V6SubVersion; diff --git a/encfs/encfs.pod b/encfs/encfs.pod index 6cb5b8f5..3269e066 100644 --- a/encfs/encfs.pod +++ b/encfs/encfs.pod @@ -419,7 +419,9 @@ In the expert / manual configuration mode, each of the above options is configurable. Here is a list of current options with some notes about what they mean: -=head1 Key Derivation Function +=over 4 + +=item I As of version 1.5, B now uses PBKDF2 as the default key derivation function. The number of iterations in the keying function is selected based on @@ -436,9 +438,7 @@ If an B filesystem configuration from 1.4.x is modified with version 1.5 function will be used and the filesystem will no longer be readable by older versions. -=over 4 - -=item I +=item I Which encryption algorithm to use. The list is generated automatically based on what supported algorithms B found in the encryption libraries. @@ -457,9 +457,8 @@ default) is probably overkill. =item I This is the size (in bytes) that B deals with at one time. Each block -gets its own initialization vector and is encoded in the cipher's -cipher-block-chaining mode. A partial block at the end of a file is encoded -using a stream mode to avoid having to store the filesize somewhere. +is padded (see below), gets its own initialization vector, and is encoded in +the cipher's cipher-block-chaining mode. Having larger block sizes reduces the overhead of B a little, but it can also add overhead if your programs read small parts of files. In order to read @@ -468,9 +467,22 @@ read and decoded, so a large block size adds overhead to small requests. With write calls it is even worse, as a block must be read and decoded, the change applied and the block encoded and written back out. -The default is 512 bytes as of version 1.0. It was hard coded to 64 bytes in -version 0.x, which was not as efficient as the current setting for general -usage. +The default is 1024 bytes as of version 1.4.2 (512 bytes as of version 1.0). +It was hard coded to 64 bytes in version 0.x, which was not as efficient as +the current setting for general usage. + +=item I + +As of 1.9.6, every block is padded with 1 byte before being encrypted in +CBC (cipher-block-chaining) mode. The last block is padded with the cipher +block sier (16 bytes with AES). This allows to compute the filesize without +having to store it somewhere. This however has a cost, in bytes, +for files > 0 byte : int((fileSize - 1) / (blockSize - 1)) + cipherBlockSize. + +Previous behavior was to use cipher stream mode for the last partial block, +without any padding. This also avoids to have to store the filesize, and has +no impact on it. However, it is known to have some potential security issue. +This can still be enabled in expert mode. =item I From 601b54db96e5d6df5725c95e468ff42030fea14b Mon Sep 17 00:00:00 2001 From: benrubson Date: Fri, 29 Jun 2018 18:55:43 +0200 Subject: [PATCH 11/14] Padding, tests with and without padding --- integration/normal.t.pl | 39 ++++++++++++-- integration/reverse.t.pl | 113 +++++++++++++++++++++++++++++---------- 2 files changed, 118 insertions(+), 34 deletions(-) diff --git a/integration/normal.t.pl b/integration/normal.t.pl index bb44da95..b057bb09 100755 --- a/integration/normal.t.pl +++ b/integration/normal.t.pl @@ -2,7 +2,7 @@ # Test EncFS normal and paranoid mode -use Test::More tests => 136; +use Test::More tests => 68*2*2; use File::Path; use File::Copy; use File::Temp; @@ -53,18 +53,30 @@ $sudo_cmd="sudo"; } -# test filesystem in standard config mode +# Configuration file related global variables +my $encfs6xml; +my $padding; + +# Test in standard mode, second call same conf but without padding +$padding = 1; +&runTests('standard'); +$padding = 0; &runTests('standard'); -# test in paranoia mode +# Test in paranoia mode, second call same conf but without padding +$padding = 1; +&runTests('paranoia'); +$padding = 0; &runTests('paranoia'); # Wrapper function - runs all tests in the specified mode sub runTests { my $mode = shift; - print STDERR "\nrunTests: mode=$mode sudo="; - print STDERR (defined($sudo_cmd) ? "on" : "off")."\n"; + print STDERR "\nrunTests: mode=$mode"; + print STDERR ($padding ? " padding=on" : " padding=off"); + print STDERR (defined($sudo_cmd) ? " sudo=on" : " sudo=off")."\n"; + &newWorkingDir; @@ -373,10 +385,27 @@ sub mount mkdir($crypt) || BAIL_OUT("Could not create $crypt: $!"); } + # Let's disable padding in our saved config and use it + if (!$padding) + { + $encfs6xml =~ s/1/0/; + open my $fh, ">", "$raw/.encfs6.xml"; + print $fh $encfs6xml; + close $fh; + } + delete $ENV{"ENCFS6_CONFIG"}; remount($args); ok( $? == 0, "encfs command returns 0") || BAIL_OUT(""); ok( -f "$raw/.encfs6.xml", "created control file") || BAIL_OUT(""); + + open my $fh, "<", "$raw/.encfs6.xml"; + $encfs6xml = do + { + local $/ = undef; + <$fh>; + }; + close $fh; } # Helper function diff --git a/integration/reverse.t.pl b/integration/reverse.t.pl index d7f4e516..1a0cc133 100755 --- a/integration/reverse.t.pl +++ b/integration/reverse.t.pl @@ -3,7 +3,7 @@ # Test EncFS --reverse mode use warnings; -use Test::More tests => 46; +use Test::More tests => 46*2*1; use File::Path; use File::Temp; use IO::Handle; @@ -39,6 +39,56 @@ $have_xattr = 0; } +# Configuration file related global variables +my $encfs6xml; +my $padding; + +# Test in standard mode, second call same conf but without padding +$padding = 1; +&runTests('standard'); +$padding = 0; +&runTests('standard'); + +# Test in paranoia mode, second call same conf but without padding +#$padding = 1; +#&runTests('paranoia'); +#$padding = 0; +#&runTests('paranoia'); + +# Wrapper function - runs all tests in the specified mode +sub runTests +{ + my $mode = shift; + print STDERR "\nrunTests: mode=$mode"; + print STDERR ($padding ? " padding=on" : " padding=off")."\n"; + + # Setup mounts + &newWorkingDir(); + &mount(); + + # Actual tests + &grow(); + &largeRead(); + ©_test(); + &encfsctl_cat_test(); + &symlink_test("/"); # absolute + &symlink_test("foo"); # relative + &symlink_test("/1/2/3/4/5/6/7/8/9/10/11/12/13/14/15/15/17/18"); # long + if ($^O ne "cygwin") + { + &symlink_test("!§\$%&/()\\<>#+="); # special characters + } + else + { + &symlink_test("!§\$%&/()//<>#+="); # special characters but without \ which is not Windows compliant + } # Absolute symlinks may failed on Windows : https://github.com/billziss-gh/winfsp/issues/153 + &symlink_test("$plain/foo"); + &writesDenied(); + + # Umount and delete files + &cleanup(); +} + # Helper function # Create a new empty working directory sub newWorkingDir @@ -85,6 +135,15 @@ sub cleanup # Directory structure: plain -[encrypt]-> ciphertext -[decrypt]-> decrypted sub mount { + # Let's disable padding in our saved config and use it + if (!$padding) + { + $encfs6xml =~ s/1/0/; + open my $fh, ">", "$plain/.encfs6.xml"; + print $fh $encfs6xml; + close $fh; + } + delete $ENV{"ENCFS6_CONFIG"}; system("./build/encfs --extpass=\"echo test\" --standard $plain $ciphertext --reverse --nocache"); ok(waitForFile("$plain/.encfs6.xml"), "plain .encfs6.xml exists") or BAIL_OUT("'$plain/.encfs6.xml'"); @@ -92,6 +151,14 @@ sub mount ok(waitForFile("$ciphertext/$e"), "encrypted .encfs6.xml exists") or BAIL_OUT("'$ciphertext/$e'"); system("ENCFS6_CONFIG=$plain/.encfs6.xml ./build/encfs --noattrcache --nodatacache --extpass=\"echo test\" $ciphertext $decrypted"); ok(waitForFile("$decrypted/.encfs6.xml"), "decrypted .encfs6.xml exists") or BAIL_OUT("'$decrypted/.encfs6.xml'"); + + open my $fh, "<", "$plain/.encfs6.xml"; + $encfs6xml = do + { + local $/ = undef; + <$fh>; + }; + close $fh; } # Helper function @@ -178,7 +245,14 @@ sub grow { # autoflush should make sure the write goes to the kernel # immediately. Just to be sure, check it here. sizeVerify($vfh, $i) or die("unexpected plain file size"); - sizeVerify($cfh, $i + int(($i - 1) / 1023) + 16) or $ok = 0; + if($padding) + { + sizeVerify($cfh, $i + int(($i - 1) / 1023) + 16) or $ok = 0; + } + else + { + sizeVerify($cfh, $i) or $ok = 0; + } sizeVerify($dfh, $i) or $ok = 0; if(md5fh($vfh) ne md5fh($dfh)) @@ -204,7 +278,14 @@ sub largeRead { my $cname = encName("largeRead"); # cfh ... ciphertext file handle ok(open(my $cfh, "<", "$ciphertext/$cname"), "open ciphertext largeRead file"); - ok(sizeVerify($cfh, (1024 * 1024) + int((1024 * 1024 - 1) / 1023) + 16), "1M file size"); + if($padding) + { + ok(sizeVerify($cfh, (1024 * 1024) + int((1024 * 1024 - 1) / 1023) + 16), "1M file size"); + } + else + { + ok(sizeVerify($cfh, 1024*1024), "1M file size"); + } } # Check that the reverse mount is read-only @@ -232,29 +313,3 @@ sub writesDenied { truncate($efn, 10); ok( $! == EROFS, "truncate denied, EROFS"); } - -# Setup mounts -newWorkingDir(); -mount(); - -# Actual tests -grow(); -largeRead(); -copy_test(); -encfsctl_cat_test(); -symlink_test("/"); # absolute -symlink_test("foo"); # relative -symlink_test("/1/2/3/4/5/6/7/8/9/10/11/12/13/14/15/15/17/18"); # long -if ($^O ne "cygwin") -{ - symlink_test("!§\$%&/()\\<>#+="); # special characters -} -else -{ - symlink_test("!§\$%&/()//<>#+="); # special characters but without \ which is not Windows compliant -} # Absolute symlinks may failed on Windows : https://github.com/billziss-gh/winfsp/issues/153 -symlink_test("$plain/foo"); -writesDenied(); - -# Umount and delete files -cleanup(); From 49a49992e04386452f9ce36c4fba1ae0e91af62b Mon Sep 17 00:00:00 2001 From: benrubson Date: Tue, 3 Jul 2018 17:50:16 +0200 Subject: [PATCH 12/14] Padding, use same wording in both read/writeOneBlock --- encfs/CipherFileIO.cpp | 46 ++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 03f69dcf..4c147018 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -467,7 +467,7 @@ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const { blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int if (ok) { - if (readSize < req.dataLen) { + if ((size_t)readSize < req.dataLen) { // this is the last block, we must pad it with cipherBlockSize() bytes memset(tmpReq.data + readSize, 0x00, cbs - padBytes); readSize += cbs - padBytes; @@ -531,6 +531,8 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { unsigned int bs = blockSize(); off_t blockNum = req.offset / bs; + ssize_t writeSize = req.dataLen; + if (haveHeader && (fileIV == 0)) { int res = initHeader(); if (res < 0) { @@ -540,26 +542,26 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { bool ok; - int padBytes = 0; if (haveCBCPadding) { if (!fsConfig->reverseEncryption) { - // we need padding bytes so that dataLen is a multiple of cipherBlockSize() - padBytes = cbs - (req.dataLen % cbs); + // we need padding bytes so that writeSize is a multiple of cipherBlockSize() + int padBytes = cbs - (writeSize % cbs); // we then add the first padding byte : 0x80 - req.data[req.dataLen] = 0x80; + req.data[writeSize] = 0x80; // if needed, we add the other padding bytes : 0x00 - memset(req.data + req.dataLen + 1, 0x00, padBytes - 1); - ok = blockWrite(req.data, (int)req.dataLen + padBytes, + memset(req.data + writeSize + 1, 0x00, padBytes - 1); + writeSize+= padBytes; + ok = blockWrite(req.data, (int)writeSize, blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int if (ok) { - /* here we use getSize() in write normal mode, we can safely assume that - * backing file has not been modified by others since we opened it - * (otherwise getSize() could return a wrong cached result). */ + /* here we use getSize() in write normal mode, we can safely assume that + * backing file has not been modified by others since we opened it + * (otherwise getSize() could return a wrong cached result). */ if ((padBytes > 1) || (req.offset + bs >= getSize())) { // this is the last block, we must pad it with cipherBlockSize() bytes - memset(req.data + req.dataLen + padBytes, 0x00, cbs - padBytes); - padBytes = cbs; + memset(req.data + writeSize, 0x00, cbs - padBytes); + writeSize += cbs - padBytes; } } } else { @@ -567,29 +569,29 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } } - else { // no CBC padding, stream cipher instead + // no CBC padding, stream cipher instead + else { if (req.dataLen != bs) { - ok = streamWrite(req.data, (int)req.dataLen, + ok = streamWrite(req.data, (int)writeSize, blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int } else { - ok = blockWrite(req.data, (int)req.dataLen, + ok = blockWrite(req.data, (int)writeSize, blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int } } - ssize_t res = 0; if (ok) { IORequest tmpReq = req; if (haveCBCPadding) { tmpReq.offset += tmpReq.offset / bs; - tmpReq.dataLen += padBytes; + tmpReq.dataLen = writeSize; } if (haveHeader) { tmpReq.offset += HEADER_SIZE; } - res = base->write(tmpReq); + writeSize = base->write(tmpReq); /* I first went with the following (interesting) commented block, but * as base->write() returns -errno or dataLen, let's simplify it. */ /* this was not the last block written (only one padding byte) @@ -609,16 +611,16 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } } */ // Simple version : - if (res > 0) { - res -= padBytes; + if (writeSize > 0) { + writeSize = req.dataLen; } } else { VLOG(1) << "encodeBlock failed for block " << blockNum << ", size " << req.dataLen; - res = -EBADMSG; + writeSize = -EBADMSG; } - return res; + return writeSize; } bool CipherFileIO::blockWrite(unsigned char *buf, int size, From 48691ee786c276e74670444370ea837560ab9850 Mon Sep 17 00:00:00 2001 From: benrubson Date: Thu, 12 Jul 2018 19:56:44 +0200 Subject: [PATCH 13/14] Padding, write in reverse mode --- encfs/BlockFileIO.cpp | 54 +++++++++++++----------- encfs/BlockFileIO.h | 1 + encfs/CipherFileIO.cpp | 93 +++++++++++++++++++++++++++--------------- 3 files changed, 91 insertions(+), 57 deletions(-) diff --git a/encfs/BlockFileIO.cpp b/encfs/BlockFileIO.cpp index b6b77621..7af19c29 100644 --- a/encfs/BlockFileIO.cpp +++ b/encfs/BlockFileIO.cpp @@ -45,6 +45,13 @@ BlockFileIO::BlockFileIO(unsigned int blockSize, const FSConfigPtr &cfg) CHECK(_blockSize > 1); _cache.data = new unsigned char[_blockSize + 16]; // we will need some room for padding _noCache = cfg->opts->noCache; + /* Even if cache is disabled, we may need to cache some data. + * This is the case in reverse mode, with CBC padding, partially writing the last block. + * If a write request is made with only some of the first bytes of the last block, + * the last bytes of this request won't certainly be properly written to disk (because of CBC mode). + * We must then cache these bytes so that subsequent request, which will certainly complete + * this last block, will properly retrieve them and finally write the full (or correctly padded) block. */ + _cachePartialWrite = cfg->reverseEncryption && cfg->config->padding; } BlockFileIO::~BlockFileIO() { @@ -65,9 +72,9 @@ ssize_t BlockFileIO::cacheReadOneBlock(const IORequest &req) const { /* we can satisfy the request even if _cache.dataLen is too short, because * we always request a full block during reads. This just means we are * in the last block of a file, which may be smaller than the blocksize. - * For reverse encryption, the cache must not be used at all, because + * For reverse encryption, the cache should not be used at all, because * the lower file may have changed behind our back. */ - if ((!_noCache) && (req.offset == _cache.offset) && (_cache.dataLen != 0)) { + if ((_cache.dataLen != 0) && (req.offset == _cache.offset)) { // satisfy request from cache size_t len = req.dataLen; if (_cache.dataLen < len) { @@ -76,23 +83,25 @@ ssize_t BlockFileIO::cacheReadOneBlock(const IORequest &req) const { memcpy(req.data, _cache.data, len); return len; } - if (_cache.dataLen > 0) { - clearCache(_cache, _blockSize); - } - // cache results of read -- issue reads for full blocks - IORequest tmp; - tmp.offset = req.offset; - tmp.data = _cache.data; - tmp.dataLen = _blockSize; - ssize_t result = readOneBlock(tmp); + // issue reads for full blocks, and keep result in cache if we're allowed to + _cache.offset = req.offset; + _cache.dataLen = _blockSize; + ssize_t result = readOneBlock(_cache); if (result > 0) { - _cache.offset = req.offset; _cache.dataLen = result; // the amount we really have if ((size_t)result > req.dataLen) { result = req.dataLen; // only as much as requested } memcpy(req.data, _cache.data, result); + if (!_noCache) { + // zero the last bytes of the cache + memset(_cache.data + _cache.dataLen, 0, _blockSize - _cache.dataLen); + } else { + clearCache(_cache, _blockSize); + } + } else { + clearCache(_cache, _blockSize); } return result; } @@ -101,19 +110,16 @@ ssize_t BlockFileIO::cacheWriteOneBlock(const IORequest &req) { // Let's point request buffer to our own buffer, as it may be modified by // encryption : originating process may not like to have its buffer modified memcpy(_cache.data, req.data, req.dataLen); - IORequest tmp; - tmp.offset = req.offset; - tmp.data = _cache.data; - tmp.dataLen = req.dataLen; - ssize_t res = writeOneBlock(tmp); - if (res < 0) { - clearCache(_cache, _blockSize); - } - else { - // And now we can cache the write buffer from the request + _cache.offset = req.offset; + _cache.dataLen = req.dataLen; + ssize_t res = writeOneBlock(_cache); + // we cache if we're allowed to or if we are writing the last partial block in CBC reverse mode + if ((res > 0) && (!_noCache || (_cachePartialWrite && (req.dataLen < _blockSize)))) { memcpy(_cache.data, req.data, req.dataLen); - _cache.offset = req.offset; - _cache.dataLen = req.dataLen; + // zero the last bytes of the cache + memset(_cache.data + req.dataLen, 0, _blockSize - req.dataLen); + } else { + clearCache(_cache, _blockSize); } return res; } diff --git a/encfs/BlockFileIO.h b/encfs/BlockFileIO.h index dfb15e2f..fe0d5a1d 100644 --- a/encfs/BlockFileIO.h +++ b/encfs/BlockFileIO.h @@ -62,6 +62,7 @@ class BlockFileIO : public FileIO { unsigned int _blockSize; bool _allowHoles; bool _noCache; + bool _cachePartialWrite; // cache last block for speed... mutable IORequest _cache; diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 4c147018..5f1466b1 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -540,7 +540,7 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } } - bool ok; + bool ok = true; if (haveCBCPadding) { if (!fsConfig->reverseEncryption) { @@ -565,7 +565,41 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } } } else { - // todo + // let's drop last bytes at a cipherBlockSize() modulo, + // they could be padding or result from a partial write + int padBytes = writeSize % cbs; + writeSize -= padBytes; + if (writeSize > 0) { + ok = blockWrite(req.data, (int)writeSize, + blockNum ^ fileIV); // cast works because we work on a + // block and blocksize fit an int + if (ok) { + padBytes = 0; + // let's look for the 0x00 padding bytes at the end inside the data block + while ((padBytes < (cbs - 1)) && (req.data[writeSize - padBytes - 1] == 0x00)) { + padBytes++; + } + // let's look for the first and needed padding byte, 0x80 + if (req.data[writeSize - padBytes - 1] == 0x80) { + padBytes++; + } else { + padBytes = 0; + } + // let's check if fully-sized block is correctly padded + if (writeSize == bs) { + if ((padBytes >= 1) && (padBytes <= cbs)) { + writeSize -= padBytes; + } else { + VLOG(1) << "writeOneBlock failed (wrong padding) for block " << blockNum << ", size " + << writeSize; + writeSize = -EBADMSG; + } + // for a partial block (could be a partial write), don't write what looks like padding + } else if ((int)((req.dataLen - writeSize) + padBytes) <= cbs) { + writeSize -= padBytes; + } + } + } } } @@ -582,44 +616,37 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } } - if (ok) { + if (!ok) { + VLOG(1) << "encodeBlock failed for block " << blockNum << ", size " + << req.dataLen; + writeSize = -EBADMSG; + } + + if (writeSize > 0) { IORequest tmpReq = req; - if (haveCBCPadding) { - tmpReq.offset += tmpReq.offset / bs; - tmpReq.dataLen = writeSize; - } - if (haveHeader) { - tmpReq.offset += HEADER_SIZE; - } - writeSize = base->write(tmpReq); - /* I first went with the following (interesting) commented block, but - * as base->write() returns -errno or dataLen, let's simplify it. */ - /* this was not the last block written (only one padding byte) - if (padBytes == 1) { - // padding byte has been written - if (res > req.dataLen) { - res--; + tmpReq.dataLen = writeSize; + if (!fsConfig->reverseEncryption) { + if (haveCBCPadding) { + tmpReq.offset += tmpReq.offset / bs; + } + if (haveHeader) { + tmpReq.offset += HEADER_SIZE; + } + } else { + if (haveHeader) { + tmpReq.offset -= HEADER_SIZE; } - // this is the last block written (more than one padding byte) - } else if (padBytes > 0) { - // we wrote above the last previous block + padding - if ((req.offset + res) > (upperSize + cbs)) { - res -= cbs; - // we wrote up to somewhere inside the previous padding - } else if ((req.offset + res) > upperSize) { - res = upperSize - req.offset; + if (haveCBCPadding) { + tmpReq.offset -= tmpReq.offset / bs; } - } */ - // Simple version : + } + writeSize = base->write(tmpReq); + // base->write() returns -errno or dataLen if (writeSize > 0) { writeSize = req.dataLen; } - - } else { - VLOG(1) << "encodeBlock failed for block " << blockNum << ", size " - << req.dataLen; - writeSize = -EBADMSG; } + return writeSize; } From caf589f41895d06132179f94a94df598d51f14a7 Mon Sep 17 00:00:00 2001 From: benrubson Date: Thu, 12 Jul 2018 23:50:42 +0200 Subject: [PATCH 14/14] Padding, same alg in normal read & reverse write --- encfs/CipherFileIO.cpp | 64 ++++++++++++++++++++++++------------------ encfs/FileUtils.cpp | 2 +- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/encfs/CipherFileIO.cpp b/encfs/CipherFileIO.cpp index 5f1466b1..faa894a5 100644 --- a/encfs/CipherFileIO.cpp +++ b/encfs/CipherFileIO.cpp @@ -430,33 +430,41 @@ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const { if (haveCBCPadding && (readSize >= 0)) { if (!fsConfig->reverseEncryption) { // remove the padding bytes which could have been added after the ciphertext - int padBytes = readSize % cbs; - readSize -= padBytes; + int padOutBytes = readSize % cbs; + readSize -= padOutBytes; if (readSize > 0) { ok = blockRead(tmpReq.data, (int)readSize, blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int if (ok) { // we could have some padding bytes at the end of the plain data (X * 0x00) - padBytes = 0; - while ((padBytes < readSize) && (tmpReq.data[readSize - padBytes - 1] == 0x00)) { - padBytes++; + int padInBytes = 0; + while ((padInBytes < (cbs - padOutBytes - 1)) && (tmpReq.data[readSize - padInBytes - 1] == 0x00)) { + padInBytes++; } - // there's the holes specific case - if (_allowHoles && (padBytes == readSize) && (readSize == (bs + 1))) { - readSize--; - // otherwise we should have at least one byte of data followed by the first padding byte (0x80) - } else if (((readSize - padBytes) > 1) && (tmpReq.data[readSize - padBytes - 1] == 0x80)) { - padBytes++; - readSize -= padBytes; + // let's look for the first and needed padding byte, 0x80 + if (tmpReq.data[readSize - padInBytes - 1] == 0x80) { + padInBytes++; } else { - VLOG(1) << "readOneBlock failed (wrong padding) for block " << blockNum << ", size " - << readSize; - readSize = -EBADMSG; + padInBytes = 0; + } + // let's check if fully-sized block is correctly padded + if (readSize == bs) { + if ((padInBytes >= 1) && (padInBytes <= cbs)) { + readSize -= padInBytes; + } else { + VLOG(1) << "readOneBlock failed (wrong padding) for block " << blockNum << ", size " + << readSize; + readSize = -EBADMSG; + } + // for a partial block, don't return what looks like padding + // could be a partially on-disk written block, for whatever reason (write failed, disk full...) + } else if ((padOutBytes + padInBytes) <= cbs) { + readSize -= padInBytes; } } } else { - VLOG(1) << "readSize zero (" << padBytes << " padBytes) for offset " << req.offset; + VLOG(1) << "readSize zero (" << padOutBytes << " padBytes) for offset " << req.offset; } } else if (readSize > 0) { int padBytes = cbs - (readSize % cbs); @@ -567,36 +575,36 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) { } else { // let's drop last bytes at a cipherBlockSize() modulo, // they could be padding or result from a partial write - int padBytes = writeSize % cbs; - writeSize -= padBytes; + int padOutBytes = writeSize % cbs; + writeSize -= padOutBytes; if (writeSize > 0) { ok = blockWrite(req.data, (int)writeSize, blockNum ^ fileIV); // cast works because we work on a // block and blocksize fit an int if (ok) { - padBytes = 0; + int padInBytes = 0; // let's look for the 0x00 padding bytes at the end inside the data block - while ((padBytes < (cbs - 1)) && (req.data[writeSize - padBytes - 1] == 0x00)) { - padBytes++; + while ((padInBytes < (cbs - padOutBytes - 1)) && (req.data[writeSize - padInBytes - 1] == 0x00)) { + padInBytes++; } // let's look for the first and needed padding byte, 0x80 - if (req.data[writeSize - padBytes - 1] == 0x80) { - padBytes++; + if (req.data[writeSize - padInBytes - 1] == 0x80) { + padInBytes++; } else { - padBytes = 0; + padInBytes = 0; } // let's check if fully-sized block is correctly padded if (writeSize == bs) { - if ((padBytes >= 1) && (padBytes <= cbs)) { - writeSize -= padBytes; + if ((padInBytes >= 1) && (padInBytes <= cbs)) { + writeSize -= padInBytes; } else { VLOG(1) << "writeOneBlock failed (wrong padding) for block " << blockNum << ", size " << writeSize; writeSize = -EBADMSG; } // for a partial block (could be a partial write), don't write what looks like padding - } else if ((int)((req.dataLen - writeSize) + padBytes) <= cbs) { - writeSize -= padBytes; + } else if ((padOutBytes + padInBytes) <= cbs) { + writeSize -= padInBytes; } } } diff --git a/encfs/FileUtils.cpp b/encfs/FileUtils.cpp index eda38259..7e9486f1 100644 --- a/encfs/FileUtils.cpp +++ b/encfs/FileUtils.cpp @@ -1054,7 +1054,7 @@ RootPtr createV6Config(EncFS_Context *ctx, bool uniqueIV = true; // selectUniqueIV() bool chainedIV = true; // selectChainedIV() bool externalIV = false; // selectExternalChainedIV() - bool allowHoles = true; // selectZeroBlockPassThrough() + bool allowHoles = false; // selectZeroBlockPassThrough() long desiredKDFDuration = NormalKDFDuration; if (reverseEncryption) {