search

fruiz500 / PassLok-Privacy

PassLok privacy app
GNU General Public License v3.0
30 stars 8 forks source link

Unjustified tweaking of third-party crypto libraries #7

Closed taltman closed 9 years ago

taltman commented 9 years ago

Imported crypto-related JS libraries, such as SJCL, shouldn't be modified without an explanation. This means that now that entire library has to be reviewed by a third party audit your changes, to make sure that PassLok's modifications haven't compromised any security. If someone had to pick between you and Dan Boneh in terms of trusting their cryptographic skills, they'll trust Dan. :-) If you have no choice but to change it, you should have an iron-clad justification.

If space is a concern, note that SJCL's build code allows you to select which components to include, and will also do JS minification/compression.

fruiz500 commented 9 years ago

The modifications are minimal and clearly explained in the code comments. They never affect the cryptography, but rather are included to provide richer feedback to the user when an error occurs. I count three instances within SJCL, all of which are clearly marked:

  1. A few lines at the place where message authentication fails in SJCL. They display different messages on PassLok's main page or display a dialog, plus information on how to correct the error.
  2. One line at the place where point validation fails in ecc.js. It is used to display a message telling the user what happened.
  3. The NIST p521 curve parameters are added to the other pre-defined curves. These are the same parameters proposed to be included in the official ecc.js. For some reason that escapes me they are still not included in ecc.js. PassLok needs them, so I added them there rather than elsewhere because it is neater and that's where they will go eventually.

Concerning audits, too. How would anyone know that there are no other changes, introduced surreptitiously? If I took the "official" SJCL files, whole or minimized, and added them by reference, could anyone be sure that no malicious changes existed? So people are going to have to read the code no matter what. This is why all libraries used are included within PassLok in their richest form.

fruiz500 commented 9 years ago

Version 2.1.03 corrects this issue, so that there are no longer any additions to the SJCL and RS libraries. This is done by using try-catch statements in the PassLok code so that specific warnings can be displayed when decryption or error correction fails. The parameters for the NIST p521 curve are defined in the main PassLok code.

A significant portion of the SJCL ecc.js library, located at the end, is cut out because it does not get used in PassLok: sample curve parameters, ElGamal, and ECDSA. I hope this won't bother those auditing the code. Likewise, significant edits remain in the secrets.js library (which implements SSSS functions) to replace its own less-secure PRNG with the SJCL PRNG.

fruiz500 commented 9 years ago

Since my pull request concerning the addition of the NIST p521 curve parameters to the SJCL master has been merged, this is no longer an issue. Version 2.1.03 of PassLok will contain the SJCL libraries without modification. We could save about 10 kB by deleting the unneeded parts of ecc.js, but probably it is better to let them be for auditing's sake.

taltman commented 9 years ago

Thanks for your reply. My suggestions:

  1. I see your mention of the curve addition in the technical documentation. This is great.
  2. The technical documentation should add a section describing the code organization, and any modifications to third-party code, especially when it concerns cryptographic or security-related functions. In other words, an auditer should be able to find in the documentation up-front descriptions of any tweaks to vetted third-party crypto, instead of having to hunt for it in the source-code.
  3. Have these changes been announced and scrutinized by SJCL folks? At the very least, it would be nice to see that the code is being offered back to the SJCL folks to integrate into their repo. Getting that done would allay concerns with tweaking a trusted crypto library.

These are two separate concerns: code provenance verification / secure distribution, and auditing. With auditing, I'm referring to technical experts going over the architecture and design of PassLok, and identifying cryptographic or security weaknesses or vulnerabilities. Really good technical documentation and well-organized code helps with transparency and critique by third-parties. I'm not referring to how can layperson users verify that their code is indeed from PassLok and hasn't been tampered (that's a whole other topic that I'll bring up in a separate issue).

fruiz500 commented 9 years ago

Version 2.1.03 is made with this issue very much in mind. Unnecessary modifications have been eliminated so that the included libraries are in their original form. Suggestions for changes have been submitted to the developers and have been accepted at least in one case.

Two third-party libraries remain in modified form:

  1. secrets.js contains its own RNG functions. These are commented out and replaced with a call to the stronger SJCL RNG functions.
  2. markovTextStego.js contains a strong RegEx that makes it impossible to use non-English cover texts. The modified RegEx, which is clearly marked, has been submitted to the developer via a pull request.