D16134: Secure history file

[Dórian Langbeck]

langbeck added inline comments.

INLINE COMMENTS

> sandsmark wrote in 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).

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?

> sandsmark wrote in History.cpp: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?

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.

> sandsmark wrote in History.cpp: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).

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.

> sandsmark wrote in History.cpp:240
> why 1KB?

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.

> sandsmark wrote in History.cpp:254
> what does this do

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.

> sandsmark wrote in History.cpp: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

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.

> sandsmark wrote in History.cpp:260
> what type is decrypted?

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

> sandsmark wrote in History.h:92
> camelCase

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

> sandsmark wrote in 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).

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

REPOSITORY
  R319 Konsole

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

To: langbeck, tcanabrava, hindenburg, #konsole, sandsmark
Cc: langbeck, ngraham, sandsmark, pino, frederico, konsole-devel, herrold, maximilianocuria, hindenburg
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20181015/d366b3f6/attachment.html>