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

Shouldn't this be using Buffer.byteLength? #6

Closed
billinghamj opened this issue Feb 10, 2018 · 3 comments
Closed

Shouldn't this be using Buffer.byteLength? #6

billinghamj opened this issue Feb 10, 2018 · 3 comments

Comments

@billinghamj
Copy link
Contributor

(Instead of String.prototype.length)

It seems to me that this is likely to result in the truncation of the string if any multi-byte characters are present:

https://github.com/Bruce17/safe-compare/blob/master/index.js#L54-L57

@billinghamj
Copy link
Contributor Author

It looks like this issue was masked by the use of the binary encoding (which is lossy): nodejs/node#5504 (comment)

This works correctly:

function safeCompare(a, b) {
    var strA = String(a);
    var strB = String(b);
    
    var len = Math.max(Buffer.byteLength(strA), Buffer.byteLength(strB));
    
    var bufA = Buffer.alloc(len, strA, 'utf8');
    var bufB = Buffer.alloc(len, strB, 'utf8');
    
    return crypto.timingSafeEqual(bufA, bufB);
}

billinghamj added a commit to billinghamj/safe-compare that referenced this issue Feb 10, 2018
@Bruce17
Copy link
Owner

Bruce17 commented Feb 10, 2018 via email

@billinghamj
Copy link
Contributor Author

Thanks. I have also opened a PR - #7

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

No branches or pull requests

2 participants