equalitie / np1sec Goto Github PK

The Hash256Bit::operator== method handles constant time comparison for us.

It's always better to just explicitly use separate instances of PublicKey and PrivateKey.

For the sake of avoiding encoding things as std::strings, which have less reliable deallocation behavior depending on implementation, we should stick to strictly using gcry_sexp_t and, preferably, the PublicKey and PrivateKey classes that abstract over them.

Being worked on in the key-type branch.

Review existing lib-pyFreenet code

When an invitee becomes a participant, they should send pending timeout messages.

If a user stops receiving any messages, they get timed out by the rest of the conversation eventually. But they don't notice the timeout themselves, which is bad.

A client should declare themselves timed out when they stop receiving their own sent messages back from the server.

Currently, chat messages contain a signature over a payload that includes the number of messages sent using that key. This is a simple way to avoid any replay attacks, as replaying an earlier chat message by a user is considered invalid (it has an incorrect message number).

This scheme only applies to chat messages, not conversation messages. As a consequence, conversation messages are vulnerable to replay attacks. The most critical parts of the protocol have data covered by the signature to prevent this, but not all messages do. It's an easy fix to avoid the entire class of problems by adding a similar message-ID field to conversation messages.

Happens when a new client connects to the server with a name already present on the server.

Say we have two clients Alice and Bob and they both log into the server with username "Charlie". First Alice joins the server. Then, when Bob joins, Alice receives RoomInterface::disconnect message infinitely many times.

These are join, accept, send and receive as documented in the paper.

/**
is called by the constructor if the room is already
inhibited
*/
bool join();

/* should be called when someone new join the chatroom.
this will modifies the session id*/
bool accept(std::string new_participant_id);

/**
this will be called when a user leave a chatroom
to update the key (better called kick out
*/
bool farewell(std::string leaver_id);

/**
When a user wants to send a message to a session
it needs to call its send function
*/
bool send(MpotrMessage message);

/**
When a message is received from a session
the receive function needs to be called to decrypt

it updates the session status

@return the decrypted message to be shown, it might
be null if the message was a meta message
*/
MpotrMessage receive(std::string raw_message);

The current timeout protocol requires that, if Alice gets timed out and Bob does not declare a timeout for Alice, Charlie needs to timeout Bob. If Bob then sets the timeout on Alice, Charlie needs to retract the timeout for Bob.

When there are multiple additional people in the conversation doing the same thing in regards to Bob, this can lead to nasty chaotic fluctuations leading to timeouts never happening.

This aspect of the protocol probably needs some redesigning. One possibility is to have each user announce, not timeout flags, but timeout levels -- if Charlie announces that Alice is timed out with level 2, that implies that everyone who has not yet timed out Alice is timed out with level 1 according to Charlie, implicitly.

They're only used in integration testing.

The triple_ed_dh function has a my_long_term_key parameter that was previously classified as an np1secAsymmetricKey. Is this semantically a private key or a public key?

Currently, no timeouts of any sort apply to unidentified invitees. This is not a huge problem as their participation is not required anywhere, but it does clog up the conversation state machine for no good reason.

There are two easy ways to fix this:

• The normal timeout rules could apply to unidentified invitees, or
• Participants could do their own timeout tracking of unidentified invitees, and uninvite any unidentified invitees that did not respond for too long.

In crypt.cc, there are two assert calls. One in Cryptic::hash and another in Cryptic::Crytpic. Rather than potentially letting the whole program using np1sec crash, should we not simply throw exceptions? If so, what ones?

There ways to confirm a malcious blocking users by checking digital signature etc. But there are situation arises that legimitely a user gets the traffic from the supposedly "DoSing" user and other doesn't? How should (n+1)sec should re-act in such situation?

1. Should we take no action?
2. Should we have a moderator and takes the side of the moderator?
3. Should we break the room in two?
4. Should we act democratically?

The solutions are ranked based on complexity of implementation.

To stick to our more safe SecureString and PublicKey & PrivateKey classes, we should elimiate calls to functions converting such data to strings.

conversation.cc:1653: bool np1sec::Conversation::fsck(): Assertion *user_it != event_it' failed.`

This assertion fires when running the tests in tests/echo_chamber/tests.cc. Unfortunately, the particular test it happens in is random and the best chance of reproducing it is to run the full test suite at once (i.e. run the echo_chamber executable without the --run_test= argument).

Currently, there is nothing stopping conversation participants from inviting usernames that are not in the room. These will bloat the conversation state machine for no good reason. This should probably be disallowed, which requires the state machine needs to track joins as well as leaves.

When the np1sec headers are included in projects other than the library itself, they should
be included in a form

#include <np1sec/interface.h>

Instead of

#include "src/interface.h"

As is done e.g. here. Keywords such as "interface" are generic and very likely used in other projects as well so adding the "np1sec/" prefix would avoid file name clashes. Plus it is nice to be explicit about where the header is located.

Most of the methods in Cryptic that are currently static don't need to be now that we have an np1sec namespace.

To facilitate FFI bindings in other languages.

Seems to be mostly relevant in message.(cc|h), session.(cc|h) and transcript_consistency.h.

Currently, almost any unexpected result by a libgcrypt call will result in a CryptoException that isn't handled anywhere, leading to an exit(). This is a rare occurrence, but still a bad thing when it does happen. This error handling should be improved.

According to documentation on group key agreement the following check seems to be ommited: z1⊕ . . . ⊕zn = 0 whilst mentioned on original Pointcheval's GKE+P protocol.

So I wondered why is ommited? I also asked upon this as well: https://crypto.stackexchange.com/q/66544/59291

This bug is exposed by the test_session_join_order test. The pseudo algorithm that the test implements goes like this:

• user0 waits for user1 and user2 to enter the room.
• Once user1 and user2 are in the room, user0 invites them both
• When user1 receives the ConversationInterface::joined_chat event, the Conversation::participants() function returns [user0, user1, user2]
• When user2 receives the ConversationInterface::joined_chat event, the Conversation::participants() function returns [user0, user1, user2]
• Once everyone has joined everyone else's chat, a new user is created with name "new_guy" and is invited by user0 into the chat.
• While user0, user1 and user2 are waiting for the "new_guy" to join the chat, user1 (or sometimes user2) receives a ConversationInterface::user_joined_chat("user2") (or user2 receives user_joined_chat("user1")).

The expected behavior is that user1 does not receive user_joined_chat("user2") event as user2 was already in the conversation (as known from the third bullet from the top).

The protocol cannot currently deal in any way with usernames changing. Let's fix this.

A good way to avoid a lot of key serialization may be to have the Participant classes not contain string keys but references to keys instead.

questions on https://github.com/equalitie/np1sec/blob/api-docs/doc/np1sec_api.md

• line 33: Once the process of creating a conversation is finished, the user is announced through the RoomInterface::conversation_created callback.

  Formerly "the user is let known" -- is "announced" correct? Or should it maybe "presented"/"introduced"?

• line 149: "To stay independent from the event loop it is used in as much as possible, users
  of the library are required to implement few timer related function" -- what is "it" in "the event loop it is used in"?

• line 176: unlike the other 2 bullet points, this doesn't see to be an event. can you confirm it shouldn't be rephrased?

We need to intercept "Receive" for sure and we should be able to send.

We also need to be notified when someone leaves.

It gives us higher performance if we listen on the "Join" event as well cause we don't need to wait for the join request to arrive from new participant to start a new session.

It doesn't look to be used anywhere and we have other accessors for KeyPair.

It causes double free of Conversations inside ConversationList.

We do mention that we can't call any room modifiers from inside Room's callbacks, but this is a bit different as the call to Room::message_received often results ConversationInterface callback being called. Implying that we can't destroy Room from inside those callbacks neither.

Possible solutions:

• Fix it such that Room can be destroyed from inside ConversationInterface callbacks
• Update documentation stating that it can't be done.

As far as gcry_sexp_t types go, the new PrivateKey and PublicKey classes will handle calling gcrypt's release function in each class' destructor.

Conversations currently don't have a title. This could easily be added, and it would probably be an improvement.

The annoying part of this concept is that there is no way to ensure conversation title uniqueness: a room could contain multiple conversations with the same title and overlapping users, in which case the name doesn't add much. But adding them is still probably better than not adding them.

It seems g++ 4.8 doesn't support some random features that the echo_chamber is using.

/home/ubuntu/np1sec/test/echo_chamber/tests.cc:547:10: error: ‘random_device’ in namespace ‘std’ does not name a type
     std::random_device rd;
 
(16:28:01) donncha: 
/home/ubuntu/np1sec/test/echo_chamber/tests.cc:548:10: error: ‘mt19937’ in namespace ‘std’ does not name a type
     std::mt19937 gen;

could you make a client that when it receives a message 1)adds "mpSeQ:"
to its begining
and 2) sends also "mpSeQ:)" message
and also when the user send a message add "mpSeQ:" to its begining

It appears that we are only converting bytes into strings here so we can easily get their length. Rather than relying on this conversion step we want to avoid, we should find a way to simple get the count we want.

This version of this function that accepts a uint8_t* parameter should be removed since we should only deal with PublicKey and PrivateKey instances outside of crypt.(h|cc).