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

Unauthenticated AES-CTR is vulnerable to malleability #11

Closed
duskwuff opened this issue Feb 5, 2019 · 3 comments · Fixed by #16
Closed

Unauthenticated AES-CTR is vulnerable to malleability #11

duskwuff opened this issue Feb 5, 2019 · 3 comments · Fixed by #16

Comments

@duskwuff
Copy link

duskwuff commented Feb 5, 2019

Messages encrypted with Cryptr are vulnerable to malleability attacks.

For example, given the encrypted string from the usage sample:

const cryptr = new Cryptr('myTotalySecretKey');
const encryptedString = cryptr.encrypt('bacon');
console.log(encryptedString); // "bcb23b81c4839d06644792878e569de4f251f08007"

If the cleartext contents of this encrypted string are known, it is possible to alter the content of the encrypted string without knowledge of the key:

var tmp = Buffer.from(encryptedString, "hex");
var b1 = Buffer.from("bacon"), b2 = Buffer.from("hello");
for (var i = 0; i < b1.length; i++) {
    tmp[i + 16] ^= b1[i] ^ b2[i];
}
var ep = tmp.toString("hex");
console.log(ep); // "bcb23b81c4839d06644792878e569de4f855ff8306"

And it decrypts to the target value:

var dp = cryptr.decrypt(ep);
console.log(dp); // "hello"

This is a really big deal from a cryptographic perspective. An attacker has just modified an encrypted message in transit, and you have no way of detecting it.

AES-CTR is not an appropriate cryptographic primitive for this use case. (Ironically, the previous choice of AES-CBC was somewhat less vulnerable to this attack.)

@MauriceButler
Copy link
Owner

Hi @duskwuff ,

I have switched to using aes-256-gcm with authentication in the use-gcm branch

This seems to mitigate this issue as far as I am aware.

I am going to do some additional testing but please let me know if this is acceptable from your point of view.

@jcohenho
Copy link

jcohenho commented Oct 9, 2019

can we merge this branch?

@MauriceButler
Copy link
Owner

I have merged and released this breaking change in v6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants