Skip to content
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.

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

Fix a use-after-free in client reset when the original RealmCoordinator is gone #5949

Merged
merged 1 commit into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* CompensatingWriteErrorInfo reported string primary keys as boolean values instead ([PR #5938](https://github.com/realm/realm-core/pull/5938), since the introduction of CompensatingWriteErrorInfo in 12.1.0).
* Fix a use-after-free if the last external reference to an encrypted Realm was closed between when a client reset error was received and when the download of the new Realm began. ([PR #5949](https://github.com/realm/realm-core/pull/5949), since 12.4.0).

### Breaking changes
* Rename RealmConfig::automatic_handle_backlicks_in_migrations to RealmConfig::automatically_handle_backlinks_in_migrations ([PR #5897](https://github.com/realm/realm-core/pull/5897)).
Expand Down
11 changes: 5 additions & 6 deletions src/realm/db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ void DB::open(const std::string& path, bool no_create_file, const DBOptions opti
// close previously, but wasn't (perhaps due to the process crashing)
cfg.clear_file = (options.durability == Durability::MemOnly && begin_new_session);

cfg.encryption_key = m_key;
cfg.encryption_key = options.encryption_key;
ref_type top_ref;
try {
top_ref = alloc.attach_file(path, cfg); // Throws
Expand Down Expand Up @@ -1048,7 +1048,7 @@ void DB::open(const std::string& path, bool no_create_file, const DBOptions opti
path);
}

if (m_key) {
if (options.encryption_key) {
#ifdef _WIN32
uint64_t pid = GetCurrentProcessId();
#else
Expand Down Expand Up @@ -1099,7 +1099,7 @@ void DB::open(const std::string& path, bool no_create_file, const DBOptions opti
uint64_t pid = getpid();
#endif

if (m_key && info->session_initiator_pid != pid) {
if (options.encryption_key && info->session_initiator_pid != pid) {
std::stringstream ss;
ss << path << ": Encrypted interprocess sharing is currently unsupported."
<< "DB has been opened by pid: " << info->session_initiator_pid << ". Current pid is " << pid
Expand Down Expand Up @@ -1280,7 +1280,7 @@ bool DB::compact(bool bump_version_number, util::Optional<const char*> output_en
}
SharedInfo* info = m_file_map.get_addr();
Durability dura = Durability(info->durability);
const char* write_key = bool(output_encryption_key) ? *output_encryption_key : m_key;
const char* write_key = bool(output_encryption_key) ? *output_encryption_key : get_encryption_key();
{
std::unique_lock<InterprocessMutex> lock(m_controlmutex); // Throws

Expand Down Expand Up @@ -2500,8 +2500,7 @@ void DB::async_request_write_mutex(TransactionRef& tr, util::UniqueFunction<void
}

inline DB::DB(const DBOptions& options)
: m_key(options.encryption_key)
, m_upgrade_callback(std::move(options.upgrade_callback))
: m_upgrade_callback(std::move(options.upgrade_callback))
{
if (options.enable_async_writes) {
m_commit_helper = std::make_unique<AsyncCommitHelper>(this);
Expand Down
3 changes: 1 addition & 2 deletions src/realm/db.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class DB : public std::enable_shared_from_this<DB> {

const char* get_encryption_key() const noexcept
{
return m_key;
return m_alloc.m_file.get_encryption_key();
}

#ifdef REALM_DEBUG
Expand Down Expand Up @@ -461,7 +461,6 @@ class DB : public std::enable_shared_from_this<DB> {
std::string m_lockfile_prefix;
std::string m_db_path;
std::string m_coordination_dir;
const char* m_key;
int m_file_format_version = 0;
util::InterprocessMutex m_writemutex;
std::unique_ptr<ReadLockInfo> m_fake_read_lock_if_immutable;
Expand Down
10 changes: 9 additions & 1 deletion src/realm/object-store/sync/sync_session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,18 @@ void SyncSession::download_fresh_realm(sync::ProtocolErrorInfo::Action server_re
server_requests_action);
}
}

std::vector<char> encryption_key;
{
util::CheckedLockGuard lock(m_config_mutex);
encryption_key = m_config.encryption_key;
}

DBOptions options;
options.encryption_key = m_db->get_encryption_key();
options.allow_file_format_upgrade = false;
options.enable_async_writes = false;
if (!encryption_key.empty())
options.encryption_key = encryption_key.data();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the encryption key need to live longer than this function, or is it only needed to open the realm initially?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only used when opening the file. The encryption layer constructs a context using the key, and explicitly copies the key when the underlying thing doesn't do that already.

Given that they don't reliably work, File::get_encryption_key() and DB::get_encryption_key() probably shouldn't exist.


std::shared_ptr<DB> db;
auto fresh_path = ClientResetOperation::get_fresh_path_for(m_db->get_path());
Expand Down