fix: SAML assertion/response IDs generated with Math.random

[jackhenry-sec-auto-remediation]

🔗 Code scanning alert: #2 — lib/utils.js:59
This PR remediates that alert. The alert closes once the fix is on the default branch and the next analysis no longer reports it.

⚠️ Automated Security Fix — Review Required

Fix authored and independently reviewed by the glasswing /patch skill
(static review only — NOT execution-verified: not compiled, run, or
re-attacked). PR opened by the non-LLM patch-apply process. Read the diff
before merging.

How to act on this PR

• Close it — If this fix doesn't make sense or isn't needed, please close with a brief note explaining why
• Merge it — If the fix looks correct and complete, go ahead and merge
• Adopt and improve — If this addresses a real vulnerability but the implementation needs work, feel free to push additional commits to this branch or open a fresh PR

Security Vulnerability Details

Field	Value
Severity	LOW
Category	insecure_randomness
CWE	—
File	lib/utils.js:59
Finding ID	f001
Reviewer verdict	ACCEPT (style 9/10)
Scanner	glasswing-runbook

Vulnerability Description

SAML assertion/response IDs generated with Math.random

Root cause: lib/utils.js:60-62 getRandomInt used Math.random(), sole entropy source for exports.uid (lib/utils.js:30), which generates the SAML Response ID and Assertion IDs on all default paths. Fix draws bytes from crypto.randomBytes with rejection sampling (bytes >= 248 discarded, 248 = 62*floor(256/62)) onto the same 62-char alphabet — unpredictable and uniform. getRandomInt had no other callers and is removed. Output format unchanged so callers in lib/saml11.js and lib/saml20.js need no changes; 32-char uid now carries ~190 bits CSPRNG entropy.

Fix Applied

Diff

diff --git a/lib/utils.js b/lib/utils.js
index a0d82e4..96d549b 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -1,3 +1,5 @@
+var crypto = require('crypto');
+
 exports.pemToCert = function(pem) {
   var cert = /-----BEGIN CERTIFICATE-----([^-]*)-----END CERTIFICATE-----/g.exec(pem.toString());
   if (cert && cert.length > 0) {
@@ -28,10 +30,16 @@ exports.reportError = function(err, callback){
 exports.uid = function(len) {
   var buf = []
     , chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'
-    , charlen = chars.length;
-
-  for (var i = 0; i < len; ++i) {
-    buf.push(chars[getRandomInt(0, charlen - 1)]);
+    , charlen = chars.length
+    , maxValid = charlen * Math.floor(256 / charlen); // reject values that would bias the modulo
+
+  while (buf.length < len) {
+    var bytes = crypto.randomBytes(len - buf.length);
+    for (var i = 0; i < bytes.length && buf.length < len; ++i) {
+      if (bytes[i] < maxValid) {
+        buf.push(chars[bytes[i] % charlen]);
+      }
+    }
   }

   return buf.join('');
@@ -45,16 +53,3 @@ exports.removeWhitespace = function(xml) {
                 .trim();
   return trimmed;
 };
-
-/**
- * Retrun a random int, used by `utils.uid()`
- *
- * @param {Number} min
- * @param {Number} max
- * @return {Number}
- * @api private
- */
-
-function getRandomInt(min, max) {
-  return Math.floor(Math.random() * (max - min + 1)) + min;
-};
diff --git a/test/utils.tests.js b/test/utils.tests.js
index e272725..81199b5 100644
--- a/test/utils.tests.js
+++ b/test/utils.tests.js
@@ -8,4 +8,19 @@ describe("saml 1.1", function() {
 			assert.ok(!cert);
 		});
 	});
+
+	describe("uid", function() {
+		it("should not rely on Math.random to generate identifiers", function() {
+			var originalRandom = Math.random;
+			Math.random = function() { return 0; };
+			try {
+				var uid = utils.uid(32);
+				assert.equal(uid.length, 32);
+				assert.ok(/^[A-Za-z0-9]{32}$/.test(uid));
+				assert.notEqual(uid, new Array(33).join('A'));
+			} finally {
+				Math.random = originalRandom;
+			}
+		});
+	});
 });

Variants checked

Grepped Math.random, getRandomInt, pseudoRandomBytes tree-wide. lib/saml11.js:59, lib/saml20.js:179, lib/saml20.js:195 all route exclusively through utils.uid — covered by the single fix. lib/saml11.js:137 already uses crypto.randomBytes. Zero Math.random occurrences remain under lib/.

Bypass considered

Fixing only one call site would leave the other assertion IDs predictable. Fixing inside utils.uid (and deleting getRandomInt) covers every default-path ID. Caller-supplied options.uid bypasses uid by design (out of scope). Naive randomBytes % 62 rejected due to modulo bias.

Test note

Added to test/utils.tests.js: pins Math.random to 0 — old implementation deterministically produces 32 x 'A', failing assert.notEqual; with fix output comes from crypto.randomBytes and test passes.

Review Checklist

• Diff was not execution-verified — build + test locally
• Code review completed by security-aware reviewer
• Verified the fix correctly addresses the vulnerability
• Unit tests pass
• Manual verification that the fix doesn't break existing functionality
• No out-of-scope changes

References

• Scanner: glasswing-runbook (/patch skill, static review)
• Finding ID: f001

---

🤖 Generated by glasswing /patch (static review) · PR opened by the non-LLM patch-apply process