[WIP] Add x3dh key exchange #1127

cammellos · 2018-07-30T08:57:08Z

This PR adds x3dh key exchange to the protocol.

It adds a new layer sitting between the transport (whisper) and the higher level protocol (chat).
This layer only understand 3 type of messages, public/broadcast (not encrypted) so noops, direct messages and eventually multicast encrypted (not implemented in this iteration).

To keep compatibility it will need to "fingerprint" messages and decrypt the new ones, leaving the old ones unchanged, for now it just naively tries to decrypt them.

crypto.go is the same encryption used in whisper.

Currently persistence.go uses leveldb and stores keys not encrypted on the filesystem, I made it an interface and the next step is to replace it using a database that supports file encryption.

cammellos · 2018-07-30T09:00:12Z

services/shhext/api.go

+		// Msg.Dst is empty is a public message, nothing to do
+		if msg.Dst != nil {
+			// There's probably a better way to do this
+			keyBytes, err := hexutil.Bytes(msg.Dst).MarshalText()


Is there a better way for this?

fmt.Sprintf()? It should call MarshalText internally.

hexutil.Bytes() has also String() method. fmt.Sprintf() will probably call String() otherwise it would also need to return err. Btw. MarshalText() will prefix the output with 0x. Is that what you want?

cammellos · 2018-07-30T09:01:38Z

services/shhext/api.go

+			}
+
+			// This needs to be pushed down in the protocol message
+			publicKey, err := crypto.UnmarshalPubkey(msg.Sig)


For now we rely on whisper to ensure authentication of the message (whisper verifies the signature), eventually we want to move the authentication layer in the encryption layer, but for now this step relies on whisper/transport.

cammellos · 2018-07-30T09:02:25Z

services/shhext/api.go

+
+			payload, err := api.service.protocol.HandleMessage(privateKey, publicKey, msg.Payload)
+
+			// Ignore errors for now


We ignore errors for now, we should probably have a better fingerprint for old messages, so we don't even attempt to decrypt, I will look into this.

adambabik

First-round :) Overall, looks great!

I have some questions about bundles. We have two types of bundles: public, private and symmetric key in the PersistenceService.

Public bundles are bundles obtained from other contacts, correct?

Private bundles are my bundles? GetAnyPrivateBundle method allows me to get or create any private bundle. Why is it ok to return any bundle? Can a single bundle be used multiple times for different contacts?

Symmetric keys are used for the old protocol encryption?

adambabik · 2018-07-30T10:31:12Z

api/backend.go

+func (b *StatusBackend) CreateX3DHBundle() (string, error) {
+	selectedAccount, err := b.AccountManager().SelectedAccount()
+	if selectedAccount == nil || err == account.ErrNoAccountSelected {
+		return "", nil


Shouldn't we return account.ErrNoAccountSelected as error?

adambabik · 2018-07-30T10:32:57Z

api/backend.go

+		return "", err
+	}
+
+	jsonBundle, err := bundle.ToJSON()


This can be implemented with json.Marshaler and you can then call it like this: jsonBundle, err := json.Marshal(bundle)

adambabik · 2018-07-30T10:34:12Z

lib/library.go

@@ -61,6 +62,20 @@ func StopNode() *C.char {
 	return makeJSONResponse(nil)
 }

+// Create an X3DH bundle
+//export CreateX3DHBundle
+func CreateX3DHBundle() *C.char {


We also have func (b *StatusBackend) CreateX3DHBundle() (string, error). Do we need both?

adambabik · 2018-07-30T10:36:08Z

lib/library.go

+
+	cstr := C.CString(bundle)
+
+	defer C.free(unsafe.Pointer(cstr))


It might crash the program. defer is called before the context is switched to the caller and when it is, the memory pointed by cstr is already gone so the caller won't have a chance to read it. Or my thinking is wrong here and it does work?

@adambabik I actually wanted to check, is not currently used this method, it will be used only to share the bundle with your contact code, this part needs to be looked up a bit.

This is definitely gonna blow up.

@adambabik This has been a recurring issue in many PRs. Is it something that'll get solved soon with gomobile? Otherwise, maybe we should do a PR that exposes a method to the caller (status-react) to free memory.

adambabik · 2018-07-30T10:41:13Z

services/shhext/api.go

+		// Msg.Dst is empty is a public message, nothing to do
+		if msg.Dst != nil {
+			// There's probably a better way to do this
+			keyBytes, err := hexutil.Bytes(msg.Dst).MarshalText()


hexutil.Bytes() has also String() method. fmt.Sprintf() will probably call String() otherwise it would also need to return err. Btw. MarshalText() will prefix the output with 0x. Is that what you want?

adambabik · 2018-07-30T10:41:32Z

services/shhext/api.go

+				return nil, err
+			}
+
+			privateKey, err := api.service.w.GetPrivateKey(string(keyBytes))


string() type conversion not needed probably.

adambabik · 2018-07-30T10:48:26Z

services/shhext/chat/encryption.go

+	return key, bundle.GetSignedPreKey(), ephemeralKey, nil
+}
+
+func (s *EncryptionService) keyFromDH(pk *ecdsa.PublicKey, privateKey *ecdsa.PrivateKey, payload []byte) ([]byte, *ecdsa.PublicKey, error) {


Is this method needed? It seems like only the first argument is required.

@adambabik no, good spot, I have changed method signature at some point and didn't update the code

adambabik · 2018-07-30T10:52:02Z

services/shhext/chat/encryption.go

+	// If not there try with a bundle and store the key
+	if symmetricKey == nil {
+		encryptionType = EncryptionTypeX3DH
+		symmetricKey, bundleID, ourEphemeralKey, err = s.keyFromX3DH(theirIdentityKey, myIdentityKey)


err should be checked.

adambabik · 2018-07-30T10:53:59Z

services/shhext/chat/persistence.go

+
+var publicKeyPrefix = []byte{0x10, 0x11}
+var privateBundleKeyPrefix = []byte{0x10, 0x12}
+var symmetricKeyKeyPrefix = []byte{0x10, 0x13}


s/symmetricKeyKeyPrefix/symmetricKeyPrefix

adambabik · 2018-07-30T10:58:27Z

services/shhext/chat/protocol.go

+	x3dhKey := msg.GetBundleKey()
+	bundleID := msg.GetBundleId()
+	if x3dhKey != nil {
+		decompressedKey, err := crypto.DecompressPubkey(symKeyID)


Shouldn't symKeyID be x3dhKey?

correct, thanks!

cammellos · 2018-07-30T11:47:46Z

@adambabik thanks for the comments!

Public bundles are bundles obtained from other contacts, correct?

Correct, contacts but also they are shared in public chats

Private bundles are my bundles? GetAnyPrivateBundle method allows me to get or create any private bundle. Why is it ok to return any bundle? Can a single bundle be used multiple times for different contacts?

It's ok to use the same bundle, although we will want to rotate it every x, but that won't be in the first iteration. Eventually we will have bundle specific to each contact. So if you send to a public chat you share your "shared" bundle, if you send a one-to-one you will propagate a specific bundle (with one-time pre keys), but again that will be not in this iteration.

Symmetric keys are used for the old protocol encryption?

Every time a successful x3dh key exchange is performed we store the sym key generated, which is the one we will use for any successive communication, when we add a ratchet it will be the shared secret to boot the ratchet.

adambabik · 2018-07-30T12:35:05Z

@cammellos thanks for the clarification. Totally makes sense with x3dh and symmetric keys.

I still don't get why it's ok to call:

func (s *EncryptionService) CreateBundle(privateKey *ecdsa.PrivateKey) (*Bundle, error) {
  // If there is any bundle, it will be always returned here
  bundle, err := s.persistence.GetAnyPrivateBundle()
  // ...  
}

Is this ok because of

It's ok to use the same bundle
and the fact that we want to change that in the next iteration?

cammellos · 2018-07-30T12:44:55Z

@adambabik
basically you can re-use bundles as you will always generate a different sym-key as x3dh depends on the other user identity key & ephemeral key (which always changes).

It is recommend though to refresh the bundle every week/month and use one time pre-keys.

So for the first iteration we will use the same bundle and no one-time pre-keys (which still provides the same guarantees, but not in case of key compromise), just to make sure poc works and everything is ok.

Then the next iteration is to:

Rotate the bundle at a time interval
Use one-time pre keys for contacts

Does it clarify a bit?

adambabik · 2018-07-30T12:57:11Z

@cammellos yes, perfectly clear now. Thanks!

gravityblast · 2018-07-31T09:18:46Z

services/shhext/api.go

+				return nil, err
+			}
+
+			privateKey, err = api.service.w.GetPrivateKey(string(keyBytes))


@cammellos as I understand keyBytes is the public key of the recipient, can you call GetPrivateKey with that? I thought you had to pass the id but maybe I'm missing something

ah I found this in the docs:

// Originally, public keys // were used as keys, now random keys are being used. And in order to // make it easier to consume, we now allow both random IDs and public // keys to be passed. func toDeterministicID(id string, expectedLen int) (string, error) {

but does it work with public key for a key created previously?

It seems to be working for older clients ( I have them successfully communicate over public and 1-to-1 chats), but what do you mean exactly created previously? How long ago? (Status-react only stores the key as pk, not the ID)

gravityblast · 2018-07-31T09:22:58Z

services/shhext/api.go

+		var privateKey *ecdsa.PrivateKey
+		var publicKey *ecdsa.PublicKey
+
+		// Msg.Dst is empty is a public message, nothing to do


are we going to always use the recipient public key in the message (msg.Dst) ?

So:

Messages to public chat will not have msg.Dst as not sent to a particular user

One to one messages will have it for now as we use asym encryption in whisper, eventually msg.Dst will be moved inside the encryption layer so we don't rely anymore on it being passed at the transport layer, and the encryption layer is self-sufficient, but that will come in a future iteration ( We need to make it play well with plausible deniability)

Does it answer your question?

ok I understand so after this we are not using the topic anymore for one to one chat right?

We already stopped using topics for one to one ( we disabled them on beta), all the messages go through the discovery topic, the whisper layer will be unchanged after this.

ok perfect then thank you!!

status-im-bot · 2018-08-08T12:34:15Z

Can one of the admins verify this patch?

pedropombeiro · 2018-09-10T14:02:34Z

services/shhext/chat/encryption.go

@@ -130,7 +130,15 @@ func (s *EncryptionService) ProcessPublicBundle(b *Bundle) error {
 }

 // DecryptPayload decrypts the payload of a DirectMessageProtocol, given an identity private key and the sender's public key
-func (s *EncryptionService) DecryptPayload(myIdentityKey *ecdsa.PrivateKey, theirIdentityKey *ecdsa.PublicKey, msg *DirectMessageProtocol) ([]byte, error) {
+func (s *EncryptionService) DecryptPayload(myIdentityKey *ecdsa.PrivateKey, theirIdentityKey *ecdsa.PublicKey, msgs *map[string]*DirectMessageProtocol) ([]byte, error) {
+	msg := (*msgs)[s.installationID]


@cammellos what should happen if msgs is nil?

pedropombeiro

Added some comments. Also, do we want to add some logic in NodeConfig.Validate to return an error if PFS is enabled but InstallationID isn't valid?

pedropombeiro · 2018-09-17T09:15:36Z

services/shhext/service.go

@@ -67,6 +79,36 @@ func (s *Service) Protocols() []p2p.Protocol {
 	return []p2p.Protocol{}
 }

+func (s *Service) InitProtocol(address string, password string) error {


Public method missing comment

pedropombeiro · 2018-09-17T09:16:23Z

services/shhext/service.go

+
+func (s *Service) ProcessPublicBundle(myIdentityKey *ecdsa.PrivateKey, bundle *chat.Bundle) error {
+	if s.protocol == nil {
+		return errors.New("Procotol is not initialized")


Error could be extracted to a variable. Error message should start with lowercase.

pedropombeiro · 2018-09-17T09:19:46Z

services/shhext/service.go

-// New returns a new Service.
-func New(w *whisper.Whisper, handler EnvelopeEventsHandler, db *leveldb.DB, debug bool) *Service {
+// New returns a new Service. dataDir is a folder path to a network-independent location
+func New(w *whisper.Whisper, handler EnvelopeEventsHandler, db *leveldb.DB, dataDir string, installationID string, debug bool, pfsEnabled bool) *Service {


Maybe it would be better to pass at least some of these arguments in a struct? The code that calls this function is not very readable currently.

pedropombeiro · 2018-09-17T09:21:15Z

services/shhext/api.go

+func (api *PublicAPI) SendDirectMessage(ctx context.Context, msg chat.SendDirectMessageRPC) ([]hexutil.Bytes, error) {
+
+	if !api.service.pfsEnabled {
+		return nil, errors.New("PFS not enabled")


Could be extracted to a var

pedropombeiro · 2018-09-17T09:21:23Z

services/shhext/api.go

+
+// SendDirectMessage sends a 1:1 chat message to the underlying transport
+func (api *PublicAPI) SendDirectMessage(ctx context.Context, msg chat.SendDirectMessageRPC) ([]hexutil.Bytes, error) {
+


pedropombeiro · 2018-09-17T09:32:45Z

services/shhext/service.go

+
+func (s *Service) GetBundle(myIdentityKey *ecdsa.PrivateKey) (*chat.Bundle, error) {
+	if s.protocol == nil {
+		return nil, errors.New("Procotol is not initialized")


lowercase message

pedropombeiro · 2018-09-17T09:33:20Z

signal/events_shhext.go

@@ -20,6 +20,9 @@ const (

 	// EventEnodeDiscovered is tiggered when enode has been discovered.
 	EventEnodeDiscovered = "enode.discovered"
+
+	// CouldNotDecrypt is triggered when we receive a message from a bundle we don't have


Out of date CouldNotDecrypt?

pedropombeiro · 2018-09-17T09:33:38Z

signal/events_shhext.go

@@ -34,6 +37,11 @@ type MailServerResponseSignal struct {
 	Cursor           string      `json:"cursor"`
 }

+// DecryptFailedSignal holds the sender of the message that could not be decrypted


Out of date DecryptFailedSignal?

pedropombeiro · 2018-09-17T09:44:02Z

Makefile

@@ -188,6 +188,9 @@ endif
 	docker push $(BOOTNODE_IMAGE_NAME):latest
 	docker push $(DOCKER_IMAGE_NAME):latest

+protoc-install:
+	apt install -y protobuf-compiler


We should move this to a script in _assets/scripts so that we can deal more easily with multiple host platforms.

pedropombeiro · 2018-09-17T09:46:09Z

api/backend.go

+
+func (b *StatusBackend) ProcessContactCode(contactCode string) error {
+	selectedAccount, err := b.AccountManager().SelectedAccount()
+	if selectedAccount == nil || err == account.ErrNoAccountSelected {


Is this code correct for the case where selectedAccount is nil (what error do we want to return)?

adambabik · 2018-09-18T09:40:56Z

_assets/compose/wnode-test-cluster/wnode-config.json

@@ -1,6 +1,7 @@
 {
    "NetworkId": 777,
    "DataDir": "/data/ethereumtest/status",
+    "NoBackupDataDir": "/data/ethereumtest/status",


What does NoBackupDataDir stand for? Why is it prefixed with No?

Stands for no as is dont. Because we don't want to save in directories that have the automatic backup by default (in android you can disable it, but they provide a no_backup directory, in ios I think it's Library).

adambabik · 2018-09-18T09:42:42Z

_assets/scripts/install_deps.sh

+	apt install -y protobuf-compiler
+fi
+
+if [ -x "$(command -v pacman)" ]; then


Please add macOS support. I believe it's this package https://formulae.brew.sh/formula/protobuf.

if [ -x "$(command -v brew)" ]; then brew install protobuf fi

adambabik · 2018-09-18T09:43:53Z

api/backend.go

+			return err
+		}
+
+		err = st.InitProtocol(address, password)


This is a bit tricky for anyone extending this method.

if err := st.InitProtocol(address, password); err != nil { return err }

adambabik · 2018-09-18T09:44:57Z

api/backend.go

@@ -431,3 +443,58 @@ func appendIf(condition bool, services []gethnode.ServiceConstructor, service ge
 	}
 	return append(services, service)
 }
+
+func (b *StatusBackend) CreateContactCode() (string, error) {


Please add a comment what this method does.

adambabik · 2018-09-18T09:45:20Z

api/backend.go

+		return "", err
+	}
+
+	base64Bundle, err := bundle.ToBase64()


Just return bundle.ToBase64() will work.

adambabik · 2018-09-18T09:46:45Z

lib/library.go

+
+	cstr := C.CString(bundle)
+
+	defer C.free(unsafe.Pointer(cstr))


This is definitely gonna blow up.

adambabik · 2018-09-18T10:13:44Z

services/shhext/chat/encryption.go

+	log            log.Logger
+	persistence    PersistenceServiceInterface
+	installationID string
+	mutex          *sync.Mutex


mutex sync.Mutex is fine, no need to make it a pointer. A default value is unlocked.

I don't think it is used btw.

adambabik · 2018-09-18T10:16:43Z

services/shhext/chat/encryption.go

+}
+
+// DecryptPayload decrypts the payload of a DirectMessageProtocol, given an identity private key and the sender's public key
+func (s *EncryptionService) DecryptPayload(myIdentityKey *ecdsa.PrivateKey, theirIdentityKey *ecdsa.PublicKey, msgs *map[string]*DirectMessageProtocol) ([]byte, error) {


msgs map[string]*DirectMessageProtocol will be fine. maps are passed by a pointer.

adambabik · 2018-09-18T10:18:50Z

services/shhext/chat/persistence.go

+}
+
+// PersistenceServiceInterface defines the interface for a storage service
+type PersistenceServiceInterface interface {


Minor: Interfaces usually are called without Interface suffix.

adambabik · 2018-09-18T10:23:42Z

services/shhext/chat/sql_lite_persistence.go

+	}
+
+	for installationID, signedPreKey := range b.GetBundle().GetSignedPreKeys() {
+		stmt, err := tx.Prepare("insert into bundles(identity, private_key, signed_pre_key, installation_id, timestamp) values(?, ?, ?, ?, ?)")


In the queries below, SQL keywords are uppercase.

adambabik · 2018-09-18T10:25:16Z

services/shhext/chat/sql_lite_persistence.go

+}
+
+// NewSQLLiteKeysStorage creates a new SQLLiteKeysStorage instance associated with the specified database
+func NewSQLLiteKeysStorage(db *sql.DB) *SQLLiteKeysStorage {


Constructors, struct declarations and methods are a bit mixed. It would be great to group them per type.

cammellos self-assigned this Jul 30, 2018

cammellos commented Jul 30, 2018

View reviewed changes

adambabik reviewed Jul 30, 2018

View reviewed changes

gravityblast reviewed Jul 31, 2018

View reviewed changes

cammellos force-pushed the features/x3dh branch from e25b49d to 7016ff4 Compare August 6, 2018 05:42

pedropombeiro force-pushed the features/x3dh branch from 6dd4f35 to f696396 Compare August 13, 2018 15:22

cammellos force-pushed the features/x3dh branch 2 times, most recently from a45683d to 2aaa38f Compare August 21, 2018 08:34

pedropombeiro force-pushed the features/x3dh branch 2 times, most recently from e0d6366 to 5575895 Compare August 21, 2018 13:37

cammellos force-pushed the features/x3dh branch 7 times, most recently from 7a2d751 to 048096c Compare August 27, 2018 10:33

pedropombeiro force-pushed the features/x3dh branch 4 times, most recently from 0daf45a to 6c73968 Compare August 29, 2018 16:17

pedropombeiro reviewed Sep 10, 2018

View reviewed changes

cammellos force-pushed the features/x3dh branch from 189025d to a357aff Compare September 14, 2018 07:01

pedropombeiro force-pushed the features/x3dh branch 2 times, most recently from 8e26c60 to 1699d72 Compare September 14, 2018 09:10

cammellos force-pushed the features/x3dh branch 3 times, most recently from 1c5a67c to 9c9930e Compare September 17, 2018 07:31

pedropombeiro suggested changes Sep 17, 2018

View reviewed changes

cammellos force-pushed the features/x3dh branch 2 times, most recently from 1bf8c84 to 5df33a8 Compare September 17, 2018 10:40

pedropombeiro mentioned this pull request Sep 17, 2018

Status Devs Meeting 4 Agenda status-im/pm#4

Closed

cammellos force-pushed the features/x3dh branch from 5df33a8 to 8230f9f Compare September 17, 2018 10:59

adambabik reviewed Sep 18, 2018

View reviewed changes

adambabik suggested changes Sep 18, 2018

View reviewed changes

cammellos force-pushed the features/x3dh branch 4 times, most recently from 1beb6fb to 826b5e5 Compare September 18, 2018 12:23

adambabik approved these changes Sep 18, 2018

View reviewed changes

pedropombeiro approved these changes Sep 18, 2018

View reviewed changes

cammellos force-pushed the features/x3dh branch 4 times, most recently from b0b11b6 to 16773d6 Compare September 24, 2018 17:17

cammellos added 4 commits September 24, 2018 19:25

Add x3dh key exchange

2ba78f4

Encrypt using the double ratchet

ff6da1c

Multi device with auto-pairing

0f475e6

Add pfs enabled flag

1ac9dd9

cammellos force-pushed the features/x3dh branch from 16773d6 to 1ac9dd9 Compare September 24, 2018 17:25

cammellos merged commit dcaf8ca into develop Sep 24, 2018

adambabik deleted the features/x3dh branch December 27, 2019 10:06


		payload, err := api.service.protocol.HandleMessage(privateKey, publicKey, msg.Payload)

		// Ignore errors for now


		// SendDirectMessage sends a 1:1 chat message to the underlying transport
		func (api *PublicAPI) SendDirectMessage(ctx context.Context, msg chat.SendDirectMessageRPC) ([]hexutil.Bytes, error) {

[WIP] Add x3dh key exchange #1127

[WIP] Add x3dh key exchange #1127

Conversation

cammellos commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedropombeiro Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cammellos commented Jul 30, 2018 • edited Loading

adambabik commented Jul 30, 2018

cammellos commented Jul 30, 2018

adambabik commented Jul 30, 2018

gravityblast Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

status-im-bot commented Aug 8, 2018

Choose a reason for hiding this comment

pedropombeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedropombeiro Sep 18, 2018 •

edited

Loading

cammellos commented Jul 30, 2018 •

edited

Loading

gravityblast Jul 31, 2018 •

edited

Loading