Secure history file
AbandonedPublic
Actions

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

Details

Reviewers

tcanabrava
sandsmark
langbeck

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.

In D16134#342047, @langbeck wrote:

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)

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).

In D16134#342488, @langbeck wrote:

@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?

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.

In D16134#343267, @langbeck wrote:

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?

In D16134#343445, @langbeck wrote:

@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).

In D16134#350909, @langbeck wrote:

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.

I don't think we should put this in Konsole. Instead, we should let users either do disk encryption or other means. They can change the location of Konsole's file to be saved there.

If we put this in Konsole, then we are responsible for verifying its security and keeping it secure.

Why CTR and not GCM (https://en.wikipedia.org/wiki/Galois/Counter_Mode)?

I don't know much about encryption, but GCM *might* protect from data corruption on disk. Please correct me if I'm wrong.

Feel free to mark https://bugs.kde.org/show_bug.cgi?id=416940 fixed as soon as this patch is submitted. You can add a line "BUG: 416940" to the commit message to have the ticket closed automatically on git push.

The suggested fix for this is to use an encrypted disk/folder/file for this. If we put this in Konsole, we are responsible for the security of it. I will save a copy of this patch for future reference as phabricator will be shutdown shortly.

hindenburg abandoned this revision.Jul 11 2020, 7:11 PM