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

Stop writing keys to debug logs #1191

Open
Tracked by #1185
neekolas opened this issue Oct 25, 2024 · 0 comments
Open
Tracked by #1185

Stop writing keys to debug logs #1191

neekolas opened this issue Oct 25, 2024 · 0 comments
Assignees

Comments

@neekolas
Copy link
Contributor

neekolas commented Oct 25, 2024

Impact

Sensitive information, such as cryptographic keys, should never be written to log files to avoid potential disclosure.

Description

Log files are generally considered to be less protected than most security-critical assets in an application. Therefore, unless a sufficient level of protection on log files is actively maintained, it is best practice to avoid logging any sensitive information to log files. Even if constrained to specific instances, such as debug configurations, there remains a risk of a user or maintainer accidentally using a build with a debug flag active.

Within the SQL KeyStore, the write_encryption_epoch_key_pairs() function is used to write the HPKE keys for the epoch to the store:

fn write_encryption_epoch_key_pairs<
    GroupId: traits::GroupId<CURRENT_VERSION>,
    EpochKey: traits::EpochKey<CURRENT_VERSION>,
    HpkeKeyPair: traits::HpkeKeyPair<CURRENT_VERSION>,
>(
    &self,
    group_id: &GroupId,
    epoch: &EpochKey,
    leaf_index: u32,
    key_pairs: &[HpkeKeyPair],
) -> Result<(), Self::Error> {
    let key = epoch_key_pairs.id(group_id, epoch, leaf_index)?;
    let value = bincode::serialize(key_pairs)?;
    tracing::debug!("Writing encryption epoch key pairs");
    tracing::debug!(" key: {}", hex::encode(&key));
    tracing::debug!(" value: {}", hex::encode(&value));

    self.write::<CURRENT_VERSION>(EPOCH_KEY_PAIRS_LABEL, &key, &value);
}

Figure 17: xmtp_mls/src/storage/sql_key_store.rs

As highlighted, the value epoch contains the EpochKey and the array key_pairs contains HPKE key pairs. Both of these cryptographic secrets are serialized and used in the computation key and value, both of which are logged using tracing::debug(). This behavior is inconsistent with other functions in the file (and the rest of the codebase), which suggests that the highlighted log messages may be unintentional or a remnant from the development process instead of necessary debug information.

To avoid potential leakage of the epoch key or HPKE keys via debug logs, it is recommended to remove or sanitize the above log statements such that they are safe to be made public.

Recommendation

Remove the key and value log outputs in write_encryption_epoch_key_pairs().

Location

xmtp_mls/src/storage/sql_key_store.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants