search

DimensionDev / Maskbook

The portal to the new, open Internet. ([I:b])
https://mask.io
GNU Affero General Public License v3.0
1.51k stars 310 forks source link

AES-CBC compatibility #8

Closed amark closed 5 years ago

amark commented 5 years ago

First off, incredible work!

Second off, don't let anything I'm saying delay launch, timing is way more important.

Third, AES-GCM is more secure than AES-CBC (tho obviously, there infinitely forever will be "X is better than Y" problems, so I'm not going to be naive that CBC has a tradeoff perhaps you intended?).

Fourth, I forget exactly what it was, but I think I found compatibility issues between NodeJS WebCrypto implementations and browser. Altho this may be because at the time I wasn't fully using node-webcrypto-ossl (the other projects failed to be fully implemented enough, or had tiny padding problems). I think it was that CBC with the other libraries (not OSSL) were what caused the compatibility issues. So just keep this in mind, GCM wound up being easier to find cross-environment working solution.

(and, well, we use GCM, so that is a selfish reason GCM could help. Please be honest, was there issues with SEA wrapper?)

Jack-Works commented 5 years ago

Not having used SEA yet. In the early stage of Maskbook, SEA is not documented yet. And to keep backward compatibility of the encrypted posts, we will not switch to SEA at least in this version (version 🎼1/4).

About the Crypto implementation, I didn't think too much, just find a lib that supports secp256k1 and have a very close API to the native WebCrypto API.

About AES-GCM or AES-CBC, @yisiliu

amark commented 5 years ago

yeah, totally makes sense. Good call.

We did go ahead and update them, at least for future reference, including a helpful "quick start" that covers ECDSA, ECDH, PBKDF2, AES, etc. in a few lines (tho ;) I'm sure you know how many WebCrypto lines are required underneath, haha, uggh!) https://gun.eco/docs/SEA

yisiliu commented 5 years ago

@amark Yes! Transferring into GCM is actually already part of the plan now:) I didn't do much research into this initially and chose the most default one, CBC mode, however no longer "default" anymore. Thanks for the suggestions!

yisiliu commented 5 years ago

We are now using secp256k1 for ECC - How do you think about it? I chose this over curve 25519 because I don't think it's "less secure" than it and more importantly, it is widely adopted by most major blockchain systems. This is just my intentions and willing to hear more.

amark commented 5 years ago

This is a hard tradeoff between the triangle of "works now -- compatibility -- security"

secp256k1 is " " " not safe " " " https://safecurves.cr.yp.to/ (ctrl+F on that page for it)

Specifically, secp256r1 (what WebCrypto & SEA use) is considered "more safe" than secp256k1, but not by much ( https://crypto.stackexchange.com/questions/18965/is-secp256r1-more-secure-than-secp256k1 , and reference P-256 ECDH is is r1 according to https://8gwifi.org/docs/window-crypto-ecdh.jsp ).

However, abandoning both means reduced web-native targets, this is important because:

So on that alone, not using k1 in favor of r1 has huge perks.

Obviously, compatibility with blockchain world is good enough reason to ignore that...

BUT NO!!!!

NEVER LET A FINANCIAL KEY BE USED FOR NON-FINANCIAL USES

Have the financial key sign off on the non-financial key. But never never never be tempted to use any Bitcoin or blockchain key because then a user might be tempted to import their Bitcoin key!!! Now you're responsible for their ENTIRE financial security. Sure, if you want to add banking to maskbook, great, do it moving forward, but don't import 10 years of financial history/baggage (the only perk you get from this is being able to have crackers tempted to hack your code to backdoor user keys - you want to eliminate that incentive).

yisiliu commented 5 years ago

@amark That's a really thoughtful comment! secp256k1 is not safe but neither does r1. The only "safe" curve is curve25519 according to DJB. However, I did some research and the reason why I gave secp256k1 a pass is: 1. k1 is better than r1 in my opinion since at least k1 has been discussed and explained well enough and in the opposite r1 was first "invented" by NIST (or NSA yesyesyes) so we cannot trust it (wtf? but yeah who knows it doesn't contain any backdoors http://linuxadvocates.blogspot.com/2013/09/is-openssls-cryptography-broken.html) 2. Still, it is widely adopted in the blockchain world. However, you made a really important statement regarding the backfire of doing so that users would (possibly) upload their Bitcoin key pair directly into Maskbook. This requires top security level to be deployed in Maskbook in case of financial losses. I would seriously doubt if webcrypto can provide the expected level of security. I am considering moving to Curve 25519 but we probably need to research more on this to fully convince, at least myself, this is safer than p256 curves.

amark commented 5 years ago

Yeah, using a currently-"safe" curve is better, altho obviously then lose all compatibility lol.

I still have to make this annoying disclaimer though, like before, is any non-native (if you use 25519 on the web, it won't be) will still be "less safe" than any native implementation. Not because it is implemented wrong or cause native can't have backdoors, but simply because JS (even in extensio) is fundamentally unsafe (and this is coming from JS diehard who built a DB in JS despite 5 years of internet bullies hating and abusing me for it, I will still fight for JS) environment. Browser and OS are too, lol, but we're talking about "less wrong" :/. Choose what you guys think best though.

yisiliu commented 5 years ago

@amark lol I agree with you on the JS part. I personally am not a JS fan, even our products are all using JS :1234: I used to work mainly on C++ and Python and wish we could have used these "better and safer" implementations on web apps. Which curve are you using in party? I bet SEA could be a standard crypto library for future use but I would ask how it differs from other libs like webcrypto.

Jack-Works commented 5 years ago

@yisiliu If you're about to change the crypto algorithm, you should do it ASAP before our first official release if you don't want keep version -41 in our code base. (We release with 2 version of support should be irrational.)

Jack-Works commented 5 years ago

And by the way, the non-native crypto library also adds complexity in our code. If you're using the native WebCrypto API, you can store the CryptoKey directly in the indexed db. Now we need to do a transform first

neruthes commented 5 years ago

Some also believe that implementing cryptography with JavaScript in browsers is rather more vulnerable than relying on native cryptography APIs. Would anyone like to discuss on this?

Jack-Works commented 5 years ago

I prefer the browser built-in one.

amark commented 5 years ago

Yeah, SEA uses WebCrypto and makes it easy to use.

But more importantly, dApps that use it (notabug.io d.tube etc.) will automatically use a SEA compatible browser extension as a secure enclave.

This means not only can the keys be in IndexedDB, but even the seed or password is never known by the dApp. This only ever stays in the chrome extension.

This same design can be applied to hardware wallets. If the extension supports hardware wallet (like upcoming MetaMask) then a web app using SEA will pass all cryptography operations to the extension which will pass it to the hardware wallet, then back to extension and back to web app.

So SEA adds real huge value to WebCrypto other than making it easy, compatible with NodeJS (non trivial), and creating automatic secure enclaves, plus a bunch of big projects already adopting it! :)

amark commented 5 years ago

As in, does this make sense?

Rather than using WebCrypto in the web app...

The web app using SEA will use the WebCrypto in the browser extension.

Thus making an even more secure environment.

Tedko commented 5 years ago

🤔