fix: catch error when unmarshaling instead of crashing (#113)

* test: Add failing tests

* fix: Fix some failing tests

* fix: various fixes on garbage error handling and respective tests

* tests: increased timeout for test timing out in CI

* tests: increasing test timeout to please the CI gods

* tests: increasing test timeout to please the CI gods

* fix: for when unMarshallPrivateKey is called with null or undefined key

src/keys/ed25519.js

@@ -7,30 +7,28 @@ exports.publicKeyLength = nacl.sign.publicKeyLength
 exports.privateKeyLength = nacl.sign.secretKeyLength
 
 exports.generateKey = function (callback) {
-  const done = (err, res) => setImmediate(() => {
-    callback(err, res)
+  setImmediate(() => {
+    let result
+    try {
+      result = nacl.sign.keyPair()
+    } catch (err) {
+      return callback(err)
+    }
+    callback(null, result)
   })
-
-  let keys
-  try {
-    keys = nacl.sign.keyPair()
-  } catch (err) {
-    return done(err)
-  }
-  done(null, keys)
 }
 
 // seed should be a 32 byte uint8array
 exports.generateKeyFromSeed = function (seed, callback) {
-  const done = (err, res) => setImmediate(() => callback(err, res))
-
-  let keys
-  try {
-    keys = nacl.sign.keyPair.fromSeed(seed)
-  } catch (err) {
-    return done(err)
-  }
-  done(null, keys)
+  setImmediate(() => {
+    let result
+    try {
+      result = nacl.sign.keyPair.fromSeed(seed)
+    } catch (err) {
+      return callback(err)
+    }
+    callback(null, result)
+  })
 }
 
 exports.hashAndSign = function (key, msg, callback) {
@@ -41,6 +39,13 @@ exports.hashAndSign = function (key, msg, callback) {
 
 exports.hashAndVerify = function (key, sig, msg, callback) {
   setImmediate(() => {
-    callback(null, nacl.sign.detached.verify(msg, sig, key))
+    let result
+    try {
+      result = nacl.sign.detached.verify(msg, sig, key)
+    } catch (err) {
+      return callback(err)
+    }
+
+    callback(null, result)
   })
 }

src/keys/index.js

@@ -81,7 +81,13 @@ exports.marshalPublicKey = (key, type) => {
 // Converts a protobuf serialized private key into its
 // representative object
 exports.unmarshalPrivateKey = (buf, callback) => {
-  const decoded = keysPBM.PrivateKey.decode(buf)
+  let decoded
+  try {
+    decoded = keysPBM.PrivateKey.decode(buf)
+  } catch (err) {
+    return callback(err)
+  }
+
   const data = decoded.Data
 
   switch (decoded.Type) {

src/keys/rsa.js

@@ -9,30 +9,36 @@ const jwkToPem = require('pem-jwk').jwk2pem
 exports.utils = require('./rsa-utils')
 
 exports.generateKey = function (bits, callback) {
-  const done = (err, res) => setImmediate(() => callback(err, res))
+  setImmediate(() => {
+    let result
+    try {
+      const key = keypair({ bits: bits })
+      result = {
+        privateKey: pemToJwk(key.private),
+        publicKey: pemToJwk(key.public)
+      }
+    } catch (err) {
+      return callback(err)
+    }
 
-  let key
-  try {
-    key = keypair({ bits: bits })
-  } catch (err) {
-    return done(err)
-  }
-
-  done(null, {
-    privateKey: pemToJwk(key.private),
-    publicKey: pemToJwk(key.public)
+    callback(null, result)
   })
 }
 
 // Takes a jwk key
 exports.unmarshalPrivateKey = function (key, callback) {
-  callback(null, {
-    privateKey: key,
-    publicKey: {
-      kty: key.kty,
-      n: key.n,
-      e: key.e
+  setImmediate(() => {
+    if (!key) {
+      return callback(new Error('Key is invalid'))
     }
+    callback(null, {
+      privateKey: key,
+      publicKey: {
+        kty: key.kty,
+        n: key.n,
+        e: key.e
+      }
+    })
   })
 }
 
@@ -41,16 +47,33 @@ exports.getRandomValues = function (arr) {
 }
 
 exports.hashAndSign = function (key, msg, callback) {
-  const sign = crypto.createSign('RSA-SHA256')
+  setImmediate(() => {
+    let result
+    try {
+      const sign = crypto.createSign('RSA-SHA256')
+      sign.update(msg)
+      const pem = jwkToPem(key)
+      result = sign.sign(pem)
+    } catch (err) {
+      return callback(new Error('Key or message is invalid!: ' + err.message))
+    }
 
-  sign.update(msg)
-  setImmediate(() => callback(null, sign.sign(jwkToPem(key))))
+    callback(null, result)
+  })
 }
 
 exports.hashAndVerify = function (key, sig, msg, callback) {
-  const verify = crypto.createVerify('RSA-SHA256')
+  setImmediate(() => {
+    let result
+    try {
+      const verify = crypto.createVerify('RSA-SHA256')
+      verify.update(msg)
+      const pem = jwkToPem(key)
+      result = verify.verify(pem, sig)
+    } catch (err) {
+      return callback(new Error('Key or message is invalid!:' + err.message))
+    }
 
-  verify.update(msg)
-
-  setImmediate(() => callback(null, verify.verify(jwkToPem(key), sig)))
+    callback(null, result)
+  })
 }

test/crypto.spec.js

@@ -9,7 +9,8 @@ chai.use(dirtyChai)
 const crypto = require('../src')
 const fixtures = require('./fixtures/go-key-rsa')
 
-describe('libp2p-crypto', () => {
+describe('libp2p-crypto', function () {
+  this.timeout(20 * 1000)
   let key
   before((done) => {
     crypto.keys.generateKeyPair('RSA', 2048, (err, _key) => {

test/helpers/test-garbage-error-handling.js

@@ -0,0 +1,46 @@
+/* eslint-env mocha */
+'use strict'
+
+const chai = require('chai')
+const dirtyChai = require('dirty-chai')
+const expect = chai.expect
+chai.use(dirtyChai)
+
+const util = require('util')
+const garbage = [Buffer.from('00010203040506070809', 'hex'), {}, null, false, undefined, true, 1, 0, Buffer.from(''), 'aGVsbG93b3JsZA==', 'helloworld', '']
+
+function doTests (fncName, fnc, num, skipBuffersAndStrings) {
+  if (!num) {
+    num = 1
+  }
+
+  garbage.forEach((garbage) => {
+    if (skipBuffersAndStrings && (Buffer.isBuffer(garbage) || (typeof garbage) === 'string')) {
+      // skip this garbage because it's a buffer or a string and we were told do do that
+      return
+    }
+    let args = []
+    for (let i = 0; i < num; i++) {
+      args.push(garbage)
+    }
+    it(fncName + '(' + args.map(garbage => util.inspect(garbage)).join(', ') + ')', cb => {
+      args.push((err, res) => {
+        expect(err).to.exist()
+        expect(res).to.not.exist()
+        cb()
+      })
+
+      fnc.apply(null, args)
+    })
+  })
+}
+
+module.exports = (obj, fncs, num) => {
+  describe('returns error via cb instead of crashing', () => {
+    fncs.forEach(fnc => {
+      doTests(fnc, obj[fnc], num)
+    })
+  })
+}
+
+module.exports.doTests = doTests

test/keys/ed25519.spec.js

@@ -10,7 +10,10 @@ const crypto = require('../../src')
 const ed25519 = crypto.keys.supportedKeys.ed25519
 const fixtures = require('../fixtures/go-key-ed25519')
 
-describe('ed25519', () => {
+const testGarbage = require('../helpers/test-garbage-error-handling')
+
+describe('ed25519', function () {
+  this.timeout(20 * 1000)
   let key
   before((done) => {
     crypto.keys.generateKeyPair('Ed25519', 512, (err, _key) => {
@@ -176,6 +179,12 @@ describe('ed25519', () => {
     })
   })
 
+  describe('returns error via cb instead of crashing', () => {
+    const key = crypto.keys.unmarshalPublicKey(fixtures.verify.publicKey)
+    testGarbage.doTests('key.verify', key.verify.bind(key), 2)
+    testGarbage.doTests('crypto.keys.unmarshalPrivateKey', crypto.keys.unmarshalPrivateKey.bind(crypto.keys))
+  })
+
   describe('go interop', () => {
     let privateKey
 

test/keys/rsa.spec.js

@@ -10,7 +10,10 @@ const crypto = require('../../src')
 const rsa = crypto.keys.supportedKeys.rsa
 const fixtures = require('../fixtures/go-key-rsa')
 
-describe('RSA', () => {
+const testGarbage = require('../helpers/test-garbage-error-handling')
+
+describe('RSA', function () {
+  this.timeout(20 * 1000)
   let key
 
   before((done) => {
@@ -131,6 +134,12 @@ describe('RSA', () => {
     })
   })
 
+  describe('returns error via cb instead of crashing', () => {
+    const key = crypto.keys.unmarshalPublicKey(fixtures.verify.publicKey)
+    testGarbage.doTests('key.verify', key.verify.bind(key), 2, true)
+    testGarbage.doTests('crypto.keys.unmarshalPrivateKey', crypto.keys.unmarshalPrivateKey.bind(crypto.keys))
+  })
+
   describe('go interop', () => {
     it('verifies with data from go', (done) => {
       const key = crypto.keys.unmarshalPublicKey(fixtures.verify.publicKey)

test/keys/secp256k1.spec.js

@@ -32,7 +32,7 @@ const mockSecp256k1Module = {
 }
 
 describe('without libp2p-crypto-secp256k1 module present', () => {
-  crypto.keys.supportedKeys['secp256k1'] = undefined
+  crypto.keys.supportedKeys.secp256k1 = undefined
 
   it('fails to generate a secp256k1 key', (done) => {
     crypto.keys.generateKeyPair('secp256k1', 256, (err, key) => {
@@ -61,7 +61,7 @@ describe('with libp2p-crypto-secp256k1 module present', () => {
   let key
 
   before((done) => {
-    crypto.keys.supportedKeys['secp256k1'] = mockSecp256k1Module
+    crypto.keys.supportedKeys.secp256k1 = mockSecp256k1Module
     crypto.keys.generateKeyPair('secp256k1', 256, (err, _key) => {
       if (err) return done(err)
       key = _key
@@ -70,7 +70,7 @@ describe('with libp2p-crypto-secp256k1 module present', () => {
   })
 
   after((done) => {
-    delete crypto.keys['secp256k1']
+    delete crypto.keys.secp256k1
     done()
   })