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

Pure JS Crypto Implementation #122

Open
lauckhart opened this issue Apr 30, 2023 · 6 comments
Open

Pure JS Crypto Implementation #122

lauckhart opened this issue Apr 30, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@lauckhart
Copy link
Collaborator

Feedback appreciated!

Goals:

  • Move remaining matter.js tests from matter-node.js
  • Enable full functionality in non-node environments without proper crypto primitives.

Proposal:

  • Create new matter-crypto.js package parallel to matter.js. Keeps the hair out of matter.js
  • Move Crypto.ts into matter-crypto.ts or (b) make matter-crypto.ts a test-only dependency of matter.ts (which would be a bit circular so could get weird)
  • For completeness, move existing JS crypto dependencies of matter.js (BN and elliptic) to this package as well
  • Implement one of below options for pure-JS crypto. After writing up I lean toward option 2

Option 1: Implement Crypto interface using SJCL

Pros

  • SJCL appears mature
  • AFAICT the only pure-JS library that supports all primitives matter.js requires
  • Could ingest only the subset we need with a small custom build

Cons

  • NPM package includes a subset of functionality that doesn't meet our needs, so custom build would be required
  • Custom build is simple but requires make, java and perl. Not a reasonable dependency for our build, so SCJL.js should probably go in matter.js git as part of matter-crypto.js (along with scripts to recreate)
  • Would require writing new Crypto derivative. Doesn't look terrible but this would be the slow bit
  • Would require ongoing maintenance of two Crypto implementations

Option 2: Shim node's OpenSSL style wrappers

Pros

  • Can use existing implementation: crypto-browserify
  • Fast to implement
  • crypto-browserify is highly modular so we could import only the subset we need
  • Would only need to maintain single implementation
  • Shim can automatically bypass if native crypto is available
  • This implementation could support native crypto in non-Node environments. For example, if I were to create a React Native app I would probably use react-native-fast-crypto which... Emulates node's API by wrapping OpenSSL

Cons

  • Weird dependency for crypto-matter.js on "browserify" component or including subset of crypto-browserify in crypto-matter.js (attributed of course)
  • crypto-browserify doesn't support CCM mode. There's an old PR adding it but we'd need to fork or include patched files in matter.js repo

Option 3: Implement crypto using mix of 3rd party implementations

Pros

  • Could adopt best-in-class implementation of each required feature
  • Keeps dependencies relatively clean

Cons

  • Not clearly superior to previous options as we'd still need to import some of either SJCL or browserify-aes to get CCM mode
  • Some implementations not available in NPM so would require build steps and/or checking into matter.js git
  • Probably slowest in terms of research and implementation

Option 4: New implementation of crypto primitives in matter-crypto.ts

Ha ha, j/k

@turon
Copy link
Contributor

turon commented May 1, 2023

What is wrong with the current system that uses standard js crypto packages?

import * as crypto from "crypto"; and import { ec } from "elliptic"; seems just fine to me.

@mfucci
Copy link
Collaborator

mfucci commented May 2, 2023

@turon : import * as crypto from "crypto" only works in node.js

If I understand well lauckhart proposal:

  1. keep matter.js vanilla => crypto.ts is an interface with no implementations (even of no implementation of Spake2p since this is just one crypto algo that happened to not be supported by node.js)
  2. create a new matter-crypto.js that offers a pure js implementation of the crypto, but adds a lot of dependencies
  3. let any environment-specific matter flavors, like matter-node.js, overrides the crypto with more optimized native implementation if they exists

An alternative would be to have the pure js implementation part of matter.js directly. This might add a bunch of extra dependencies that won't be needed for everyone.

Probably splitting it into its own package is the best approach.

@lauckhart :
SJCL and crypto-browserify have both there last updates 5 years ago so I would be worried about using them.
Are there not more recent implementations?

@lauckhart
Copy link
Collaborator Author

@turon -- @mfucci is correct, there's a brief discussion in Discord. I think your suggestion is actually pretty close to my preferred path -- shim for node's "crypto" module when necessary and otherwise leave our implementation as is for now.

@mfucci -- I couldn't find anything more recent, no. Forge is somewhat less stale but doesn't support AES-CCM. I think Node adding their OpenSSL wrapper and development of WebCrypto may have killed interest in these slow JS libraries. For option 3 we could use e.g. WebCrypto for key generation (I haven't confirmed browser support is mature enough) and elliptic.js for EC signatures but unfortunately at least AES-CCM would still be a gap.

Right now the immediate use case is getting Karma testing and for that I figure the easy path of shimming is OK. We could swap out as other use cases become clearer (to me, at least).

Frankly I'd be hesitant to trust any of these libraries for real security, especially loaded from NPM. I figure prominent warnings in documentation and a WARN-level log message are warranted anytime matter.js falls back to crypto that isn't OpenSSL or WebCrypto. elliptic.js may be OK but I find it hard to trust crypto until there's at least one prominent CERT advisory. 🤔

@Apollon77
Copy link
Collaborator

Because I was kind ofg the initiator of the idea having a "Pure JS" implementation here also my POV:

I also came more from the "it would be cool to have a pure JS implementation of crypto" in first place for testing to allow to have testing in matter.js really "complete" also in the matter.js package and not outside - andhave the same code tested in web and "node" tests for matter.js and not different.
Then the matter-node.js package (and a pot later matter-web.js package for web can have own testings for it's "webcrypto" implementation).

The side effect would be basic crypto support for limited platforms like low.js, but that was not my main goal here - but then the pure JS ones would be needed to be "good" security wise. but as said, not my main goal.

I basically like the idea of an own package for that to keep the dependencies low, but I would leave the current deps and Crypto.ts in matter.js as "abstract core definition" and the other package is just providing the CryptoJs.ts as compatible interface. And matter.js just uses this then as "dev dep" for testing only :-)

An alternative would be to directly implement a webcrypto based impleemntation that falls back to nodejs ones if existing - that would already solve the testing topic hopefully anddirectly bring the webcrypto one with it. Then we would have the main goals for matter.js fullfilled, but then special frameworks like low.js would still need own solutions (but in fact as said above that is a complete "out of scope side topic" and just beneficial.

@lauckhart
Copy link
Collaborator Author

OK I researched best-available implementations for each feature. Google thinks I'm a bot now and regularly makes me select pictures of motorcycles.

The following is a list by feature. I'd appreciate a sanity check from anyone with crypto expertise.

The list below -- in theory -- supports Node, Karma, all modern browsers and react native (via plugins and shims, par for the course). low.js is an interesting case but ECs in V8 on an esp32 will be even more interesting. 😁

I'll try for a new package based on below and leave open the question of a.) additional shimming and b.) whether the current Node implementation is necessary.

  • AES-CCM encrypt/decrypt
  • ECDH handoff - web crypto
  • Randomness - web crypto
  • SHA authentication - web crypto
  • PBKDF2 key derivation - web crypto
  • HKDF key derivation - web crypto
  • HMAC authentication - web crypto
  • PKCS8 encoding - web crypto
  • SEC1 encoding - I think this is just the raw private key? web crypto "raw" should work
  • SPKI encoding - web crypto
  • ECDH key generation - web crypto

@Apollon77
Copy link
Collaborator

browserify-aes

Maybe we could poke the PR again to see if someone can take up. SOme other browserify packages were updated not too long ago, so basically there is activity

From my first short look (yes, I'm far away from crypo expert) that looks ok.
In fact wioth a testing that compares the resut of the new with the current one/try to decode what was encoded there and vise versa ( at least for initial implementation) should secure that it is the same in the end.

Thank you very much for your efforts here!

@Apollon77 Apollon77 added the enhancement New feature or request label May 5, 2023
lauckhart added a commit to lauckhart/matter.js that referenced this issue Aug 6, 2023
Replace key handling with a JS implementation that converts keys to standard
JWK Key format.  Streamlines the Crypto.ts API and removes the various formats
and hacks we previously used to pass keys to Node's OpenSSL wrapper.

fixes project-chip#212
could be considered phase 1 of project-chip#122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Planned
Development

No branches or pull requests

4 participants