Skip to content

Change OTP generation to 4-digit random number #13

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.

Sign up for GitHub

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

Open
Commando-X wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Change OTP generation to 4-digit random number #13

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion badApi/otp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const router = express.Router();
*/

router.post('/otp', (req, res) => {
const otp = Math.floor(100 + Math.random() * 900); // Generate 3-digit random number
const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number

@claude claude Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:large_yellow_circle: HIGH

Issue: OTP is generated with Math.random(), a non-cryptographic PRNG.

Location: badApi/otp.js:42

Risk: Math.random() in V8 is xorshift128+, seeded once per context and not cryptographically secure. Because this endpoint hands the raw OTP back to the caller (see separate comment), an attacker can request a handful of OTPs, recover the 128-bit internal state by solving for it, and then predict every subsequent OTP the process will emit — including OTPs generated for other users. Widening the range from 3 to 4 digits does not change this: the output is deterministic once the state is known, so the effective entropy is 0 bits after state recovery, regardless of digit count.

Fix: Use a CSPRNG. crypto.randomInt() is available in Node's stdlib and is unbiased:

Suggested change
const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number
const otp = crypto.randomInt(100000, 1000000); // 6-digit CSPRNG OTP

(add const crypto = require('crypto'); alongside the other requires at the top of the file).

Reference: CWE-338 (Use of Cryptographically Weak PRNG), CWE-330 (Use of Insufficiently Random Values), OWASP ASVS V2.8.3 / V6.3.1

@claude claude Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:large_orange_circle: MEDIUM

Issue: The new range does not produce a 4-digit OTP, and the OTP space is small enough to brute-force given there is no rate limiting.

Location: badApi/otp.js:42

Risk: Math.floor(100 + Math.random() * 9000) yields values in [100, 9099], not [1000, 9999] as the comment claims — roughly 10% of issued OTPs are 3-digit values, so the code is inconsistent with its own stated intent and with any 4-digit-length validation on the verifying side. More importantly the keyspace is only 9,000 values (~13.1 bits). badApi/server.js registers this router with no rate-limit or lockout middleware anywhere in the app, so an attacker can exhaust the entire space in seconds. The endpoint's own Swagger description already acknowledges the missing rate limiting; this change increases the space only ~10x, which does not meaningfully raise the brute-force cost.

Fix: Use a 6-digit OTP from a CSPRNG (see the crypto.randomInt(100000, 1000000) suggestion above) and add per-IP + per-account rate limiting and an attempt counter that invalidates the OTP after ~5 failures.

Reference: CWE-307 (Improper Restriction of Excessive Authentication Attempts), CWE-330, OWASP API4:2023 Unrestricted Resource Consumption

return res.json({ otp });

@claude claude Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL (pre-existing on this line; this PR keeps it in place)

Issue: The generated OTP is returned directly in the HTTP response body, on an unauthenticated endpoint, and is never stored or bound to a user.

Location: badApi/otp.js:41-44 (route registered at badApi/server.js:39)

Risk: Three defects compound here:

  1. Sensitive data exposure — the secret second factor is disclosed to whoever calls POST /otp. An out-of-band factor that is returned in-band provides no assurance at all.
  2. Missing authentication/authorization — no auth middleware guards the route; any anonymous caller can mint OTPs.
  3. No server-side state — the OTP is not persisted, not tied to a user or session, and has no expiry or single-use flag, so nothing can actually verify it. grep shows no consumer of this value anywhere in the repo.

Returning the OTP is also what makes the Math.random() state-recovery attack in the comment above practical, since it gives the attacker unlimited PRNG output.

Fix: Persist the OTP server-side as a hash keyed to the authenticated user/session with a short TTL (e.g. 5 min), single-use, with an attempt counter; deliver it out-of-band (SMS/email) and return only { status: "sent" }. Require authentication (or a verified enrollment token) on the route.

Reference: CWE-200 (Exposure of Sensitive Information), CWE-306 (Missing Authentication for Critical Function), CWE-613 (Insufficient Session Expiration), OWASP API2:2023 Broken Authentication

});

Expand Down