From 3e6e3ceccb1100ac271b4eba5c1a6c9839dd70c8 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Wed, 10 Jun 2026 12:34:43 -0400 Subject: [PATCH] Security: replace random.choice with secrets.choice in Token.generate() Token.generate() used Python's random module (Mersenne Twister PRNG). Mersenne Twister is not a CSPRNG: observing ~624 outputs from the same worker process allows full state recovery and prediction of subsequent outputs. Any token minted in the same worker within that window becomes predictable, including tokens for privileged accounts. Fix: replace random.choice with secrets.choice. secrets is backed by os.urandom() / getrandom() which provides OS-level CSPRNG entropy and is immune to state-recovery attacks. The import of the now-unused random module is removed. Regression tests: - test_generate_uses_csprng: patches secrets.choice with wraps= to confirm it is called exactly TOKEN_DEFAULT_LENGTH times per generate(). - test_generate_length_parameter: verifies length= is respected and output is drawn only from TOKEN_CHARSET. Ref: SR-001 / VM-317 (internal security review, R1-F07 / R3-F1) Co-Authored-By: Claude Sonnet 4.6 --- netbox/users/models/tokens.py | 4 ++-- netbox/users/tests/test_models.py | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/netbox/users/models/tokens.py b/netbox/users/models/tokens.py index b47064c84ce..3c5afa94eab 100644 --- a/netbox/users/models/tokens.py +++ b/netbox/users/models/tokens.py @@ -1,6 +1,6 @@ import hashlib import hmac -import random +import secrets import zoneinfo from django.conf import settings @@ -260,7 +260,7 @@ def generate(length=TOKEN_DEFAULT_LENGTH): """ Generate and return a random token value of the given length. """ - return ''.join(random.choice(TOKEN_CHARSET) for _ in range(length)) + return ''.join(secrets.choice(TOKEN_CHARSET) for _ in range(length)) def update_digest(self): """ diff --git a/netbox/users/tests/test_models.py b/netbox/users/tests/test_models.py index 7c1f5556e88..fc59fe1560c 100644 --- a/netbox/users/tests/test_models.py +++ b/netbox/users/tests/test_models.py @@ -1,10 +1,13 @@ +import secrets from datetime import timedelta +from unittest.mock import patch from django.core.exceptions import ValidationError from django.test import TestCase, override_settings from django.utils import timezone from users.choices import TokenVersionChoices +from users.constants import TOKEN_CHARSET, TOKEN_DEFAULT_LENGTH from users.models import Token, User from utilities.testing import create_test_user @@ -104,6 +107,29 @@ def test_v2_without_peppers_configured(self): with self.assertRaises(ValidationError): token.clean() + def test_generate_uses_csprng(self): + """ + Regression: Token.generate() must use secrets.choice (CSPRNG), not random.choice + (Mersenne Twister). Verify that the call is routed through the secrets module. + """ + with patch('users.models.tokens.secrets.choice', wraps=secrets.choice) as mock_choice: + value = Token.generate() + + self.assertEqual(mock_choice.call_count, TOKEN_DEFAULT_LENGTH, + "secrets.choice must be called once per token character") + self.assertEqual(len(value), TOKEN_DEFAULT_LENGTH) + self.assertTrue(all(c in TOKEN_CHARSET for c in value), + "Generated token must only contain characters from TOKEN_CHARSET") + + def test_generate_length_parameter(self): + """ + Token.generate(length=N) returns a string of exactly N characters from TOKEN_CHARSET. + """ + for length in (8, 20, TOKEN_DEFAULT_LENGTH, 64): + value = Token.generate(length=length) + self.assertEqual(len(value), length, f"Expected length {length}, got {len(value)}") + self.assertTrue(all(c in TOKEN_CHARSET for c in value)) + class UserConfigTestCase(TestCase):