Hacker News new | ask | show | jobs
by tptacek 3938 days ago
Is this what I think it is? An ECB-mode RSA implementation?
4 comments

    function encrypt(public_key, file) {
     var pem_pub_key = sshKeyToPEM(public_key); // convert rsa to pem
    
     var chunks = [];
     var buffer = new Buffer(fs.readFileSync(file, 'utf8'));
    
     // work around for 214 character limit for encrypting
     // text with small openssh rsa pub key
     for (var i = 0; i <= (buffer.length / 214); i++) {
      chunks.push(buffer.slice(i * 214, (i * 214) + 214));
     }
    
     chunks.forEach(function(chunk) {
       var encrypted = crypto.publicEncrypt(pem_pub_key, new Buffer(chunk));
       console.log(encrypted.toString('base64'));
     });
    }
According to the docs, crypto.publicEncrypt uses OAEP by default, so the bulk of the terribleness should mainly be how horribly slow this is. It does clearly indicate that the author has no idea what they're doing, though.

Edit: For some reason I thought OAEP included randomness. It does not, which should mean you can guess-and-check the plaintext.

OAEP doesn't allow you to encrypt variable-length data. They may very well be using OAEP, but that's not my point.
It's awful crypto and it made me throw up in my mouth a little bit. They should be using RSA to encrypt a random key then encrypting the rest of the message with some sort of authenticated encryption.
Sorry I made you barf ryan-c. If you have any interest in making this better I'd be willing to convert your constructive criticism into code. Issues are open on GH :)
Can you take this as a learning experience?

1. Tools to encrypt messages using Github SSH keys are probably not a good idea. They're no more usable than real message encryption solutions, but have far more constraints.

2. You cannot safely use RSA like a normal cipher. RSA is a tool for building crypto protocols. The way you've deployed it here has a serious vulnerability.

If you want to build things that use cryptography, I think you really need to work with a high-level library. Nacl (or libsodium) is a great example of a package that goes out of its way to bulletproof itself.

Agree on higher level libraries being necessary - but Nacl or libsodium? Those are some enormous dependencies you're talking about here.

I'd say we need more effort on small, focus built libraries for tasks such as these. You can talk about how people need to add in Nacl into their project, but actually doing that is simply not possible for many developers.

eg, more libraries like this one: https://github.com/tozny/java-aes-crypto

Absolutely a learning experience, this is why I posted it to HN. Really appreciate your feedback and everyone else's - it has been tremendous. Also thanks for the note on Nacl and libsodium, I will look into those.
It doesn't appear to be a new implementation. It looks like it uses Node's crypto lib:

https://nodejs.org/api/crypto.html#crypto_crypto_publicencry...

Not sure why it says DSA is supported, the crypto library only supports RSA.

It uses this library that stitches together a PEM from an ssh public key:

https://github.com/dominictarr/ssh-key-to-pem/blob/master/in...

The point is that it's using ECB mode with RSA, which indicates the developer has no real knowledge of crypto and is just blindly using pairs of "encrypt/decrypt" functions from Node's built-in crypto lib (which is essentially a thing wrapper around OpenSSL and thus joins its illustrious legacy of encouraging developers to make catastrophic cryptographic implementation mistakes).

In encryptMessage.js, the plaintext is split into chunks, and then chunks.forEach encrypts each chunk (via crypto.publicEncrypt) independently, and concatenates the chunks to form the ciphertext, aka ECB mode. You shouldn't use ECB mode with any cipher because it is not semantically secure. This is Crypto 101.

In addition, it's a bit wacky to use RSA encryption on your entire message because RSA operations are slow and you are limited to encrypting messages that are the length of your RSA key (well, minus the padding, which is how this developer arrived at the "split plaintext into 214 byte chunks" workaround).

A better solution (in every way) would be to use a "hybrid" encryption scheme, similar to TLS or GPG. To do this, you would:

1. Generate a random key for use with a symmetric cipher (e.g. AES-256) 2. Encrypt the plaintext with the random key, using a secure block mode (e.g. CBC, CTR) 3. Encrypt the random key with the RSA public key 4. Package those things together and share it on Github

Efficient and secure. Also totally unnecessary (you basically just reinvented a subset of GPG) but that's neither here nor there.

I love this comment. Thank you for putting the time in to proposing a more thorough solution. I will take this (and other) comments into consideration, and make some much needed improvements :)
There are a whole bunch of things you're likely to get wrong trying to design your own "hybrid" encryption system. It's not easy. Why not spend some time learning how to break crypto before you start building it?
I agree. There are whole classes of problems that are not at all obvious until one has spent time learning how to break bad crypto. Matasano's challenges[0] (which I think tptacek helped create) are a good start.

0. http://cryptopals.com/

Do you have any specific resources that you can recommend for learning to break crypto?
- Matasano crypto challenges: http://cryptopals.com/

- https://www.crypto101.io/ both the presentation and the book.

I think those are good to begin with.

It appears to be using Node's crypto library to apply the RSA transform directly to the plaintext in modulus-size chunks. The problem isn't the quality of the RSA implementation.
Which means for an arbitrary length message, you can rearrange blocks and it will decrypt (and the victim is none the wiser).
This is unfortunately sort of common. Java's crypto package exposes a similar interface.
What is an ECB-mode RSA? Is it using RSA as a block cipher of blocksize order-1 ? And padding each block and concatenate them?