D16134: Secure history file

[Martin Tobias Holmedahl Sandsmark]

sandsmark requested changes to this revision.
sandsmark added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> History.cpp:234
> +SecureHistoryFile::SecureHistoryFile() :
> +    _rcipher(QString::fromUtf8("aes128"), QCA::Cipher::CTR),
> +    _wcipher(QString::fromUtf8("aes128"), QCA::Cipher::CTR)

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

> History.cpp:237
> +{
> +    _iv_ref = QCA::Random::randomArray(8) + QCA::SecureArray(8, 0);
> +    _iv_counter = (((qint64_be*) _iv_ref.data()) + 1);

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?

> History.cpp:238
> +    _iv_ref = QCA::Random::randomArray(8) + QCA::SecureArray(8, 0);
> +    _iv_counter = (((qint64_be*) _iv_ref.data()) + 1);
> +    _key = QCA::SymmetricKey(16);

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

> History.cpp:240
> +    _key = QCA::SymmetricKey(16);
> +    _rbuf.reserve(0x400);
> +

why 1KB?

> History.cpp:248
> +    auto encrypted = _wcipher.update(QByteArray(buffer, count));
> +    Q_ASSERT(_wcipher.ok());
> +

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

> History.cpp:254
> +void SecureHistoryFile::get(char *buffer, qint64 size, qint64 loc) {
> +    qint64 base = loc & ~0x0F;
> +    qint64 off = loc & 0x0F;

what does this do

> History.cpp:255
> +    qint64 base = loc & ~0x0F;
> +    qint64 off = loc & 0x0F;
> +

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

> History.cpp:260
> +
> +    auto decrypted = decrypt_block(_rbuf, loc >> 4);
> +    memcpy(buffer, decrypted.constData() + off, size);

what type is decrypted?

> History.cpp:271
> +    auto decrypted = _rcipher.update(buf);
> +    Q_ASSERT(_rcipher.ok());
> +

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.

> History.h:92
> +private:
> +    QCA::MemoryRegion decrypt_block(QByteArray &buf, qint64 block_idx);
> +

camelCase

> main.cpp:91
> +    // Initialize QCA to enable SecureHistoryFile support
> +    QCA::Initializer init;
> +

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

REPOSITORY
  R319 Konsole

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

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