This is a cross-site scripting vulnerability, yes, but client-side crypto does not necessitate cross-site scripting.
This implementation just so happens to not protect against it properly. There are legitimate arguments against client-side cryptography; this is not one of them.
The argument is that implementing crypto within an application that is designed to download and execute untrusted code from untrusted servers and has an extremely large attack service [1] is a difficult if not dangerous task.
Hey, cool project. As another poster in this thread mentioned, you have a cross-site scripting vulnerability because you don't properly sanitize the decrypted user input.
I might work on this if I get a little time later today, but in case I don't, here are a few resources for you:
The good news is that PHP is so widespread that there are really great tutorials and libraries for sanitizing user input and preventing cross-site scripting.
well maybe I just have the NIH-Syndrom ;)
this is a microservice. Anyone can re-build it in some days. I just wanted that fancy Medium Editor, Markdown and encryption, and didn't found it yet.
Ikkez, you use hash == HMAC-SHA256(CT, SHA256(password)) to check if the password is valid. You should replace the "SHA256(password)" part here with a strong password-based key derivation function (PBKDF2, scrypt, bcrypt) with a decent number of iterations.
SHA256 is blazing fast. While this is a good thing for a hash function in general, it is not for a key derivation. PBKDF2, scrypt, bcrypt and others are slow by design to make an attack much harder.
I think it does not matter that much here, as this hash is just used for checking the message integrity. This hash is build from the encrypted doc + the hashed password. if you are about to brute force the HMAC function, you would just get a SHA hash back. It would make more sense to try to break the message directly. I got this trick from http://stackoverflow.com/a/23190781/2038179
I think you use this to validate the password, not to check the integrity of the message. And an attacker gets a password as a result of brute-forcing, not a hash. Look at this snippet in pseudocode:
// Get the hash and the encrypted text from the encrypted data
hash = substing(encryptedData, 0, 64);
encryptedText = substing(encryptedData, 64);
foreach password in PasswordDictionary {
if hash == HMAC(encryptedText, SHA256(password)) {
print "Password found: " + password;
// Decrypt the message
print "Message: " + AESDecrypt(encryptedText, password);
}
}
The loop will run fast, and you get a clear-text password as a result, which can decrypt the message. If you replace SHA256(password) with PBKDF2(password, 20000) the loop becomes insanely slower.
that assumes an async pbkdf2 implementation for the browser... most browser implementations for crypto I've seen are synchronous, though I'm uncertain today, I haven't checked for a couple years now. In the case of synchronous, most browser based variants will kill the script before it completes... you could use workers, but that won't work for ie<=9 [1]. Which may or may not be an option here. As it stands, it would be a nice options.
I really like the markdown editor implementation, will look at how they are using medium-editor... I'm working on a similar implementation and was considering going side-by-side, but this actually looks/works better. I did write a sanitization utility[2] for such inputs, but hadn't yet completed the editor.
The WebCrypto API is meant to solve this but many aspects of it are not quite ready for production yet.
I'd personally love to see JS crypto libraries like CryptoJS, SJCL and Forge start implementing polyfills for WebCrypto. Even just using the getRandomValues method (which seems to be fairly stable across all modern browsers) for their PRNG implementations would result in a much more theoretically sound crypto library.
I'd like to see more async implementations of crypto for JS, breaking work up with setTimeout/setImmediate so that they don't kill the browser... Lately, I'm more inclinded to first reach for the browserify polyfills, and use the node crypto api.
You should have a prominent "clear" action button, to wipe the pre-loaded example content with a click. I tried clicking on "Create New" in the upper right hoping that would generate that effect, but it just resets it all back. I think a 'clear content' button would be especially nice for mobile.
and this is why we don't like crypto in the browser...