Feedback

backend/zkppSalt.js

@@ -1,7 +1,8 @@
 import sbp from '@sbp/sbp'
 import { randomBytes, timingSafeEqual } from 'crypto'
 import nacl from 'tweetnacl'
-import { base64ToBase64url, base64urlToBase64, boxKeyPair, computeCAndHc, encryptContractSalt, encryptSaltUpdate, decryptSaltUpdate, hash, hashRawStringArray, hashStringArray, parseRegisterSalt, randomNonce } from '~/shared/zkpp.js'
+import { base64ToBase64url, base64urlToBase64, boxKeyPair, computeCAndHc, decryptSaltUpdate, encryptContractSalt, encryptSaltUpdate, hash, hashRawStringArray, hashStringArray, parseRegisterSalt, randomNonce } from '~/shared/zkpp.js'
+import { AUTHSALT, CONTRACTSALT, SU } from '~/shared/zkppConstants.js'
 
 // used to encrypt salts in database
 let recordSecret: string
@@ -296,7 +297,7 @@ export const updateContractSalt = async (contract: string, r: string, s: string,
     throw new Error('update: Bad challenge')
   }
 
-  const encryptionKey = hashRawStringArray('SU', c).slice(0, nacl.secretbox.keyLength)
+  const encryptionKey = hashRawStringArray(SU, c).slice(0, nacl.secretbox.keyLength)
   const encryptedArgsBuf = Buffer.from(base64urlToBase64(encryptedArgs), 'base64')
   const nonce = encryptedArgsBuf.slice(0, nacl.secretbox.nonceLength)
   const encryptedArgsCiphertext = encryptedArgsBuf.slice(nacl.secretbox.nonceLength)
@@ -317,8 +318,8 @@ export const updateContractSalt = async (contract: string, r: string, s: string,
       return false
     }
 
-    const authSalt = Buffer.from(hashStringArray('AUTHSALT', c)).slice(0, 18).toString('base64')
-    const contractSalt = Buffer.from(hashStringArray('CONTRACTSALT', c)).slice(0, 18).toString('base64')
+    const authSalt = Buffer.from(hashStringArray(AUTHSALT, c)).slice(0, 18).toString('base64')
+    const contractSalt = Buffer.from(hashStringArray(CONTRACTSALT, c)).slice(0, 18).toString('base64')
 
     const token = encryptSaltUpdate(
       hashUpdateSecret,

backend/zkppSalt.test.js

@@ -5,13 +5,14 @@ import should from 'should'
 import initDB from './database.js'
 import 'should-sinon'
 
+import { AUTHSALT, CONTRACTSALT, CS, SU } from '~/shared/zkppConstants.js'
 import { registrationKey, register, getChallenge, getContractSalt, updateContractSalt } from './zkppSalt.js'
 
 const saltsAndEncryptedHashedPassword = (p: string, secretKey: Uint8Array, hash: string) => {
   const nonce = nacl.randomBytes(nacl.secretbox.nonceLength)
   const dhKey = nacl.hash(nacl.box.before(Buffer.from(p, 'base64url'), secretKey))
-  const authSalt = Buffer.from(nacl.hash(Buffer.concat([nacl.hash(Buffer.from('AUTHSALT')), dhKey]))).slice(0, 18).toString('base64')
-  const contractSalt = Buffer.from(nacl.hash(Buffer.concat([nacl.hash(Buffer.from('CONTRACTSALT')), dhKey]))).slice(0, 18).toString('base64')
+  const authSalt = Buffer.from(nacl.hash(Buffer.concat([nacl.hash(Buffer.from(AUTHSALT)), dhKey]))).slice(0, 18).toString('base64')
+  const contractSalt = Buffer.from(nacl.hash(Buffer.concat([nacl.hash(Buffer.from(CONTRACTSALT)), dhKey]))).slice(0, 18).toString('base64')
   const encryptionKey = nacl.hash(Buffer.from(authSalt + contractSalt)).slice(0, nacl.secretbox.keyLength)
   const encryptedHashedPassword = Buffer.concat([nonce, nacl.secretbox(Buffer.from(hash), nonce, encryptionKey)]).toString('base64url')
 
@@ -75,7 +76,7 @@ describe('ZKPP Salt functions', () => {
 
     const saltBuf = Buffer.from(salt, 'base64url')
     const nonce = saltBuf.slice(0, nacl.secretbox.nonceLength)
-    const encryptionKey = nacl.hash(Buffer.concat([Buffer.from('CS'), c])).slice(0, nacl.secretbox.keyLength)
+    const encryptionKey = nacl.hash(Buffer.concat([Buffer.from(CS), c])).slice(0, nacl.secretbox.keyLength)
     const [retrievedContractSalt] = JSON.parse(
       Buffer.from(nacl.secretbox.open(saltBuf.slice(nacl.secretbox.nonceLength), nonce, encryptionKey)).toString()
     )
@@ -105,7 +106,7 @@ describe('ZKPP Salt functions', () => {
     const c = nacl.hash(Buffer.concat([nacl.hash(Buffer.from(hash)), nacl.hash(ħ)]))
     const hc = nacl.hash(c)
 
-    const encryptionKey = nacl.hash(Buffer.concat([Buffer.from('SU'), c])).slice(0, nacl.secretbox.keyLength)
+    const encryptionKey = nacl.hash(Buffer.concat([Buffer.from(SU), c])).slice(0, nacl.secretbox.keyLength)
     const nonce = nacl.randomBytes(nacl.secretbox.nonceLength)
 
     const encryptedArgsCiphertext = nacl.secretbox(Buffer.from(JSON.stringify(['a', 'b', 'c'])), nonce, encryptionKey)

frontend/controller/app/identity.js

@@ -7,7 +7,7 @@ import Vue from 'vue'
 import { LOGIN, LOGIN_COMPLETE, LOGIN_ERROR, NEW_PREFERENCES, NEW_UNREAD_MESSAGES } from '~/frontend/utils/events.js'
 import { Secret } from '~/shared/domains/chelonia/Secret.js'
 import { EVENT_HANDLED } from '~/shared/domains/chelonia/events.js'
-import { boxKeyPair, buildRegisterSaltRequest, buildUpdateSaltRequestEa, computeCAndHc, decryptContractSalt, hash, hashPassword, randomNonce } from '~/shared/zkpp.js'
+import { boxKeyPair, buildRegisterSaltRequest, buildUpdateSaltRequestEc, computeCAndHc, decryptContractSalt, hash, hashPassword, randomNonce } from '~/shared/zkpp.js'
 // Using relative path to crypto.js instead of ~-path to workaround some esbuild bug
 import { CURVE25519XSALSA20POLY1305, EDWARDS25519SHA512BATCH, deriveKeyFromPassword, serializeKey } from '../../../shared/domains/chelonia/crypto.js'
 import { handleFetchResult } from '../utils/misc.js'
@@ -202,7 +202,7 @@ export default (sbp('sbp/selectors/register', {
 
     const [c, hc] = computeCAndHc(r, s, h)
 
-    const [contractSalt, encryptedArgs] = await buildUpdateSaltRequestEa(newPassword.valueOf(), c)
+    const [contractSalt, encryptedArgs] = await buildUpdateSaltRequestEc(newPassword.valueOf(), c)
 
     const response = await fetch(`${sbp('okTurtles.data/get', 'API_URL')}/zkpp/${encodeURIComponent(username)}/updatePasswordHash`, {
       method: 'POST',
@@ -471,17 +471,17 @@ export default (sbp('sbp/selectors/register', {
   'gi.app/identity/logout': (...params) => {
     return sbp('okTurtles.eventQueue/queueEvent', 'APP-LOGIN', ['gi.app/identity/_private/logout', ...params])
   },
-  'gi.app/identity/changePassword': async (woldPassword: Secret<string>, wnewPassword: Secret<string>) => {
+  'gi.app/identity/changePassword': async (wOldPassword: Secret<string>, wNewPassword: Secret<string>) => {
     const state = sbp('state/vuex/state')
     if (!state.loggedIn) return
     const getters = sbp('state/vuex/getters')
 
     const { identityContractID } = state.loggedIn
     const username = getters.usernameFromID(identityContractID)
-    const oldPassword = woldPassword.valueOf()
-    const newPassword = wnewPassword.valueOf()
+    const oldPassword = wOldPassword.valueOf()
+    const newPassword = wNewPassword.valueOf()
 
-    const [newContractSalt, oldContractSalt, updateToken] = await sbp('gi.app/identity/updateSaltRequest', username, woldPassword, wnewPassword)
+    const [newContractSalt, oldContractSalt, updateToken] = await sbp('gi.app/identity/updateSaltRequest', username, wOldPassword, wNewPassword)
 
     const oldIPK = await deriveKeyFromPassword(EDWARDS25519SHA512BATCH, oldPassword, oldContractSalt)
     const oldIEK = await deriveKeyFromPassword(CURVE25519XSALSA20POLY1305, oldPassword, oldContractSalt)

shared/domains/chelonia/Secret.js

@@ -5,6 +5,10 @@ import { serdesDeserializeSymbol, serdesSerializeSymbol, serdesTagSymbol } from
 /* Wrapper class for secrets, which identifies them as such and prevents them
 from being logged */
 
+// Use a `WeakMap` to store the actual secret outside of the returned `Secret`
+// object. This ensures that the only way to access the secret is via the
+// `.valueOf()` method, and it prevents accidentally logging things that
+// shouldn't be logged.
 const wm = new WeakMap()
 export class Secret<T> {
   // $FlowFixMe[unsupported-syntax]

shared/domains/chelonia/encryptedData.js

@@ -8,10 +8,20 @@ import { isRawSignedData, signedIncomingData } from './signedData.js'
 const rootStateFn = () => sbp('chelonia/rootState')
 
 export interface EncryptedData<T> {
+  // The ID of the encryption key used
   encryptionKeyId: string,
+  // The unencrypted data. For outgoing data, this is the original data given
+  // as input. For incoming data, decryption will be attempted.
   valueOf: () => T,
+  // The serialized _encrypted_ data. For outgoing data, encryption will be
+  // attempted. For incoming data, this is the original data given as input.
+  // The `additionalData` parameter is only used for outgoing data, and binds
+  // the encrypted payload to additional information.
   serialize: (additionalData: ?string) => [string, string],
+  // A string version of the serialized encrypted data (i.e., `JSON.stringify()`)
   toString: (additionalData: ?string) => string,
+  // For incoming data, this is an alias of `serialize`. Undefined for outgoing
+  // data.
   toJSON?: () => [string, string]
 }
 

shared/domains/chelonia/signedData.js

@@ -8,13 +8,28 @@ import { ChelErrorSignatureError, ChelErrorSignatureKeyNotFound, ChelErrorSignat
 const rootStateFn = () => sbp('chelonia/rootState')
 
 export interface SignedData<T> {
+  // The ID of the signing key used
   signingKeyId: string,
+  // The unsigned data. For outgoing data, this is the original data given
+  // as input. For incoming data, signature verification will be attempted.
   valueOf: () => T,
+  // The serialized _signed_ data. For outgoing data, signing will be
+  // attempted. For incoming data, this is the original data given as input.
+  // The `additionalData` parameter is only used for outgoing data, and binds
+  // the signed payload to additional information.
   serialize: (additionalData: ?string) => { _signedData: [string, string, string] },
+  // Data needed to recreate signed data.
+  // [contractID, data, height, additionalData]
   context?: [string, Object, number, string],
+  // A string version of the serialized signed data (i.e., `JSON.stringify()`)
   toString: (additionalData: ?string) => string,
+  // For outgoing data, recreate SignedData using different data and the same
+  // parameters
   recreate?: (data: T) => SignedData<T>,
+  // For incoming data, this is an alias of `serialize`. Undefined for outgoing
+  // data.
   toJSON?: () => [string, string],
+  // `get` and `set` can set additional (unsigned) fields within `SignedData`
   get: (k: string) => any,
   set?: (k: string, v: any) => void
 }

shared/zkpp.js

@@ -1,5 +1,6 @@
 import nacl from 'tweetnacl'
 import scrypt from 'scrypt-async'
+import { AUTHSALT, CONTRACTSALT, CS, SU } from './zkppConstants.js'
 
 // .toString('base64url') only works in Node.js
 export const base64ToBase64url = (s: string): string => s.replace(/\//g, '_').replace(/\+/g, '-').replace(/=*$/, '')
@@ -31,7 +32,7 @@ export const computeCAndHc = (r: string, s: string, h: string): [Uint8Array, Uin
 }
 
 export const encryptContractSalt = (c: Uint8Array, contractSalt: string): string => {
-  const encryptionKey = hashRawStringArray('CS', c).slice(0, nacl.secretbox.keyLength)
+  const encryptionKey = hashRawStringArray(CS, c).slice(0, nacl.secretbox.keyLength)
   const nonce = nacl.randomBytes(nacl.secretbox.nonceLength)
 
   const encryptedContractSalt = nacl.secretbox(Buffer.from(contractSalt), nonce, encryptionKey)
@@ -40,7 +41,7 @@ export const encryptContractSalt = (c: Uint8Array, contractSalt: string): string
 }
 
 export const decryptContractSalt = (c: Uint8Array, encryptedContractSaltBox: string): string => {
-  const encryptionKey = hashRawStringArray('CS', c).slice(0, nacl.secretbox.keyLength)
+  const encryptionKey = hashRawStringArray(CS, c).slice(0, nacl.secretbox.keyLength)
   const encryptedContractSaltBoxBuf = Buffer.from(base64urlToBase64(encryptedContractSaltBox), 'base64')
 
   const nonce = encryptedContractSaltBoxBuf.slice(0, nacl.secretbox.nonceLength)
@@ -52,7 +53,7 @@ export const decryptContractSalt = (c: Uint8Array, encryptedContractSaltBox: str
 export const encryptSaltUpdate = (secret: string, recordId: string, record: string): string => {
   // The nonce is also used to derive a single-use encryption key
   const nonce = nacl.randomBytes(nacl.secretbox.nonceLength)
-  const encryptionKey = hashRawStringArray('SU', secret, nonce, recordId).slice(0, nacl.secretbox.keyLength)
+  const encryptionKey = hashRawStringArray(SU, secret, nonce, recordId).slice(0, nacl.secretbox.keyLength)
 
   const encryptedRecord = nacl.secretbox(Buffer.from(record), nonce, encryptionKey)
 
@@ -63,7 +64,7 @@ export const decryptSaltUpdate = (secret: string, recordId: string, encryptedRec
   // The nonce is also used to derive a single-use encryption key
   const encryptedRecordBoxBuf = Buffer.from(base64urlToBase64(encryptedRecordBox), 'base64')
   const nonce = encryptedRecordBoxBuf.slice(0, nacl.secretbox.nonceLength)
-  const encryptionKey = hashRawStringArray('SU', secret, nonce, recordId).slice(0, nacl.secretbox.keyLength)
+  const encryptionKey = hashRawStringArray(SU, secret, nonce, recordId).slice(0, nacl.secretbox.keyLength)
 
   const encryptedRecord = encryptedRecordBoxBuf.slice(nacl.secretbox.nonceLength)
 
@@ -92,8 +93,8 @@ export const saltAgreement = (publicKey: string, secretKey: Uint8Array): false |
     return false
   }
 
-  const authSalt = Buffer.from(hashStringArray('AUTHSALT', dhKey)).slice(0, 18).toString('base64')
-  const contractSalt = Buffer.from(hashStringArray('CONTRACTSALT', dhKey)).slice(0, 18).toString('base64')
+  const authSalt = Buffer.from(hashStringArray(AUTHSALT, dhKey)).slice(0, 18).toString('base64')
+  const contractSalt = Buffer.from(hashStringArray(CONTRACTSALT, dhKey)).slice(0, 18).toString('base64')
 
   return [authSalt, contractSalt]
 }
@@ -136,19 +137,25 @@ export const buildRegisterSaltRequest = async (publicKey: string, secretKey: Uin
   return [contractSalt, base64ToBase64url(Buffer.concat([nonce, encryptedHashedPasswordBuf]).toString('base64'))]
 }
 
-export const buildUpdateSaltRequestEa = async (password: string, c: Uint8Array): Promise<[string, string]> => {
-  // Derive S_A and S_C as follows:
+// Build the `E_c` (encrypted arguments) to send to the server to negotiate a
+// password change. `password` corresponds to the raw user password, `c` is a
+// negotiated shared secret between the server and the client. The encrypted
+// payload contains the salted user password (using `authSalt`).
+// The return value includes the derived contract salt and the `E_c`.
+export const buildUpdateSaltRequestEc = async (password: string, c: Uint8Array): Promise<[string, string]> => {
+  // Derive S_A (authentication salt) and S_C (contract salt) as follows:
   //   -> S_T -< BASE64(SHA-512(SHA-512(T) + SHA-512(c))[0..18]) with T being
-  //     `AUTHSALT` or `CONTRACTSALT`
+  //     `AUTHSALT` (for S_A) or `CONTRACTSALT` (for S_C).
   // This way, we ensure both the server and the client contribute to the
-  //   salts' entropy.
+  //   salts' entropy. Having more sources of entropy contributes to higher
+  //   randomness in the result.
   // When sending the encrypted data, the encrypted information would be
   // `hashedPassword`, which needs to be verified server-side to verify
   // it matches p and would be used to derive S_A and S_C.
-  const authSalt = Buffer.from(hashStringArray('AUTHSALT', c)).slice(0, 18).toString('base64')
-  const contractSalt = Buffer.from(hashStringArray('CONTRACTSALT', c)).slice(0, 18).toString('base64')
+  const authSalt = Buffer.from(hashStringArray(AUTHSALT, c)).slice(0, 18).toString('base64')
+  const contractSalt = Buffer.from(hashStringArray(CONTRACTSALT, c)).slice(0, 18).toString('base64')
 
-  const encryptionKey = hashRawStringArray('SU', c).slice(0, nacl.secretbox.keyLength)
+  const encryptionKey = hashRawStringArray(SU, c).slice(0, nacl.secretbox.keyLength)
   const nonce = nacl.randomBytes(nacl.secretbox.nonceLength)
 
   const hashedPassword = await hashPassword(password, authSalt)

shared/zkppConstants.js

@@ -0,0 +1,4 @@
+export const AUTHSALT = 'AUTHSALT'
+export const CONTRACTSALT = 'CONTRACTSALT'
+export const CS = 'CS'
+export const SU = 'SU'