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

PreKey is being removed after the first message encryption #74

Open
gediminasiv opened this issue Mar 15, 2022 · 6 comments
Open

PreKey is being removed after the first message encryption #74

gediminasiv opened this issue Mar 15, 2022 · 6 comments

Comments

@gediminasiv
Copy link

gediminasiv commented Mar 15, 2022

Hello!

I am trying to implement your protocol to our own application and encountered an unexpected problem.

Lets imagine a standard situation. User A sends message to User B. Operation is standard - User A encrypts a message using User B prekeys, and then User B downloads that message from server, decrypts it with his prekeys.

The problem occurs when User B doesn't reply to User A message, and User A sends another message. Message type is still 3, so it has to be decrypted with decryptPreKeyWhisperMessage. Problem is that after first decryption of prekey message, local prekey of User B is deleted, so it cannot do any more decryption for User A messages.

If I leave removePreKey function in my storage class empty, everything works just fine.

The code is pretty basic, taken from your samples:

 try {
      if (message.type === 1) {
        plaintext = await cipher.decryptWhisperMessage(message.body!, "binary");
      } else if (message.type === 3) {
        plaintext = await cipher.decryptPreKeyWhisperMessage(
          message.body!,
          "binary"
        );
      }

      stringPlaintext = new TextDecoder().decode(new Uint8Array(plaintext));
    } catch (e) {
      console.log(
        `Decrypting unsuccessful. Message shouldn't be saved to server.`
      );
      throw e;
    }Is this a bug, or am I missing something out?

┆Issue is synchronized with this Asana task

@rolfeschmidt
Copy link
Contributor

Hi @gediminasiv and apologies for the slow response on your comment on the other repo. At first glance this does look like a bug in the SDK, not in the demo. The pre-key needs to be removed, but should not be removed until the session is established by both parties.

I will confirm and see if we can delay pre-key removal in the SDK and add a test case based on your scenario. If you have a specific proposal for when the pre-key should be removed, let me know or create a PR.

In the meantime you could work around this at a higher layer by having your application immediately return an ACK or "read receipt" message after receiving that first type 3 message. That way there will be an encrypted response to the first message whether or not User B replies. There will still be failed delivery risks, so it isn't perfect, but messaging failure seems far less likely than a non-responsive user :)

@gediminasiv
Copy link
Author

Hi, Rolfe!

No worries about the slow response over there. I figured that you might not be checking the samples repository as often, so created an issue here :)

As far as I understand, till you release a fix, we will need to manually check whether the session is already established by two parties or not. Maybe do you have any suggestions on how we should do it? :)

@rolfeschmidt
Copy link
Contributor

After thinking about this a little I think the problem is that we shouldn't be reusing that one-time prekey. I won't have a chance to look into it until this weekend, but I will try to put together a definitive answer then - either an SDK fix or usage examples.

In the meantime, I would see if you can:

  • call your directory to get a new prekey bundle for the recipient and call sessionBuilder.processPreKey(newBundle) before creating the next message.
  • OR have the recipient send an ACK message upon receipt (you could use this to fill in the "has been received" checkmarks at the bottom of the message)

I think the first one, or something close to it, is the real solution but I haven't been working inside this code for a while so I must answer hesitantly :)

@rolfeschmidt
Copy link
Contributor

Hi @gediminasiv! I finally looked at this carefully and here is my understanding of the situation:

  • When the first V3 message is received, we perform the X3DH key exchange to create a shared secret, then delete the one-time prekey because we won't need it anymore.
  • When we receive another V3 message after that, we don't have the prekey but we don't need it - we've already created the master secret and can use it.
  • So we can still decrypt incoming v3 messages.

To verify this I have added some simple tests: https://github.com/privacyresearchgroup/libsignal-protocol-typescript/blob/master/src/__test__/session-builder.test.ts#L55

So... it seems I do not quite understand the problem you are facing. Do you think you can give me a more detailed code example? Or better yet a PR with a failing test case? Thanks, and sorry I couldn't come back with an easy answer :)

@gediminasiv
Copy link
Author

Hey, @rolfeschmidt !

Thank you so much for investigating this in such detail. Unfortunately I cannot attach the full repo as I am under an NDA in my company, but your answer suggests that I might be missing something here, so I will try to provide you with the bits that might be needed :D

Here is my message decryption code, from the frontend side:

async readMessage({ message, senderId, id, createdAt }: any) {
    let plaintext: ArrayBuffer = new Uint8Array().buffer;

    const cipher = new SessionCipher(
      this.store,
      new SignalProtocolAddress(senderId, 420) // hardcoded, need to change this to something better
    );

    let stringPlaintext: string;
    try {
      if (message.type === 1) {
        plaintext = await cipher.decryptWhisperMessage(message.body!, "binary");
      } else if (message.type === 3) {
        plaintext = await cipher.decryptPreKeyWhisperMessage(
          message.body!,
          "binary"
        );
      }

      stringPlaintext = new TextDecoder().decode(new Uint8Array(plaintext));
    } catch (e) {
      console.log(
        `Decrypting unsuccessful. Message shouldn't be saved to server.`
      );
      throw e;
    }

    const messageData = {
      senderId,
      id,
      createdAt,
    };

    try {
      await this.saveMessage(messageData, stringPlaintext);
    } catch (e) {
      console.log(e);
      console.log(
        `Unable to save message. This means that the message shouldn't be deleted from server.`
      );
    }

    return true;
  }

Parameters are as follows:
message - Signal message object (the one that contains type, body and registrationId).
senderId - unique ID of sender, that we use to create SignalProtocolAddress with.
id, createdAt - message id and message creation date in our backend, it is only required for message saving.

It is worth mentioning that this function is called in a fashion similar to messagesFetchedFromServer.map(message => this.readMessage(message) when we are fetching new messages from the server. Maybe the problem is lying with the asynchronous fashion the promises of this map are fulfilled?

Anyway, when I receive several prekey messages and try to read them with this code, it decrypts first prekey message successfully, and when it tries to decrypt the second one I get an error from SignalProtocolStorage.loadPreKey method which tells me that the prekey that was being loaded was not found (which is logical because it was deleted after the first decryption) and indicates me that decrypt functions don't event try to access existing session.

@rolfeschmidt
Copy link
Contributor

Hi @gediminasiv - I haven't figured out what is going on here so haven't updated. My concern is with the storage of the SessionRecord in the SignalProtocolStore. The code for that is a bit convoluted, but still looks correct to me and I have not produced a failing test case (I have to admit I haven't had much time to try lately...).

Here is where I would be looking:

  • After deleting that ephemeral key, you should have stored a SessionRecord that contains all you need to continue decrypting messages. It will be retrieved by the function SessionRecord::getSessionByBaseKey. I would put a breakpoint/console.log/etc in there and see why it is not returning a valid session.
  • That session should be stored in the SessionRecord the first time you process a v3 message, in the function SessionBuilder::processV3. It is then stored to your SignalProtocolStore in SessionCipher::decryptPreKeyWhisperMessage.

I would use the tools at my disposal - debugger, console logs, etc - to confirm that the session is in fact getting stored correctly, that the baseKey is in fact the same on all of the v3 messages, that the session is still present in the SessionRecord after loading it from the store, etc.

I'll keep thinking about this and trying to find test cases. Can you confirm two things for me:

  • If Alice sends Bob a v3 message, Bob decrypts it, then Bob responds with a v1 message then Alice can decrypt the message and everything works from there on.
  • If Alice sends two v3 messages in a row without response from Bob, the first one is decrypted successfully and the second one fails with message unable to find session for base key <base 64 for base key> and that base 64 is the same as the base64 for the base key on the first message sent?

I know I'm sending you deeper into the guts of this than we'd like, but I'm stumped.

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

2 participants