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

Security fixes #816

Merged
merged 9 commits into from Dec 17, 2018
Merged

Security fixes #816

merged 9 commits into from Dec 17, 2018

Conversation

twiss
Copy link
Member

@twiss twiss commented Dec 14, 2018

No description provided.

src/message.js Outdated
} catch (err) {
util.print_debug_error(err);
await Promise.all(privateKeys.map(async function(privateKey) {
const primaryUser = await privateKey.getPrimaryUser(/* symEncryptedPacket.created, userId */);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Essentially what we should be looking at is not the current preferences of the primary user, but the preferences of the user that the message was sent to at the time that the message was sent. (...that the sender knew of -- but that part is obviously impossible.)
The comment sort of serves as a reminder to do so, but it might require adding a toUserIds parameter to openpgp.decrypt and .decryptSessionKeys, which is a bit weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we at least pass in the time though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it turns out the symEncryptedPacket.created property was wishful thinking, and it doesn't exist, so we don't have the time here.

src/message.js Outdated
await Promise.all(privateKeys.map(async function(privateKey) {
const primaryUser = await privateKey.getPrimaryUser(/* symEncryptedPacket.created, userId */);
let algos = [
enums.symmetric.aes256, // OpenPGP.js default
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace this with the current config value? Or perhaps add that one as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe. The issue is that if you change the config value, other people using the normal default will throw -- so maybe it's better to find that out quickly.
Similarly, GPG will warn even with our normal default if we send an AES256 message to someone without AES256 in the preferred keys - so maybe this is just an argument to switch to 3DES as the fallback, as the spec mandates, make that non-configurable, and then add the config value to the start of the preferred algos when generating keys (and use it for password encryption).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's do that

@twiss twiss merged commit b1b1994 into openpgpjs:master Dec 17, 2018
@twiss twiss deleted the security-fixes branch December 17, 2018 17:55
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

Successfully merging this pull request may close these issues.

None yet

2 participants