Secure history file
Needs RevisionPublic

Authored by langbeck on Oct 11 2018, 4:24 PM.

Details

Reviewers
tcanabrava
hindenburg
sandsmark
Group Reviewers
Konsole
Summary

Create a secure version of HistoryFile (SecureHistoryFile) that encrypts the data, using an ephemeral key, before writing to the underlying file.
Also changes HistoryScrollFile to use a SecureHistoryFile (instead of normal HistoryFile) to keep _cells and _index secure.

Test Plan

Edit the Konsole profile to enable "Unlimited scrollback" and 2 (for _cells and _index) of 3 of the temporary files created will be encrypted with an ephemeral AES-128 key using CTR cipher mode.

Diff Detail

Repository
R319 Konsole
Lint
Lint Skipped
Unit
Unit Tests Skipped
langbeck created this revision.Oct 11 2018, 4:24 PM
Restricted Application added a project: Konsole. · View Herald TranscriptOct 11 2018, 4:24 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
langbeck requested review of this revision.Oct 11 2018, 4:24 PM

I'd prefer having QCA as an optional dependent. Did you do any benchmarks on if this slows Konsole down?

I'd say that if we get encryption for free *and* the operations is not noticiable slower, then it should be on by default without the need for a optional dependency.

I asked @frederico to run in an old notebook that he has with a Celeron u2300 1.2GHz running Debian Buster.
He used find in its home folder to test. After some time I asked him to scroll back and forth and tell if he noticed any difference.
Apparently he did not found any noticeable difference.

I asked @frederico to run in an old notebook that he has with a Celeron u2300 1.2GHz running Debian Buster.
He used find in its home folder to test. After some time I asked him to scroll back and forth and tell if he noticed any difference.
Apparently he did not found any noticeable difference.

As this is a change that *can* have some speed bumps, can you change the review to accept a option to enable / disable, so we can test and quickly disable if needed, and if this is proven not to cause data loss / speed loses, we remove the option in the future?

I need to double-check if frameworks/plasma require QCA; if they do, then it should be installed for Konsole

@hindenburg, I'm using Debian Buster and apt-cache shows that kde-runtime depends on libplasma3 that depends on libqca2.

Regarding @tcanabrava's request, I don't have experience with UI and I wouldn't know how to easily change the UI to show such option.
What I can do is add a build switch using a cmake option. But I don't know how to easily/elegantly add such code switch.

If you want manually enable/disable the feature in the code you just need to switch from

SecureHistoryFile _index; // lines Row(qint64)
SecureHistoryFile _cells; // text  Row(Character)
HistoryFile _lineflags; // flags Row(unsigned char)

to

HistoryFile _index; // lines Row(qint64)
HistoryFile _cells; // text  Row(Character)
HistoryFile _lineflags; // flags Row(unsigned char)

and that's it.

All the rest of the code will not be exercited if these two instances of SecureHistoryFile are removed (the performance effect is the same that without this feature enabled).

pino added a subscriber: pino.Oct 13 2018, 5:05 PM

@hindenburg, I'm using Debian Buster and apt-cache shows that kde-runtime depends on libplasma3 that depends on libqca2.

That is for kdelibs 4.x, not KF5. KF5 plasma does not use qca, the code for that was dropped as dead/unmaintained.

this is pretty useless if you have an encrypted disk, so I'd like it to be optional as well.

and maybe create a QBENCHMARK test in src/autotests/HistoryTest.cpp to get some concrete numbers?

sandsmark requested changes to this revision.Oct 14 2018, 4:52 PM

just did a quite read-through, but hindenburg has the final say if he disagrees with any of my requests.

src/History.cpp
234

is there any error handling in case this goes wrong? does it throw or assert?

passing in strings like that looks extremely error prone (and system config error prone), unfortunately most crypto APIs like that (botan is a bit better).

237

use camelCase not snake_case

and could probably do with a comment to explain what is happening and why. I guess you're generating 8 random bytes and padding with 8 zeroes?

238

I'm not sure I understand what does does, and that casting doesn't look safe (but I could be wrong, I just don't immediately understand what is happening).

240

why 1KB?

248

don't use assert here (same as below, I just read this review the wrong way).

254

what does this do

255

samesies

if it does what I think it does the usual (and more understandable, imho) way of doing this is to use division and modulo

260

what type is decrypted?

271

don't use an assert here, this isn't some error that just makes sense to check for when developing.

either handle it properly all the way to the top or just abort();, otherwise you very quickly get a false sense of security.

src/History.h
92

camelCase

src/main.cpp
91

make it locally static in the SecureHistory constructor, then we get less ifdefs (that is also threadsafe, fwiw, so shouldn't be any different from this).

This revision now requires changes to proceed.Oct 14 2018, 4:52 PM

I think this has to be cmake optional check and then a UI checkbox for user to disable. If you're not familiar w/ the UI, I or someone else can do that. We can try to get this in 18.12 though I see no need to rush it.

The 18.12 'not offical yet' schedule has Nov 8 as dependency freeze and string freeze Nov 15th

https://community.kde.org/Schedules/Applications/18.12_Release_Schedule

Rather than making it optional and user-facing, how about automatically encrypting the if the user does not have an encrypted disk/homedir (if we can detect that), and never encrypting if they do?

If the user has a encrypted disk but the disk is mounted, then this is
userfull to not allow other mallicious app in the same disk to peek into
the history file, no?

Em dom, 14 de out de 2018 às 23:57, Nathaniel Graham <
noreply@phabricator.kde.org> escreveu:

ngraham added a comment. View Revision
https://phabricator.kde.org/D16134

Rather than making it optional and user-facing, how about automatically
encrypting the if the user does not have an encrypted disk/homedir (if we
can detect that), and never encrypting if they do?

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D16134

*To: *langbeck, tcanabrava, hindenburg, Konsole, sandsmark
*Cc: *ngraham, sandsmark, pino, frederico, konsole-devel, herrold,
maximilianocuria, hindenburg

@sandsmark and @ngraham, these are temporary files and the disk where they are located will be always mounted and the file will not exists anymore when the computer is powered off (if the Konsole wasn't killed).
We don't need to protect those files in case someone steals your computer or something like. If that was the case, then I agree that disk encryption is the solution and this patch is useless.
What we need is to protect those files from malicious applications running in your system. In that case, there is nothing what disk encryption could do to help us.

Now that we have clarified what's the threat model this patch is trying to prevent, lets move on to the "Qt5 dropping the use of QCA" problem.
I think I can replace QCA with libssl (that is used by libqt5network5) at some point during the next week or two.

I'll address @sandsmark's code review comments now.

pino added a comment.Oct 15 2018, 4:30 AM

I think I can replace QCA with libssl (that is used by libqt5network5) at some point during the next week or two.

No, please do not use openssl (or any other cryption lbrary) directly, especially when there is already qca leveraging its use.
Using it directly means playing with a costantly-changing & difficult-to-use API, that poses also more licensing burdens (the "advertizing clause").

langbeck added inline comments.Oct 15 2018, 12:24 PM
src/History.cpp
234

The default provider (OpenSSL) does support AES128 and will never fails.
I can try do address this unlikely-but-still-possible error case during migration to OpenSSL.

Note that this solution depends that the cipher has 128 bits block size and the algorithm will not be available to be configured by the user.
The code can be changed to work with any block size, but it will require more code.
Such change can be made later in a second pull-request. What do you think?

237

Yes, you are right. The zeros is the counter part of the IV.
Remembering that the IV size is 16 bytes because 16 is the block size of AES.

238

This is just accessing the counter part of of the IV (last 64 bits).
The counter must be big-endian to allow random single-block read match with random multi-block reads.

240

Just a buffer size that's larger than the bigger read request that I saw in my machine during my tests.
But this value is just an initial guess to minimize unecessary allocations.

254

This is only clearing the lower 4 bits of the initial cell address.
The result is the address of the block that contains the cell that we want.

255

When the modulo is a power of 2, use mask is the way to go (and that's the case for AES128).
These operations are well known among those who already worked with memory paging.

260

It's QCA::MemoryRegion, as can be seen just 4 lines bellow.

src/History.h
92

I'll keep that in mind when I change the code to move from QCA to libssl.

src/main.cpp
91

I wasn't remembering of locally static variables. Great idea.

@pino, but you had just said that "KF5 plasma does not use qca, the code for that was dropped as dead/unmaintained".
I just thought in using OpenSSL directly because you said that.
So, should I use QCA or not?

And, if libqt5network5 is already using it then we should not have "licensing burdens", right?

Since nobody seems to see/understand the importance of this patch:
Should I go ahead and abandon this revision?

pino added a comment.Oct 30 2018, 11:55 AM

@pino, but you had just said that "KF5 plasma does not use qca, the code for that was dropped as dead/unmaintained".

I meant that the code that in plasma 4 used qca was removed, because that code was unmaintained.

I just thought in using OpenSSL directly because you said that.

Definitely not what I meant.

So, should I use QCA or not?

If you need crypto, yes.

And, if libqt5network5 is already using it then we should not have "licensing burdens", right?

That is another story (dynamic loading of openssl at runtime).

Since nobody seems to see/understand the importance of this patch:
Should I go ahead and abandon this revision?

There were changes requested, and they are not done. I'd start by doing the changes according to the reviews.