Skip to content

Replace deprecated os.popen() call with Python's built-in file handling#865

Closed
Edu92337 wants to merge 1 commit intoSeedSigner:devfrom
Edu92337:dev
Closed

Replace deprecated os.popen() call with Python's built-in file handling#865
Edu92337 wants to merge 1 commit intoSeedSigner:devfrom
Edu92337:dev

Conversation

@Edu92337
Copy link
Copy Markdown

Description

This PR refactors the CPU serial reading logic in tools_views.py to use Python's native file I/O instead of the deprecated os.popen() function.

What changed:

  • Replaced os.popen("cat /proc/cpuinfo | grep Serial") with direct file reading using open()
  • Removed dependency on external shell commands (cat, grep)
  • Removed unused import os
  • Maintained identical behavior and error handling

Why this change:

  • os.popen() has been deprecated since Python 2.6
  • Eliminates unnecessary shell spawning, improving security posture for entropy generation
  • Removes dependency on external binaries that may not exist in all environments
  • Direct file I/O is more performant (avoids spawning shell and piped processes)
  • Follows modern Python best practices for reading procfs files

Security context:
Since this code is part of the entropy generation pathway for seed creation in a Bitcoin hardware wallet, eliminating any shell interaction—even with static strings—reduces the attack surface and follows defense-in-depth principles.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I've run pytest and made sure all unit tests pass before submitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • No, I'm a fool
  • Yes
  • N/A (refactor maintains identical behavior, existing tests should cover this)

I have tested this PR on the following platforms/os:

  • Raspberry Pi OS Manual Build
  • SeedSigner OS on a Pi0/Pi0W board
  • Other: Local development environment with /proc/cpuinfo verification

Testing methodology:

  • Verified Python syntax compiles without errors
  • Code review confirms identical logic for extracting Serial field
  • Fallback behavior (hash_bytes = b'0') preserved on exceptions

Note: No existing unit tests cover this specific code path (image entropy generation). The refactor maintains identical behavior, so existing integration should work unchanged. I don't currently have access to physical Raspberry Pi hardware, but happy to work with maintainers who have hardware access to validate before merging.


Fixes #856

@alvroble
Copy link
Copy Markdown
Contributor

alvroble commented Feb 1, 2026

Thanks for the review and welcome!

A couple of notes:

  • I general it's good practice to mention the related issue Harden: Replace os.popen with Python file handling in tools_views.py #856 for additional context.
  • os.popen() is not strictly deprecated, so it may be better to avoid describing it as such in the PR description. But that definition has been also shown in the related issue.
  • Being extremely picky, the new implementation is not exactly equivalent in behavior. If /proc/cpuinfo contained multiple Serial entries, the previous approach would effectively select the last one, while this implementation selects the first one. At the moment, all /proc/cpuinfo formats only contain a single Serial field, but this could theoretically change in the future. If we'd like to maintain exactly the same behavior, the recommended alternative to os.popen() is subprocess.run().

That said, I believe the new approach follows best practices and I like it better than the previous one, given that the import os is actually deleted.

@Edu92337 Edu92337 closed this by deleting the head repository Mar 22, 2026
@S1DDHEY
Copy link
Copy Markdown

S1DDHEY commented Apr 4, 2026

Hello @alvroble I've gone through this PR and the related issue (#856) and have made a PR (#897) for it. Please let me know if anything else is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden: Replace os.popen with Python file handling in tools_views.py

3 participants