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

Use the group password only through a KDF #27

Open
wants to merge 3 commits into
base: ngc_merge
Choose a base branch
from

Conversation

sudden6
Copy link

@sudden6 sudden6 commented May 1, 2022

This changes the NGC protocol in the following ways:

  • The password is turned into a "passkey" through a Key Derivation Function (KDF), which uses the group public id as a salt.
  • The API is changed such that the password can not be read from the group again

This has the following advantages:

  • The "password" will no longer be stored in RAM in plaintext, although getting access to the group shared state will still allow access to the group. It will now be impossible to get what was used as the current group password in plaintext.
  • Allows arbitrarily long passwords, although at the cost of having to always transmit 32bytes for packets where the group password used to be. (IMHO that's not really relevant, as the password was up to (1 to 32)+2 bytes)
  • It's good practice to never use plaintext passwords in protocols as far as possible. This PR shows that it's possible, so I'm strongly in favor of implementing these changes to the NGC protocol.

This has the following disadvantages:

  • The plaintext password will no longer be known to the whole group once the founder changes it. If peers should be able to invite other peers the plaintext password must be redistributed via some way. IMO this could also be a good feature, allowing the founder to invite people to the group and at some point "closing" the group for new invites by changing the password. An option to workaround this problem would be to allow the founder to optionally send a broadcast to the group (or to select people in the group) with the plaintext password. It really depends on what we want to do though.

In this PR I implemented a PoC on what changes would be needed. It is not fully complete and I think I broke packet encoding somewhere, but it should suffice to show that this doesn't increase the complexity.

JFreegman and others added 3 commits May 1, 2022 15:02
This avoids storing plaintext passwords and allows to use arbitrarily
long passwords in groups.
It's state is encoded in the presence/absence of the passkey bytes.


/* We re-use the Group ID as salt for the password, if this condition is violated and out-of-bounds read happens */
static_assert(EXT_PUBLIC_KEY_SIZE >= crypto_pwhash_scryptsalsa208sha256_SALTBYTES,

Choose a reason for hiding this comment

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

afaik, toxcore uses C99, but static_assert is a C11 feature.
https://en.cppreference.com/w/c/error/static_assert

Copy link
Author

Choose a reason for hiding this comment

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

It's used a bit above, so I think it works. Not sure though why...

@JFreegman JFreegman force-pushed the ngc_merge branch 15 times, most recently from f9c7385 to 17ab6d1 Compare May 9, 2022 21:47
@JFreegman JFreegman force-pushed the ngc_merge branch 6 times, most recently from d57ebf3 to 8c8b321 Compare May 17, 2022 13:55
@JFreegman JFreegman force-pushed the ngc_merge branch 5 times, most recently from c6b454a to e514356 Compare May 29, 2022 14:45
@JFreegman JFreegman force-pushed the ngc_merge branch 5 times, most recently from 074f0eb to 372bd1c Compare June 5, 2022 14:09
@JFreegman JFreegman force-pushed the ngc_merge branch 3 times, most recently from c1eb0d6 to 02996f0 Compare June 16, 2022 15:31
@JFreegman JFreegman force-pushed the ngc_merge branch 3 times, most recently from 3a0ad65 to 4e80d9c Compare July 13, 2022 16:05
@JFreegman JFreegman force-pushed the ngc_merge branch 2 times, most recently from 87e0a26 to 2acb790 Compare July 29, 2022 05:38
@JFreegman JFreegman force-pushed the ngc_merge branch 11 times, most recently from eca315a to 0a277b5 Compare September 22, 2022 15:16
@emdee-is
Copy link

@JFreegman and @Green-Sky I think we need the Tox equivalent of Python PEPs to get proposals for improvement identified, and prioritized.

If https://wiki.tox.chat allowed logged in users to write then we could use that unless there's something better.

@Green-Sky
Copy link

@JFreegman @sudden6 , i think this should be reopened/retargeted to c-toxcore:master for further discussion.

@emdee-is
Copy link

Can you make/add a Security label for these kind of issues, so we can keep track of vulnerabilities or rooms for improvement?

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.

4 participants