Skip to content

Commit

Permalink
Merge pull request from GHSA-3f99-hvg4-qjwj
Browse files Browse the repository at this point in the history 
* fix double String.fromCharCode

* use crypto module if available

Co-authored-by: Julian Gruber <julian@juliangruber.com>
  • Loading branch information
@kevinbackhouse @juliangruber
kevinbackhouse and juliangruber authored Oct 11, 2021
1 parent 87c62f2 commit 9596418
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,11 @@ util.createBuffer = function(input, encoding) {
*/

var prng = forge.prng = {};
var crypto = null;

var crypto;
try {
crypto = require('crypto');
} catch (_) {}

prng.create = function(plugin) {
var ctx = {
Expand Down Expand Up @@ -1005,7 +1009,7 @@ prng.create = function(plugin) {
// throw in more pseudo random
next = seed >>> (i << 3);
next ^= Math.floor(Math.random() * 0xFF);
b.putByte(String.fromCharCode(next & 0xFF));
b.putByte(next & 0xFF);
}
}
}
Expand Down

3 comments on commit 9596418

@ChALkeR
Copy link

@ChALkeR ChALkeR commented on 9596418 Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this a fix?

This looks like it still falls back to xorshift128+ for the purpose of ssh keys generation?

That... doesn't look like a good idea perhaps.

Why is Math.random based code even present there?

The environments where there is no way to generate the key in a secure way should fail, and not fall back to a predictable pseudo-random generator.

Note: not considering this confidential, as this is all over Twitter now afaik.

@juliangruber
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a security researcher myself so I'm preferring to relay judgement of this issue to those who are, which has happened in this advisory.

At this point I'm not going to perform any major changes to this library myself. If you want to contribute, would you consider making a docs PR with your concerns, adding a disclaimer to the README?

@normanr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for responding promptly to the security researchers and taking the time to merge this security fix. A good example of how to handle security related bug reports.

Please sign in to comment.