Repository: pydicom/pydicom

Issue:
generate_uid is not uniformly or cryptographically random
The function `pydicom.uid.generate_uid` is meant to generate an unpredictable UID.

Given the use of SHA-512, it appears that the intention is to generate a *cryptographically* random ID.  Or at least, the function seems intended to enable applications to generate cryptographically secure IDs if desired.

Two issues with this:

1. The current *default* behavior is to use a hash of several very poor entropy sources.  Despite appearing unpredictable, it is not at all secure.

2. The current hashing algorithm does not produce a *uniformly* random number.  Although it's likely still "secure enough" in some cases (given a suitable source of entropy), the fact that it's not uniform makes it hard to reason about how secure or collision-resistant it is, especially for long `prefix` values.

Here's an example showing that the output is non-uniform:
```
>>> import collections, pydicom
>>> collections.Counter(pydicom.uid.generate_uid()[26] for _ in range(10000))
Counter({'1': 3459, '3': 858, '4': 854, '2': 841, '5': 819, '8': 815, '9': 786, '7': 784, '6': 784})
```

Here's a sketch of something that I think would be reasonably secure:
```
def generate_uid(
    prefix: Union[str, None] = PYDICOM_ROOT_UID,
    entropy_srcs: Union[bytes, List[str], None] = None,
) -> UID:
    """
    ...
    """

    if prefix is None:
        # UUID -> as 128-bit int -> max 39 characters long
        return UID(f"2.25.{uuid.uuid4().int}")

    max_uid_len = 64
    if len(prefix) > max_uid_len - 1:
        raise ValueError("The prefix must be less than 63 chars")
    if not re.match(RE_VALID_UID_PREFIX, prefix):
        raise ValueError("The prefix is not in a valid format")

    avail_digits = max_uid_len - len(prefix)

    if entropy_srcs is None:
        entropy_srcs = os.urandom(64)

    if isinstance(entropy_srcs, list):
        # Deprecated algorithm, kept for backward compatibility.
        # The result is NOT uniformly random.
        hash_val = hashlib.sha512("".join(entropy_srcs).encode("utf-8"))
        suffix = str(int(hash_val.hexdigest(), 16))[:avail_digits]
    else:
        # Generate an integer between 0 and (10 ** avail_digits - 1).
        # If entropy_srcs is random, the result is uniformly random.
        value = max_value = 10 ** avail_digits
        bit_mask = (1 << max_value.bit_length()) - 1
        while value >= max_value:
            entropy_srcs = hashlib.sha512(entropy_srcs).digest()
            value = int.from_bytes(entropy_srcs, 'little') & bit_mask
        suffix = str(value)

    dicom_uid = prefix + suffix

    return UID(dicom_uid)
```

Disclaimers:

1. I'm not a cryptographer.

2. I know very little about DICOM, and haven't ever used pydicom myself.  A colleague asked for my opinion on this code, and whether it would be suitable to use as part of a secure de-identification process.


Discussion:
scaramallion: I'd appreciate any feedback @bemoody

Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 7446efc6b..bc0768eeb 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -2,13 +2,13 @@
 # See https://pre-commit.com/hooks.html for more hooks
 repos:
 - repo: https://github.com/crate-ci/typos
-  rev: v1.16.21
+  rev: typos-dict-v0.11.3
   hooks:
     - id: typos
       exclude: _.*_dict.py
 - repo: https://github.com/astral-sh/ruff-pre-commit
   # Ruff version.
-  rev: v0.1.3
+  rev: v0.1.4
   hooks:
   - id: ruff
     args: [src, --fix, --exit-non-zero-on-fix]
diff --git a/doc/release_notes/v3.0.0.rst b/doc/release_notes/v3.0.0.rst
index 152d8aefa..d9d4731b7 100644
--- a/doc/release_notes/v3.0.0.rst
+++ b/doc/release_notes/v3.0.0.rst
@@ -60,6 +60,10 @@ Changes
   module instead
 * :func:`~pydicom.data_manager.get_testdata_file` and :func:`~pydicom.data_manager.get_testdata_files`
   now raise ``ValueError`` if called with an absolute path or pattern
+* :func:`~pydicom.uid.generate_uid` has been changed to use a random suffix
+  generated using :func:`~secrets.randbelow` when `entropy_srcs` isn't used, and
+  the maximum allowed length of the `prefix` has been changed to 54 characters
+  (:issue:`1773`)
 
 
 Enhancements
diff --git a/src/pydicom/uid.py b/src/pydicom/uid.py
index 164c20ed7..4f11c469c 100644
--- a/src/pydicom/uid.py
+++ b/src/pydicom/uid.py
@@ -5,9 +5,10 @@ import hashlib
 import os
 import random
 import re
+import secrets
 import sys
-import uuid
 from typing import Any
+import uuid
 
 from pydicom import config
 from pydicom._uid_dict import UID_dictionary
@@ -375,6 +376,9 @@ UncompressedTransferSyntaxes = [
 """Uncompressed (native) transfer syntaxes."""
 
 
+_MAX_PREFIX_LENGTH = 54
+
+
 def generate_uid(
     prefix: str | None = PYDICOM_ROOT_UID,
     entropy_srcs: list[str] | None = None,
@@ -386,19 +390,26 @@ def generate_uid(
        When `prefix` is ``None`` a conformant UUID suffix of up to
        39 characters will be used instead of a hashed value.
 
+    .. versionchanged:: 3.0
+
+       * When `entropy_srcs` is ``None`` the suffix is now generated using
+         :func:`~secrets.randbelow`
+       * The maximum length of `prefix` is now 54 characters
+
     Parameters
     ----------
     prefix : str or None, optional
         The UID prefix to use when creating the UID. Default is the *pydicom*
-        root UID ``'1.2.826.0.1.3680043.8.498.'``. If not used then a prefix of
-        ``'2.25.'`` will be used with the integer form of a UUID generated
-        using the :func:`uuid.uuid4` algorithm.
+        root UID ``'1.2.826.0.1.3680043.8.498.'``. If `prefix` is ``None`` then
+        a prefix of ``'2.25.'`` will be used with the integer form of a UUID
+        generated using the :func:`uuid.uuid4` algorithm.
     entropy_srcs : list of str, optional
         If `prefix` is used then the `prefix` will be appended with a
         SHA512 hash of the supplied :class:`list` which means the result is
         deterministic and should make the original data unrecoverable. If
-        `entropy_srcs` isn't used then random data will be appended instead
-        (default). If `prefix` is not used then `entropy_srcs` has no effect.
+        `entropy_srcs` isn't used then a random number from
+        :func:`secrets.randbelow` will be appended to the `prefix`. If `prefix`
+        is ``None`` then `entropy_srcs` has no effect.
 
     Returns
     -------
@@ -408,7 +419,7 @@ def generate_uid(
     Raises
     ------
     ValueError
-        If `prefix` is invalid or greater than 63 characters.
+        If `prefix` is invalid or greater than 54 characters.
 
     Examples
     --------
@@ -422,31 +433,39 @@ def generate_uid(
     1.2.826.0.1.3680043.8.498.87507166259346337659265156363895084463
     >>> generate_uid(entropy_srcs=['lorem', 'ipsum'])
     1.2.826.0.1.3680043.8.498.87507166259346337659265156363895084463
+
+    References
+    ----------
+
+    * DICOM Standard, Part 5, :dcm:`Chapters 9<part05/chapter_9.html>` and
+      :dcm:`Annex B<part05/chapter_B.html>`
+    * ISO/IEC 9834-8/`ITU-T X.667 <https://www.itu.int/rec/T-REC-X.667-201210-I/en>`_
     """
     if prefix is None:
         # UUID -> as 128-bit int -> max 39 characters long
         return UID(f"2.25.{uuid.uuid4().int}")
 
-    max_uid_len = 64
-    if len(prefix) > max_uid_len - 1:
-        raise ValueError("The prefix must be less than 63 chars")
-    if not re.match(RE_VALID_UID_PREFIX, prefix):
-        raise ValueError("The prefix is not in a valid format")
+    if len(prefix) > _MAX_PREFIX_LENGTH:
+        raise ValueError(
+            f"The 'prefix' should be no more than {_MAX_PREFIX_LENGTH} characters long"
+        )
 
-    avail_digits = max_uid_len - len(prefix)
+    if not re.match(RE_VALID_UID_PREFIX, prefix):
+        raise ValueError(
+            "The 'prefix' is not valid for use with a UID, see Part 5, Section "
+            "9.1 of the DICOM Standard"
+        )
 
     if entropy_srcs is None:
-        entropy_srcs = [
-            str(uuid.uuid1()),  # 128-bit from MAC/time/randomness
-            str(os.getpid()),  # Current process ID
-            hex(random.getrandbits(64)),  # 64 bits randomness
-        ]
+        maximum = 10 ** (64 - len(prefix))
+        # randbelow is in [0, maximum)
+        # {prefix}.0, and {prefix}0 are both valid
+        return UID(f"{prefix}{secrets.randbelow(maximum)}"[:64])
+
     hash_val = hashlib.sha512("".join(entropy_srcs).encode("utf-8"))
 
     # Convert this to an int with the maximum available digits
-    dicom_uid = prefix + str(int(hash_val.hexdigest(), 16))[:avail_digits]
-
-    return UID(dicom_uid)
+    return UID(f"{prefix}{int(hash_val.hexdigest(), 16)}"[:64])
 
 
 # Only auto-generated Storage SOP Class UIDs below - do not edit manually


Test patch:
diff --git a/tests/test_data_manager.py b/tests/test_data_manager.py
index 5ba75723a..5e106512c 100644
--- a/tests/test_data_manager.py
+++ b/tests/test_data_manager.py
@@ -6,6 +6,7 @@ import os
 from os.path import basename
 from pathlib import Path
 import shutil
+from tempfile import TemporaryDirectory
 
 import pytest
 
@@ -24,9 +25,16 @@ from pydicom.data import download
 from pydicom.data.download import get_data_dir, calculate_file_hash, get_cached_filehash
 
 
-EXT_PYDICOM = False
-if "pydicom-data" in external_data_sources():
-    EXT_PYDICOM = True
+EXT_PYDICOM = "pydicom-data" in external_data_sources()
+if EXT_PYDICOM:
+    DATA_SRC = external_data_sources()["pydicom-data"].data_path
+    try:
+        with open(DATA_SRC / "test", "wb") as f:
+            pass
+        os.remove(DATA_SRC / "test")
+        IS_WRITEABLE = True
+    except Exception as exc:
+        IS_WRITEABLE = False
 
 
 @pytest.fixture
@@ -131,7 +139,9 @@ class TestGetData:
             get_testdata_files("/foo/*.dcm")
 
 
-@pytest.mark.skipif(not EXT_PYDICOM, reason="pydicom-data not installed")
+@pytest.mark.skipif(
+    not EXT_PYDICOM or not IS_WRITEABLE, reason="pydicom-data not installed or RO"
+)
 class TestExternalDataSource:
     """Tests for the external data sources."""
 
@@ -139,14 +149,15 @@ class TestExternalDataSource:
         self.dpath = external_data_sources()["pydicom-data"].data_path
 
         # Backup the 693_UNCI.dcm file
-        p = self.dpath / "693_UNCI.dcm"
-        shutil.copy(p, self.dpath / "PYTEST_BACKUP")
+        self.src = self.dpath / "693_UNCI.dcm"
+        self.tdir = TemporaryDirectory(ignore_cleanup_errors=True)
+        self.dst = Path(self.tdir.name) / "PYTEST_BACKUP"
+        shutil.copy(self.src, self.dst)
 
     def teardown_method(self):
         # Restore the backed-up file
-        p = self.dpath / "693_UNCI.dcm"
-        shutil.copy(self.dpath / "PYTEST_BACKUP", p)
-        os.remove(self.dpath / "PYTEST_BACKUP")
+        shutil.copy(self.dst, self.src)
+        self.tdir.cleanup()
 
         if "mylib" in external_data_sources():
             del external_data_sources()["mylib"]
diff --git a/tests/test_uid.py b/tests/test_uid.py
index 41322137d..2393761ff 100644
--- a/tests/test_uid.py
+++ b/tests/test_uid.py
@@ -36,7 +36,7 @@ class TestGenerateUID:
 
         # Test invalid UID prefixes
         for invalid_prefix in (
-            ("1" * 63) + ".",
+            ("1" * 54) + ".",
             "",
             ".",
             "1",
@@ -54,7 +54,7 @@ class TestGenerateUID:
             "1.",
             "1.23.",
             "1.0.23.",
-            ("1" * 62) + ".",
+            ("1" * 53) + ".",
             "1.2.3.444444.",
         ):
             uid = generate_uid(prefix=valid_prefix)