Skip to content

Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style #14268

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

Merged
laanwj merged 3 commits into from
Jan 16, 2019
Merged

Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style #14268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
951a44e
Drop unused setRange arg to BerkeleyBatch::ReadAtCursor
Empact Sep 19, 2018
1a9f9f7
Introduce SafeDbt to handle DB_DBT_MALLOC raii-style
Empact Sep 19, 2018
4a86a0a
Make SafeDbt DB_DBT_MALLOC on default initialization
Empact Nov 25, 2018
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
39 changes: 39 additions & 0 deletions src/wallet/db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,45 @@ BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const std::string&
return (fRecovered ? VerifyResult::RECOVER_OK : VerifyResult::RECOVER_FAIL);
}

BerkeleyBatch::SafeDbt::SafeDbt()
{
m_dbt.set_flags(DB_DBT_MALLOC);
}

BerkeleyBatch::SafeDbt::SafeDbt(void* data, size_t size)
: m_dbt(data, size)
{
}

BerkeleyBatch::SafeDbt::~SafeDbt()
{
if (m_dbt.get_data() != nullptr) {
// Clear memory, e.g. in case it was a private key
memory_cleanse(m_dbt.get_data(), m_dbt.get_size());
Copy link
Member

@laanwj laanwj Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes behavior: in the case of Write it was also cleansing the dtabase key, now it no longer is.
Not sure if this is necessary, I don't think the database keys ever contain sensitive information, but it should be noted.

Copy link
Contributor Author

@Empact Empact Nov 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is cleansing the key - if you're referring to the cleansing here:
https://github.com/bitcoin/bitcoin/pull/14268/files#diff-c557c8e6d496041acccfd8ac4e3db625L257

Then the key and value are separate SafeDbt objects, with each managing/cleansing its own data:
https://github.com/bitcoin/bitcoin/pull/14268/files#diff-c557c8e6d496041acccfd8ac4e3db625R258
https://github.com/bitcoin/bitcoin/pull/14268/files#diff-c557c8e6d496041acccfd8ac4e3db625R264

// under DB_DBT_MALLOC, data is malloced by the Dbt, but must be
// freed by the caller.
// https://docs.oracle.com/cd/E17275_01/html/api_reference/C/dbt.html
if (m_dbt.get_flags() & DB_DBT_MALLOC) {
free(m_dbt.get_data());
}
}
}

const void* BerkeleyBatch::SafeDbt::get_data() const
{
return m_dbt.get_data();
}

u_int32_t BerkeleyBatch::SafeDbt::get_size() const
{
return m_dbt.get_size();
}

BerkeleyBatch::SafeDbt::operator Dbt*()
Copy link
Contributor

@practicalswift practicalswift Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const? :-)

Copy link
Member

@laanwj laanwj Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's returning a mutable pointer to the internal structure, I don't think it'd make sense to make the method const (unless you mean it should return a const* pointer too, but that's likely not good enough for the BDB functions)

{
return &m_dbt;
}

bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename)
{
std::string filename;
Expand Down
81 changes: 34 additions & 47 deletions src/wallet/db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,29 @@ class BerkeleyDatabase
bool IsDummy() { return env == nullptr; }
};


/** RAII class that provides access to a Berkeley database */
class BerkeleyBatch
{
/** RAII class that automatically cleanses its data on destruction */
class SafeDbt final
{
Dbt m_dbt;

public:
// construct Dbt with internally-managed data
SafeDbt();
// construct Dbt with provided data
SafeDbt(void* data, size_t size);
~SafeDbt();

// delegate to Dbt
const void* get_data() const;
u_int32_t get_size() const;

// conversion operator to access the underlying Dbt
operator Dbt*();
};

protected:
Db* pdb;
std::string strFile;
Expand Down Expand Up @@ -197,7 +216,6 @@ class BerkeleyBatch
/* verifies the database file */
static bool VerifyDatabaseFile(const fs::path& file_path, std::string& warningStr, std::string& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc);

public:
template <typename K, typename T>
bool Read(const K& key, T& value)
{
Expand All @@ -208,13 +226,11 @@ class BerkeleyBatch
CDataStream ssKey(SER_DISK, CLIENT_VERSION);
ssKey.reserve(1000);
ssKey << key;
Dbt datKey(ssKey.data(), ssKey.size());
SafeDbt datKey(ssKey.data(), ssKey.size());

// Read
Dbt datValue;
datValue.set_flags(DB_DBT_MALLOC);
int ret = pdb->get(activeTxn, &datKey, &datValue, 0);
memory_cleanse(datKey.get_data(), datKey.get_size());
SafeDbt datValue;
int ret = pdb->get(activeTxn, datKey, datValue, 0);
bool success = false;
if (datValue.get_data() != nullptr) {
// Unserialize value
Expand All @@ -225,10 +241,6 @@ class BerkeleyBatch
} catch (const std::exception&) {
// In this case success remains 'false'
}

// Clear and free memory
memory_cleanse(datValue.get_data(), datValue.get_size());
free(datValue.get_data());
}
return ret == 0 && success;
}
Expand All @@ -245,20 +257,16 @@ class BerkeleyBatch
CDataStream ssKey(SER_DISK, CLIENT_VERSION);
ssKey.reserve(1000);
ssKey << key;
Dbt datKey(ssKey.data(), ssKey.size());
SafeDbt datKey(ssKey.data(), ssKey.size());

// Value
CDataStream ssValue(SER_DISK, CLIENT_VERSION);
ssValue.reserve(10000);
ssValue << value;
Dbt datValue(ssValue.data(), ssValue.size());
SafeDbt datValue(ssValue.data(), ssValue.size());

// Write
int ret = pdb->put(activeTxn, &datKey, &datValue, (fOverwrite ? 0 : DB_NOOVERWRITE));

// Clear memory in case it was a private key
memory_cleanse(datKey.get_data(), datKey.get_size());
memory_cleanse(datValue.get_data(), datValue.get_size());
int ret = pdb->put(activeTxn, datKey, datValue, (fOverwrite ? 0 : DB_NOOVERWRITE));
return (ret == 0);
}

Expand All @@ -274,13 +282,10 @@ class BerkeleyBatch
CDataStream ssKey(SER_DISK, CLIENT_VERSION);
ssKey.reserve(1000);
ssKey << key;
Dbt datKey(ssKey.data(), ssKey.size());
SafeDbt datKey(ssKey.data(), ssKey.size());

// Erase
int ret = pdb->del(activeTxn, &datKey, 0);

// Clear memory
memory_cleanse(datKey.get_data(), datKey.get_size());
int ret = pdb->del(activeTxn, datKey, 0);
return (ret == 0 || ret == DB_NOTFOUND);
}

Expand All @@ -294,13 +299,10 @@ class BerkeleyBatch
CDataStream ssKey(SER_DISK, CLIENT_VERSION);
ssKey.reserve(1000);
ssKey << key;
Dbt datKey(ssKey.data(), ssKey.size());
SafeDbt datKey(ssKey.data(), ssKey.size());

// Exists
int ret = pdb->exists(activeTxn, &datKey, 0);

// Clear memory
memory_cleanse(datKey.get_data(), datKey.get_size());
int ret = pdb->exists(activeTxn, datKey, 0);
return (ret == 0);
}

Expand All @@ -315,20 +317,12 @@ class BerkeleyBatch
return pcursor;
}

int ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue, bool setRange = false)
int ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue)
{
// Read at cursor
Dbt datKey;
unsigned int fFlags = DB_NEXT;
if (setRange) {
datKey.set_data(ssKey.data());
datKey.set_size(ssKey.size());
fFlags = DB_SET_RANGE;
}
Dbt datValue;
datKey.set_flags(DB_DBT_MALLOC);
datValue.set_flags(DB_DBT_MALLOC);
int ret = pcursor->get(&datKey, &datValue, fFlags);
SafeDbt datKey;
SafeDbt datValue;
int ret = pcursor->get(datKey, datValue, DB_NEXT);
if (ret != 0)
return ret;
else if (datKey.get_data() == nullptr || datValue.get_data() == nullptr)
Expand All @@ -341,16 +335,9 @@ class BerkeleyBatch
ssValue.SetType(SER_DISK);
ssValue.clear();
ssValue.write((char*)datValue.get_data(), datValue.get_size());

// Clear and free memory
memory_cleanse(datKey.get_data(), datKey.get_size());
memory_cleanse(datValue.get_data(), datValue.get_size());
free(datKey.get_data());
free(datValue.get_data());
return 0;
}

public:
bool TxnBegin()
{
if (!pdb || activeTxn)
Expand Down