Skip to content

Harden secret handling #1453

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.

Sign up for GitHub

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

Open
metsma wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Harden secret handling #1453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 50 additions & 12 deletions client/CDocSupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,15 @@
#include <QtCore/QBuffer>
#include <QtCore/QJsonDocument>
#include <QtCore/QJsonObject>
#include <QtCore/QMessageAuthenticationCode>
#include <QtCore/QtEndian>
#include <QtCore/QTemporaryFile>
#include <QtCore/QUrlQuery>
#if QT_VERSION >= QT_VERSION_CHECK(6, 8, 0)
#include <QtCore/QSpan>
#endif
#include <QtNetwork/QNetworkAccessManager>
#include <QtNetwork/QPasswordDigestor>
#include <QtNetwork/QSslKey>
#include <QLoggingCategory>
#include <QXmlStreamReader>
Expand All @@ -49,10 +54,14 @@ QDebug operator<<(QDebug d, std::string_view str) {
}
#endif

static QByteArray toByteArray(const std::vector<uint8_t> &data) {
static QByteArray asByteArray(const std::vector<uint8_t> &data) {
return QByteArray::fromRawData(reinterpret_cast<const char *>(data.data()), data.size());
}

static QByteArray asByteArray(QByteArrayView data) {
return QByteArray::fromRawData(data.data(), data.size());
}

std::vector<libcdoc::FileInfo>
CDocSupport::getCDocFileList(const QString &filename)
{
Expand Down Expand Up @@ -129,8 +138,7 @@ DDCryptoBackend::decryptRSA(std::vector<uint8_t>& dst, const std::vector<uint8_t
return getDecryptStatus(val.error());
backend.reset(val.value());
}
QByteArray decryptedKey = backend->decrypt(toByteArray(data), oaep);
dst.assign(decryptedKey.cbegin(), decryptedKey.cend());
dst = backend->decrypt(asByteArray(data), oaep);
return getDecryptResultStatus(dst, std::move(backend));
}

Expand All @@ -149,9 +157,8 @@ DDCryptoBackend::deriveConcatKDF(std::vector<uint8_t>& dst, const std::vector<ui
return getDecryptStatus(val.error());
backend.reset(val.value());
}
QByteArray decryptedKey = backend->deriveConcatKDF(toByteArray(publicKey), SHA_MTH.value(digest),
toByteArray(algorithmID), toByteArray(partyUInfo), toByteArray(partyVInfo));
dst.assign(decryptedKey.cbegin(), decryptedKey.cend());
dst = backend->deriveConcatKDF(asByteArray(publicKey), SHA_MTH.value(digest),
asByteArray(algorithmID), asByteArray(partyUInfo), asByteArray(partyVInfo));
return getDecryptResultStatus(dst, std::move(backend));
}

Expand All @@ -164,15 +171,46 @@ DDCryptoBackend::deriveHMACExtract(std::vector<uint8_t>& dst, const std::vector<
return getDecryptStatus(val.error());
backend.reset(val.value());
}
QByteArray decryptedKey = backend->deriveHMACExtract(toByteArray(key_material), toByteArray(salt), ECC_KEY_LEN);
dst.assign(decryptedKey.cbegin(), decryptedKey.cend());
dst = backend->deriveHMACExtract(asByteArray(key_material), asByteArray(salt), ECC_KEY_LEN);
return getDecryptResultStatus(dst, std::move(backend));
}

libcdoc::result_t
DDCryptoBackend::getSecret(std::vector<uint8_t>& _secret, unsigned int idx)
DDCryptoBackend::extractHKDF(std::vector<uint8_t> &dst, const std::vector<uint8_t> &salt,
const std::vector<uint8_t> &pwSalt, int32_t kdfIter, unsigned int idx)
{
_secret = secret;
if(secret.isEmpty() || salt.empty() || kdfIter < 0 || (kdfIter > 0 && pwSalt.empty()))
return INVALID_PARAMS;

QByteArray derivedKeyMaterial;
auto keyMaterialGuard = qScopeGuard([&] { derivedKeyMaterial.fill(0); });
QByteArrayView keyMaterial(secret);
if(kdfIter > 0) {
derivedKeyMaterial = QPasswordDigestor::deriveKeyPbkdf2(QCryptographicHash::Sha256,
secret, asByteArray(pwSalt), kdfIter, 32);
keyMaterial = QByteArrayView(derivedKeyMaterial);
} else if(secret.size() != 32) {
return INVALID_PARAMS;
}
if(keyMaterial.size() != 32)
return libcdoc::CRYPTO_ERROR;

#if QT_VERSION >= QT_VERSION_CHECK(6, 8, 0)
dst.resize(32);
const QByteArrayView result = QMessageAuthenticationCode::hashInto(
dst, keyMaterial, salt, QCryptographicHash::Sha256);
if(result.size() != 32) {
std::fill(dst.begin(), dst.end(), 0);
dst.clear();
return libcdoc::CRYPTO_ERROR;
}
#else
QByteArray kekPreMaster = QMessageAuthenticationCode::hash(asByteArray(keyMaterial), asByteArray(salt), QCryptographicHash::Sha256);
auto kekPreMasterGuard = qScopeGuard([&] { kekPreMaster.fill(0); });
if(kekPreMaster.size() != 32)
return libcdoc::CRYPTO_ERROR;
dst.assign(kekPreMaster.cbegin(), kekPreMaster.cend());
#endif
return libcdoc::OK;
}

Expand Down Expand Up @@ -278,8 +316,8 @@ libcdoc::result_t DDNetworkBackend::sendKey(
}
QScopedPointer<QNetworkAccessManager, QScopedPointerDeleteLater> nam(CheckConnection::setupNAM(req, Settings::CDOC2_POST_CERT));
QNetworkReply *reply = nam->post(req, QJsonDocument({
{QLatin1String("recipient_id"), QLatin1String(toByteArray(rcpt_key).toBase64())},
{QLatin1String("ephemeral_key_material"), QLatin1String(toByteArray(key_material).toBase64())},
{QLatin1String("recipient_id"), QLatin1String(asByteArray(rcpt_key).toBase64())},
{QLatin1String("ephemeral_key_material"), QLatin1String(asByteArray(key_material).toBase64())},
{QLatin1String("capsule_type"), QLatin1String(type.c_str())},
}).toJson());
QEventLoop e;
Expand Down
17 changes: 14 additions & 3 deletions client/CDocSupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,29 @@ struct DDCryptoBackend final : public libcdoc::CryptoBackend {
const std::vector<uint8_t> &publicKey,
const std::vector<uint8_t> &salt,
unsigned int idx) final;
libcdoc::result_t getSecret(std::vector<uint8_t> &secret,
unsigned int idx) final;
libcdoc::result_t extractHKDF(std::vector<uint8_t> &dst,
const std::vector<uint8_t> &salt,
const std::vector<uint8_t> &pwSalt,
int32_t kdfIter,
unsigned int idx) final;
std::string getLastErrorStr(libcdoc::result_t code) const final;

std::unique_ptr<QCryptoBackend> backend;
std::vector<uint8_t> secret;
QByteArray secret;

explicit DDCryptoBackend() = default;

void setBackend(std::unique_ptr<QCryptoBackend> &&backend) {
this->backend = std::move(backend);
}
void setSecret(QByteArray &&value) {
clearSecret();
secret = std::move(value);
}
void clearSecret() {
secret.fill(0);
secret.clear();
}
};

//
Expand Down
25 changes: 17 additions & 8 deletions client/CryptoDoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,12 @@ ContainerState CryptoDoc::state() const
return d->isEncrypted() ? EncryptedContainer : UnencryptedContainer;
}

bool CryptoDoc::decrypt(const libcdoc::Lock *lock, const QByteArray& secret)
bool CryptoDoc::decrypt(const libcdoc::Lock *lock, QByteArray secret)
{
auto secretGuard = qScopeGuard([&] {
d->crypto.clearSecret();
secret.fill(0);
});
if(!d->reader)
{
WarningDialog::create()
Expand Down Expand Up @@ -321,8 +325,7 @@ bool CryptoDoc::decrypt(const libcdoc::Lock *lock, const QByteArray& secret)
return false;
}

d->crypto.secret.assign(secret.cbegin(), secret.cend());

d->crypto.setSecret(std::move(secret));
TempListConsumer cons;
libcdoc::result_t result = waitFor([&]{
std::vector<uint8_t> fmk;
Expand All @@ -337,6 +340,7 @@ bool CryptoDoc::decrypt(const libcdoc::Lock *lock, const QByteArray& secret)
qCDebug(CRYPTO) << "Decryption result: " << result << " " << QString::fromStdString(d->reader->getLastErrorStr());
return result;
});
d->crypto.clearSecret();
if (result != libcdoc::OK) {
QString str;
const std::string &msg = d->reader->getLastErrorStr();
Expand Down Expand Up @@ -390,8 +394,13 @@ bool CryptoDoc::decrypt(const libcdoc::Lock *lock, const QByteArray& secret)

DocumentModel *CryptoDoc::documentModel() const { return d->documents; }

bool CryptoDoc::encrypt(const QString &filename, const QString& label, const QByteArray& secret)
bool CryptoDoc::encrypt(const QString &filename, const QString& label, QByteArray secret)
{
const bool encryptWithPassword = !secret.isEmpty();
auto secretGuard = qScopeGuard([&] {
d->crypto.clearSecret();
secret.fill(0);
});
// I think the correct semantics is to fail if container is already encrypted
if(d->reader)
return false;
Expand All @@ -405,7 +414,7 @@ bool CryptoDoc::encrypt(const QString &filename, const QString& label, const QBy
->open();
return false;
}
if(secret.isEmpty() && d->keys.empty())
if(!encryptWithPassword && d->keys.empty())
{
WarningDialog::create()
->withTitle(tr("Failed to encrypt document"))
Expand All @@ -414,6 +423,7 @@ bool CryptoDoc::encrypt(const QString &filename, const QString& label, const QBy
return false;
}
QString writer_last_error;
d->crypto.setSecret(std::move(secret));
libcdoc::result_t result = waitFor([&] -> libcdoc::result_t {
qCDebug(CRYPTO) << "Encrypt" << d->fileName;
auto writer = std::unique_ptr<libcdoc::CDocWriter>(libcdoc::CDocWriter::createWriter(d->version, d->fileName.toStdString(), &d->conf, &d->crypto, &d->network));
Expand Down Expand Up @@ -442,9 +452,8 @@ bool CryptoDoc::encrypt(const QString &filename, const QString& label, const QBy
}
}
// Encrypt with symmetric key
if (!secret.isEmpty()) {
if (encryptWithPassword) {
// NIST recommends at least 600000 iterations for PBKDF2 with SHA-256, see https://csrc.nist.gov/publications/detail/sp/800-132/final
d->crypto.secret.assign(secret.cbegin(), secret.cend());
libcdoc::Recipient rcpt = libcdoc::Recipient::makeSymmetric({}, 600000);
rcpt.setLabelValue(libcdoc::CDoc2::Label::LABEL, label.toStdString());
enc_keys.push_back(std::move(rcpt));
Expand All @@ -458,7 +467,7 @@ bool CryptoDoc::encrypt(const QString &filename, const QString& label, const QBy
QFile::remove(d->fileName);
return result;
});
d->crypto.secret.clear();
d->crypto.clearSecret();
if (result != libcdoc::OK) {
WarningDialog::create()
->withTitle(tr("Failed to encrypt document"))
Expand Down
4 changes: 2 additions & 2 deletions client/CryptoDoc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ class CryptoDoc final: public QObject
bool addEncryptionKey(const CKey& key);
bool canDecrypt(const QSslCertificate &cert);
void clear(const QString &file = {}, int version = -1);
bool decrypt(const libcdoc::Lock *lock, const QByteArray& secret);
bool encrypt(const QString &filename = {}, const QString& label = {}, const QByteArray& secret = {});
bool decrypt(const libcdoc::Lock *lock, QByteArray secret);
bool encrypt(const QString &filename = {}, const QString& label = {}, QByteArray secret = {});
DocumentModel* documentModel() const;
QString fileName() const;
const std::vector<CKey>& keys() const;
Expand Down
33 changes: 18 additions & 15 deletions client/QCNG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include <QtCore/QRegularExpression>
#include <QtNetwork/QSslKey>

#include <algorithm>

using namespace Qt::Literals::StringLiterals;

Q_LOGGING_CATEGORY(CNG, "qdigidoc4.QCNG")
Expand Down Expand Up @@ -67,9 +69,9 @@ QCNG::Status QCNG::login(const TokenData &token)
return PinOK;
}

QByteArray QCNG::decrypt(const QByteArray &data, bool oaep) const
std::vector<uint8_t> QCNG::decrypt(const QByteArray &data, bool oaep) const
{
return exec([&](NCRYPT_PROV_HANDLE prov, NCRYPT_KEY_HANDLE key, QByteArray &result) {
return exec<std::vector<uint8_t>>([&](NCRYPT_PROV_HANDLE prov, NCRYPT_KEY_HANDLE key, std::vector<uint8_t> &result) {
BCRYPT_OAEP_PADDING_INFO padding {BCRYPT_SHA256_ALGORITHM, nullptr, 0};
PVOID paddingInfo = oaep ? &padding : nullptr;
DWORD flags = oaep ? NCRYPT_PAD_OAEP_FLAG : NCRYPT_PAD_PKCS1_FLAG;
Expand All @@ -78,19 +80,19 @@ QByteArray QCNG::decrypt(const QByteArray &data, bool oaep) const
paddingInfo, nullptr, 0, &size, flags);
if(FAILED(err))
return err;
result.resize(int(size));
result.resize(size);
err = NCryptDecrypt(key, PBYTE(data.constData()), DWORD(data.size()),
paddingInfo, PBYTE(result.data()), DWORD(result.size()), &size, flags);
if(SUCCEEDED(err))
result.resize(int(size));
result.resize(size);
return err;
});
}

template<typename F>
QByteArray QCNG::derive(const QByteArray &publicKey, F &&func) const
std::vector<uint8_t> QCNG::derive(const QByteArray &publicKey, F &&func) const
{
return exec([&](NCRYPT_PROV_HANDLE prov, NCRYPT_KEY_HANDLE key, QByteArray &derived) {
return exec<std::vector<uint8_t>>([&](NCRYPT_PROV_HANDLE prov, NCRYPT_KEY_HANDLE key, std::vector<uint8_t> &derived) {
BCRYPT_ECCKEY_BLOB oh { BCRYPT_ECDH_PUBLIC_P384_MAGIC, ULONG((publicKey.size() - 1) / 2) };
switch((publicKey.size() - 1) * 4)
{
Expand All @@ -111,10 +113,10 @@ QByteArray QCNG::derive(const QByteArray &publicKey, F &&func) const
});
}

QByteArray QCNG::deriveConcatKDF(const QByteArray &publicKey, QCryptographicHash::Algorithm digest,
std::vector<uint8_t> QCNG::deriveConcatKDF(const QByteArray &publicKey, QCryptographicHash::Algorithm digest,
const QByteArray &algorithmID, const QByteArray &partyUInfo, const QByteArray &partyVInfo) const
{
return derive(publicKey, [&](NCRYPT_SECRET_HANDLE sharedSecret, QByteArray &derived) {
return derive(publicKey, [&](NCRYPT_SECRET_HANDLE sharedSecret, std::vector<uint8_t> &derived) {
std::array paramValues{
BCryptBuffer{ULONG(algorithmID.size()), KDF_ALGORITHMID, PBYTE(algorithmID.data())},
BCryptBuffer{ULONG(partyUInfo.size()), KDF_PARTYUINFO, PBYTE(partyUInfo.data())},
Expand All @@ -135,16 +137,16 @@ QByteArray QCNG::deriveConcatKDF(const QByteArray &publicKey, QCryptographicHash
SECURITY_STATUS err {};
if(FAILED(err = NCryptDeriveKey(sharedSecret, BCRYPT_KDF_SP80056A_CONCAT, &params, nullptr, 0, &size, 0)))
return err;
derived.resize(int(size));
derived.resize(size);
if(SUCCEEDED(err = NCryptDeriveKey(sharedSecret, BCRYPT_KDF_SP80056A_CONCAT, &params, PBYTE(derived.data()), size, &size, 0)))
derived.resize(32);
return err;
});
}

QByteArray QCNG::deriveHMACExtract(const QByteArray &publicKey, const QByteArray &salt, int keySize) const
std::vector<uint8_t> QCNG::deriveHMACExtract(const QByteArray &publicKey, const QByteArray &salt, int keySize) const
{
return derive(publicKey, [&](NCRYPT_SECRET_HANDLE sharedSecret, QByteArray &derived) {
return derive(publicKey, [&](NCRYPT_SECRET_HANDLE sharedSecret, std::vector<uint8_t> &derived) {
std::array paramValues{
BCryptBuffer{ULONG(salt.size()), KDF_HMAC_KEY, PBYTE(salt.data())},
BCryptBuffer{ULONG(sizeof(BCRYPT_SHA256_ALGORITHM)), KDF_HASH_ALGORITHM, PBYTE(BCRYPT_SHA256_ALGORITHM)},
Expand All @@ -154,20 +156,20 @@ QByteArray QCNG::deriveHMACExtract(const QByteArray &publicKey, const QByteArray
SECURITY_STATUS err = 0;
if(FAILED(err = NCryptDeriveKey(sharedSecret, BCRYPT_KDF_HMAC, &params, nullptr, 0, &size, 0)))
return err;
derived.resize(int(size));
derived.resize(size);
if(SUCCEEDED(err = NCryptDeriveKey(sharedSecret, BCRYPT_KDF_HMAC, &params, PBYTE(derived.data()), size, &size, 0)))
derived.resize(keySize);
return err;
});
}

template<typename F>
QByteArray QCNG::exec(F &&func) const
template<typename Result, typename F>
Result QCNG::exec(F &&func) const
{
if (!d)
return {};
status = UnknownError;
QByteArray result;
Result result;
switch(func(d->prov, d->key, result))
{
case ERROR_SUCCESS:
Expand All @@ -177,6 +179,7 @@ QByteArray QCNG::exec(F &&func) const
case ERROR_CANCELLED:
status = PinCanceled;
default:
std::fill(result.begin(), result.end(), 0);
return {};
}
}
Expand Down
12 changes: 6 additions & 6 deletions client/QCNG.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,18 @@ class QCNG final: public QCryptoBackend

Status login(const TokenData &token) final;

QByteArray decrypt(const QByteArray &data, bool oaep) const final;
QByteArray deriveConcatKDF(const QByteArray &publicKey, QCryptographicHash::Algorithm digest,
std::vector<uint8_t> decrypt(const QByteArray &data, bool oaep) const final;
std::vector<uint8_t> deriveConcatKDF(const QByteArray &publicKey, QCryptographicHash::Algorithm digest,
const QByteArray &algorithmID, const QByteArray &partyUInfo, const QByteArray &partyVInfo) const final;
QByteArray deriveHMACExtract(const QByteArray &publicKey, const QByteArray &salt, int keySize) const final;
std::vector<uint8_t> deriveHMACExtract(const QByteArray &publicKey, const QByteArray &salt, int keySize) const final;
QByteArray sign(QCryptographicHash::Algorithm type, const QByteArray &digest) const final;

static QList<TokenData> tokens();
private:
template<typename F>
QByteArray derive(const QByteArray &publicKey, F &&func) const;
template<typename F>
QByteArray exec(F &&func) const;
std::vector<uint8_t> derive(const QByteArray &publicKey, F &&func) const;
template<typename Result = QByteArray, typename F>
Result exec(F &&func) const;

struct Private;
std::unique_ptr<Private> d;
Expand Down
Loading
Loading