Skip to content
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

Handle CBC padding #521

Closed
wants to merge 14 commits into from
50 changes: 44 additions & 6 deletions encfs/CipherFileIO.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -391,14 +391,23 @@ ssize_t CipherFileIO::readOneBlock(const IORequest &req) const {
IORequest tmpReq = req;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message is missing


// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this line do? Needs a comment.

tmpReq.dataLen += 1;
} else {
tmpReq.offset -= tmpReq.offset / tmpReq.dataLen;
tmpReq.dataLen -= 1;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use the remainder of the division of the offset by the request length.... Why? What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, tmpReq.dataLen == blockSize (each read / write request is blockSize bytes in length).
Remember that every block is padded with 1 byte.
We then add to the requested offset the number of backing padding bytes which exist before the upper position we want to read from.

// 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);
Expand Down Expand Up @@ -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;
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I feel like this has way too many levels of indentation. Maybe split the subsections into functions? Or at least add comments for what block starts at each "else {" ?


Expand Down