Leaking Identity Documents and Document Urls: could you please resolve this big problem?

[raphael10-collab]

Could you please encrypt identities?

pushpin/src/renderer/components/content-types/workspace/omnibox/OmniboxWorkspaceListMenu.tsx

Line 504 in c05a90a

if (!s.invites[recipientUrl]) {

[pvh]

Hi Raphael -- thanks for filing the issue! The comment is somewhat more dire than the problem. Document links themselves are now being encrypted (that's line 509). Encrypting the identity URLs as well would be feasible but a little trickier. The right solution is going to depend on the threat model.

The current situation is that someone you collaborate with in PushPin is going to be able to see who the other people you've shared documents with are, but not what documents you've shared with them. What kind of problems might you see arising from that?

[raphael10-collab]

Hi Peter!
Thank you for your kind reply.

If someone I collaborate with is able to see who are the other people I've shared documents with, it cannot be a problem if all these people know each others, may be because they belong to the same "circle" , to the same group of friends, to the same company.

But if all these people do not belong to the same "circle", strict environment, group of friends or company, it is actually a problem of privacy, and, therefore, of "security".

[pvh]

PushPin takes many shortcuts throughout its design, usually around security, which are mostly documented in the warnings linked from the readme. PushPin is not secure software. That's by design -- a secure design is undoubtedly possible but we did not design this project to be secure.

Fixing this particular bug would not eliminate the many other shortcomings of the system.

On the other hand, depending on what your threat model is, PushPin might be more secure than your other options if you only use it with trusted collaborators.

I'm delighted to see you working on the project, and I don't think it's wasted time to fix issues like this, but I really don't want to give you the impression that fixing this bug will leave you with "secure" software afterwards, nor do I want you to have the impression that PushPin is suitable for holding legally, or politically sensitive information.

[raphael10-collab]

Your kind and with heart description of the current situation is really appreciable.
Actually my objective is to incrementally build a collaborative system suitable for holding legally sensitive information.
If the flaws and "bugs"-by-design can be incrementally, step-by-step, fixed, or, somehow, effectively circumvented, it would be great!.
For example the other "flaw" or "bug" to fix would be to be able to unshare PushPin links once they are shared.

I'm slowing understanding the code of PushPin, and being conscious that it's an experimental software, I'm available, with your help, to try to fix or to bypass some of these privacy and "security" shortcomings

[pvh]

So, first -- I would like to again emphasize that while I am happy to see the security of the system improve over time I want to absolutely underline that this software is not currently secure against any kind of well-resourced or motivated opposition and also it is not intended to be so.

That said, this might be a good early patch for you then.

The key problem here is that your identity document is semi-public. It defines you to other people. What you need is a place that another peer can find data you share with them in a way that other people can't use to connect the two of you.

The "outbox" sharing mechanism means that I can publish documents links that only the intended recipient can decrypt.

One approach would be to share a document URL out-of-band when you're both online, but PushPin is designed never to assume a direct connection exists between two peers, not for security reasons but for robustness and to support asynchronous collaboration.

Perhaps a simple approach here would simply be to make the "identity" the string "for_you" encrypted with the recipient's pubkey. Then the receiver would simply check each key in their peers' documents for a share section that decrypted to the known value.

Why not give that a shot?

[raphael10-collab]

At this stage, I have more questions, than answers.

in the OmniboxWorkspaceListMenu.tsx's offerDocumentToIdentity function I mixed your proposed solution with my comments and questions, and tried to sketch a beginning of solution :

import CryptoJS from 'crypto-js';

 // -------------------------------------------------------------------------------------------------------------------------------------
  offerDocumentToIdentity = async (recipientPushpinUrl: PushpinUrl) => {
    if (
      // eslint-disable-next-line
      !window.confirm(
        'Are you sure you want to share the currently viewed document ' +
          '(and all its linked documents) with this user?'
      )
    ) {
      return
    }

    // XXX out of scope RN but consider if we should change the key for consistency?
    const { type, hypermergeUrl: recipientUrl } = parseDocumentLink(recipientPushpinUrl)
    const { doc: workspace } = this.state

    if (!workspace || !workspace.selfId) {
      return
    }

    if (type !== 'contact') {
      throw new Error(
        'Offer the current document to a contact by passing in the contact id document.'
      )
    }
   const senderSecretKey =
     workspace.secretKey &&
     (await this.props.repo.crypto.verifiedMessage(this.props.hypermergeUrl, workspace.secretKey))
    if (!senderSecretKey) {
      throw new Error(
        'Workspace is missing encryption key. Sharing is disabled until the workspace is migrated to support encrypted sharing. 
Open the workspace on the device on which it was first created to migrate t$
      )
    }

    const recipient = await getDoc<ContactDoc>(this.props.repo, recipientUrl) // type: contactDoc
    const recipientPublicKey =
      recipient.encryptionKey &&
      (await this.props.repo.crypto.verifiedMessage(recipientUrl, recipient.encryptionKey))
    if (!recipientPublicKey) {
      throw new Error('Unable to share with the recipient - they do not support encrypted sharing.')
    }

    const box = await this.props.repo.crypto.box(
      senderSecretKey,
      recipientPublicKey,
      workspace.currentDocUrl
    )

    this.props.repo.change(workspace.selfId, (s: ContactDoc) => {
      if (!s.invites) {
        s.invites = {}
      }

      // XXX right now this code leaks identity documents and document URLs to
      //     every single person who knows you
      // TODO: encrypt identity

      // The key problem here is that your identity document is semi-public.
      // It defines you to other people.
      // What you need is a place that
      // another peer can find data, that you share with them,
      // in a way that other people can't use to connect the two of you.
      // The "outbox" sharing mechanism means that I can publish documents links
      // that only the intended recipient can decrypt.

      // This implies that:
      // - the sender publish documents to everyone
      // But:
      // - the sender specifies in advance who is the receiver
      // - the sender encrypts the receiver identity together with the receiver's public key
      // - All the receivers receive all the documents but only the intended recipient can decrypt

      // Make the "identity" the string "for_you" encrypted with the recipient's public key
      const recipientObj = {rUrl: recipientUrl, recipientPKey: recipientPublicKey}
      let cipheredRecipientObj = CryptoJS.AES.encrypt(recipientObj, 'secret key 123').toString();

      // Then the receiver would simply check each key in their peers' documents
      // for a share section that decrypted to the known value

      // I imagine that the receiver and the sender, are the only ones to own the secret key,
      // thus the only ones to be able to decrypt the messages.

      // But: where I can send only to that specific receiver the secret key for that specific workspace,
      // which enables her/him to be the only one to decrypt the messages, creating a private exchange?
      // And how to extended this to a group of people?
      // (one way could be that the very first one initiating the group exchange,
      // would send to everyone else the secret key, enabling private group conversation).

      // XXX right now this code leaks identity documents and document URLs to every single person who knows you :

      if (!s.invites[recipientUrl]) {
        //s.invites[recipientUrl] = []
        s.invites[cipheredRecipientObj] = []
      }

      // TODO: prevent duplicate shares.
      s.invites[recipientUrl].push(box)
    })
    // ------------------------------------------------------------------------------------------
}

In my very first sketch of beginning of a solution, I used crypto-js because I do not understand where and how exactly in https://github.com/automerge/hypermerge/blob/master/dist/Crypto.d.ts the encryption takes place.

[pvh]

This is great! I'm delighted to see you making some progress here. Hypermerge has some poorly documented cryptographic functions which do the work here and for consistency I'd probably encourage you to re-use those rather than adding more dependencies. The crypto functions in question are from libsodium / sodium-native, which the hyper* stack uses for encryption features. Does that help get you pointed in the right direction?

[raphael10-collab]

The sodium's sealed boxes seem to be the the right tool, because they encrypt a message in a sealed box using a throw-away keypair, that is overwritten during encryption, hiding the association between the sender and the cyphered text.

With

const recipientObj = {rUrl: recipientUrl, recipientPKey: recipientPublicKey}
const recipientObjStringified = JSON.stringify(recipientObj)
let pk, sk = sodium.crypto_keypair()
const senderPKey = Crypto.encode(pk)

const sealedRecipientObj = await this.props.repo.crypto.sealedBox(
  senderPKey,
  recipientObjStringified
)

I'm getting

Argument of type 'EncodedPublicSigningKey' is not assignable to parameter of type 'EncodedPublicEncryptionKey'
    503 |     const sealedRecipientObj = await this.props.repo.crypto.sealedBox(
  > 504 |       senderPKey,
        |        ^

I didn't find a way to get an EncryptionKey with sodium

And If I use

const encryptionKeyPair = await this.props.repo.crypto.encryptionKeyPair()

const senderPKey = Crypto.encode(encryptionKeyPair.publicKey)

I get

Argument of type 'EncodedPublicSigningKey' is not assignable to parameter of type 'EncodedPublicEncryptionKey'

But sealedBox of CryptoClient: https://github.com/automerge/hypermerge/blob/90ed6241d4f49f6da30bc01e0b9770d3f90237a9/src/CryptoClient.ts#L95
requires Crypto.EncodedPublicEncryptionKey type as input

[mjtognetti]

Hi Raphael! I worked on the sharing/invite stuff and might be able to help - with a re-emphasis on @pvh's disclaimer on pushpin's security and the the important caveat that I'm not a security expert by any means.

A few notes:

• Keep in mind that the sealed box is being written into the sender's contact doc. The "outbox" model used by pushpin is maybe an inverse of how you would expect invites to work - if User A wants to share a document with User B, User A writes the invitation into their (User A's) own contact document. User B watches the contact documents of everyone they know to see if anyone has shared anything new with them. Consider whether this makes sense for your use case.
• For sealed boxes, the public key passed to crypto.sealedBox should be the recipient's public key, rather than the sender's. I should have made the argument names reflect this!
• I do not know if sealed boxes could potentially leak the recipient if an observer also has the recipient's public key. You would need an expert's advice on this.
• Given the discussion and code snippet above, I would guess you're looking for something like: crypto.sealedBox(recipientPk, 'for_you')
• You typically won't need to do any encoding/decoding of the keys. repo.crypto handles this for you. For example, if you look at the return type of crypto.encryptionKeyPair(), it's an EncodedEncryptionKeyPair - so no need to encode again!

There also might be a few system/design considerations that come with this change:

• Currently, invites is a map from recipientUrl to a list of boxed document urls. When a sender wants to offer a new document to a recipient, they can append the boxed document url to the invites list for the recipient (see here). If using sealed boxes instead of recipient urls, there won't be anyway for the sender to know which sealed box maps to which recipient - so (without changing anything else) invites will end up being a map of sealedBox: [oneDocumentBox] with no way to push/append documents in the future. This should actually work fine as long as recipients iterate the entire list - but something to think about!
• Its probably worth noting that the presence of a value with a contact doc is not an assurance that the owner of that contact doc actually put it there. Anyone can write to any document, as long as they know the url (@pvh correct me if I'm wrong or misremembering here). The current workaround for this is to sign/verify messages, which you can see here. What this means (if not using signing/verification) is that a User C can write a sealed box invite for User B into User A's contact doc, giving the illusion that User A is sharing that document with User B. Notably, the boxes which actually contain the document url do have some sender verification built in since they rely on keys from both parties. You would need to think about potential interactions here.

Great to see work being done here! I'll follow along and answer questions as best I can.

[raphael10-collab]

Thank you Matt for this in-depth examination of my very first attempt to encode some sort of privacy around the receiver-sender "channel".

It seems to me that the sealedBox tool, which I thought it could be used for "inflating" privacy, has more contraindications that positive aspects, if we view it with a system/design perspective:

• encrypt a message in a sealed box, hiding the association between the sender and the cyphered text, does hide the specific
  sender to the rest of the users (positive outcome), but it also hampers the sender verification mechanism, which verifies, if I
  understood correctly, that the Contact Doc's invites are indeed written only by the contact Doc's owner and not by other users.

  This sender verification mechanism is needed because, by design, everyone can write to every documents, and thus, can write
  to every contact docs as well.

• The recipient of a sealed box can be identified by its public key. So I do not understand why, if without changing anything else,
  there won't be no ways for the sender to know which sealed box maps to which recipient.
  But, anyway, if the map invites end up being a map of sealedBox: [oneDocumentBox], the very essence of the map invites
  will be distorted, because:

  • it becomes a single key-value pair,
    and
  • it doesn't collect anymore all the invitations of documents sharing written by the user, owner of the Contact Doc containing the
    map invites.

So the use of sealedBox tool would be disruptive, because it would destroy the main sharing mechanism: the map, owned by
each user, of her/his invitations to share documents to and with other users.

Set aside the sealedBox tool,
a question:

if the architrave of the whole "sharing mechanism" is this map invitations, map owned, within its own Contact Doc, by each user, would make sense, and would it be feasible, to make the Contact Doc writable only by its user, and to make it read-only by the other users?
In this way the sender verification mechanism wouldn't be needed, and each user would gain more control over its contact doc, the document which keeps track of her/his documents sharing invitations to other users.

And what if the other users would be able to read in the map invitations only the key that concerns them?
In this way it would be solved also this privacy, and thus, security, problem :
"Shared document contents are private, but anyone who has encountered your profile can see the contact document for anyone you have shared data with"

[raphael10-collab]

@pvh @mjtognetti Did I say something wrong or unfeasible?

[mjtognetti]

@raphael10-collab I think your original attempt was pointed in the right general direction. Were you able to get the recipient public key encryption working?

The recipient of a sealed box can be identified by its public key

The issue here is that crypto.sealedBox will generate different ciphertext every time, and the sender isn't able to decrypt the contents. This means the sender won't be able to tell which sealedBox corresponds with which recipient.

The characteristics of crypto.sealedBox wouldn't necessarily prevent it from working in this case, but they have implications worth considering. crypto.box is an alternative that you might want to look at. If useful, the underlying crypto functions are documented here.

To your questions:

would it be feasible, to make the Contact Doc writable only by its user, and to make it read-only by the other users?

Write permissions/capabilities have long been on the todo list, but are tricky to get right with pushpin's document model. One strategy you could use immediately would be to make use of the sign/verify functions. They won't prevent other users from writing to a document, but you'll be able to verify whether or not a message was written by the owner of the document.

As an example, we use these functions to ensure the public key written to a contact document was actually written by the document owner (and not anyone else): https://github.com/automerge/pushpin/blob/master/src/renderer/components/content-types/workspace/omnibox/OmniboxWorkspaceListMenu.tsx#L485

what if the other users would be able to read in the map invitations only the key that concerns them?

Because of pushpin's document distribution model, the closest you could get to this with the "outbox" model would be using encryption to make sure only the recipient can read the data. crypto.box and crypto.sealedBox were built into hypermerge for exactly this purpose.

[raphael10-collab]

@mjtognetti

I created an object containing the recipientUrl and the recipient public key:

const recipientObj = {rUrl: recipientUrl, recipientPKey: recipientPublicKey}
const recipientObjStringified = JSON.stringify(recipientObj)

Based on the signatures of box and openBox functions in CryptoClient, I did this:

let pk, sk = sodium.crypto_box_easy() // public and secret keys to be used once for the sender

const boxedRecipientObj = await this.props.repo.crypto.box(
  sk,
  recipientPublicKey,
  recipientObjStringified
)

this.props.repo.change (workspace.selfId, (s: ContactDoc) => {
  if (!s.invites) {
    s.invites = {}
  }
  if (!s.invites[recipientUrl]) {
    s.invites[recipientUrl] = []
  }
  // TODO: prevent duplicate shares.
  //s.invites[recipientUrl].push(box)
  s.invites[recipientUrl].push(boxedRecipientObj) 
})

In InvitationsView.tsx I'm having difficulty in properly getting the recipientPublicKey:

  watchContact = async (contactId: HypermergeUrl) => {
    if (this.contactHandles[contactId]) {
      return
    }
    const workspace = await getDoc<Doc>(this.repo, this.workspaceHandle.url)
    const recipientSecretKey =
      workspace.secretKey &&
      (await this.repo.crypto.verifiedMessage(this.workspaceHandle.url, workspace.secretKey))
    if (!recipientSecretKey) {
      return
    }

    const recipientContactDoc = this.workspaceHandle.state
    if (!recipientContactDoc) {
      return
    }

    this.contactHandles[contactId] = this.repo.watch(contactId, async (sender) => {
      const senderUrl = contactId
      if (!sender.invites) {
        return
      }
      const senderPublicKey =
        sender.encryptionKey &&
        (await this.repo.crypto.verifiedMessage(senderUrl, sender.encryptionKey))
      if (!senderPublicKey) {
        return
      }

      const invitations = (this.selfId && sender.invites[this.selfId]) || []

      invitations.forEach(async (box) => {
        //const documentUrl = await this.repo.crypto.openBox(senderPublicKey, recipientSecretKey, box)
        const recipientObjStringified = await this.repo.crypto.openBox(senderPublicKey, recipientSecretKey, box)
        const recipientObject = JSON.parse(recipientObjStringified)
        const recipientPublicKey =
          recipientContactDoc.encryptionKey &&
          (await this.repo.crypto.verifiedMessage(recipientObject["recipientPKey"], recipientContactDoc.encryptionKey))
        if (!recipientPublicKey) {
          throw new Error("Recipient doesn't match")
        }
        const documentUrl = recipientObject["document"]
        const { hypermergeUrl } = parseDocumentLink(documentUrl)
        const matchOffer = (offer: Invitation) =>
          offer.documentUrl === documentUrl && offer.offererId === senderUrl

        if (!this.pendingInvitations.some(matchOffer)) {
          this.pendingInvitations.push({
            documentUrl: documentUrl as PushpinUrl,
            offererId: senderUrl,
            sender,
            hypermergeUrl,
          })
          this.watchDoc(hypermergeUrl)
        }
      })
    })
  }

I'm getting this error:

Property 'encryptionKey' does not exist on type 'FreezeObject<Doc>'.
const recipientPublicKey =
  recipientContactDoc.encryptionKey &&

I also have two more questions:

Here : https://libsodium.gitbook.io/doc/public-key_cryptography/authenticated_encryption
I read :
"Using public-key authenticated encryption, Bob can encrypt a confidential message specifically for Alice, using Alice's public key.
Using Bob's public key, Alice can compute a shared secret key. Using Alice's public key and his secret key, Bob can compute the exact same shared secret key. That shared secret key can be used to verify that the encrypted message was not tampered with, before eventually decrypting it.
Alice only needs Bob's public key, the nonce and the ciphertext. Bob should never ever share his secret key, even with Alice.
And in order to send messages to Alice, Bob only needs Alice's public key. Alice should never ever share her secret key either, even with Bob.
Alice can reply to Bob using the same system, without having to generate a distinct key pair."

So, If I understand correctly, sender (Bob) and receiver (Alice) can send each other only their public keys, public keys which let them, in conjunction with their personal secret key, to encrypt and decrypt the messages they exchange.
And, importantly, their secret keys must be kept, secret, and not shared with (sent) the other person.

But here: https://libsodium.gitbook.io/doc/public-key_cryptography/authenticated_encryption#combined-mode

int crypto_box_easy(unsigned char *c, const unsigned char *m,
                    unsigned long long mlen, const unsigned char *n,
                    const unsigned char *pk, const unsigned char *sk);

where: sk = sender's secret key , which I thought it should have been kept secret and not exchanged with the receiver
n = nonce

The signature of the function box in CryptoClient: https://github.com/automerge/hypermerge/blob/master/dist/CryptoClient.d.ts#L15

 box(senderSecretKey: Crypto.EncodedSecretEncryptionKey, recipientPublicKey: Crypto.EncodedPublicEncryptionKey, 
message: string): Promise<Crypto.Box>

requires the sender secret key, but no nonce.

On the other hand,
the signature of crypto_box_open_easy:

int crypto_box_open_easy(unsigned char *m, const unsigned char *c,
                         unsigned long long clen, const unsigned char *n,
                         const unsigned char *pk, const unsigned char *sk);

which, according to the libsodium documentation, verifies and decrypts a chipertext produced by crypto_box_easy(),
requires pk = public key of the sender
sk = secret key of the receiver
n = nonce which has to match the nonce used to encrypt and authenticate the message

and the signature of the function openbox in CryptoClient: https://github.com/automerge/hypermerge/blob/master/dist/CryptoClient.d.ts#L16

openBox(senderPublicKey: Crypto.EncodedPublicEncryptionKey, recipientSecretKey: Crypto.EncodedSecretEncryptionKey, 
box: Crypto.Box): Promise<string>;

requires the sender public key and no nonce

So.. :

1. How to properly get recipientPublicKey in InvitationsView?

2. Do I need to use a nonce with the functions box and openBox of CryptoClient ?

3. Is it correct to send to the receiver (Alice) Bob's secret key?

[mjtognetti]

ok! I have a few questions to help me understand the intent here:

• Is there a reason you're trying to generate keys with let pk, sk = sodium.crypto_box_easy() instead of using the existing sender and recipient keys as done here?
• Given it's not needed for decryption, what is the reason for wanting the recipient public key in InvitationsView? Since the box is encrypted with the recipientPublicKey and decrypted with the recipientSecretKey, only the recipient with the appropriate keys will be able to decrypt the box - so its not clear to me what the intent with the public key in InvitationsView is.

Overall, it may be worth spending extra time getting familiar with the internals of the sharing system as well as the underlying crypto primitives. This is particularly important since you want to use encryption. I think this would help answer some of your questions and provide some clarity on how to accomplish your goal.

Some specific areas I would recommend looking at:

CryptoClient.ts and Crypto.ts in hypermerge

All of the repo.crypto functions are directly passed through to CryptoClient.ts, which in turn uses the functions in Crypto.ts. The difference between these is that CryptoClient.ts runs in the renderer process (and so has to be lightweight) whereas Crypto.ts runs in a separate node process and does all of the actual cryptography.

Specifically, take a look at the implementations of box, openBox, the sealed box equivalents, and the types in Crypto.ts. For example, the box implementation and the Box interface will help answer your question about the nonces. You can see where/how they're generated and used.

The underlying crypto library

The docs you've listed for authenticated encryption are a great start. One example to illustrate why this might be useful: in your code above you're using let pk, sk = sodium.crypto_box_easy() to try generate keys. However, this function doesn't generate keys, it uses them to encrypt a message.

It's worth noting, given your question about secret keys, that crypto_box_easy and crypto_box_open_easy only use the secret keys to encrypt/decrypt the message. They do not share those secret keys and secret keys should not be shared.

That being said, you probably won't need to use sodium directly. repo.crypto will most likely provide all of the functionality you'll need.

How workspace documents and contact documents work together

The document returned by this.workspaceHandle in InvitationsView.tsx is the current user's workspace document. This document contains the user's secret key and should not be shared. Contact docs contain public keys, which can be shared. This is why you see the senderPublicKey being retrieved from the sender's contact doc and the recipient's secret key from the workspace doc.

In InvitationsView the current user is the recipient and the contact docs belong to senders. If you want to find the recipient's public key from InvitationsView.tsx you'll need to find the current user's contact doc, which you can find on the workspace doc