[Security] Fix shell injection and temp file leak in qrimage_io

[none34829]

Description

Problem or Issue being addressed

Two security issues in qrimage_io() in src/seedsigner/helpers/qr.py (reported in #872, related to #857):

1. Shell injection — user-controlled data (QR payloads containing seed entropy, PSBTs, xpubs) is interpolated directly into a shell command string via subprocess.call(cmd, shell=True). A crafted payload containing shell metacharacters ("; rm -rf / #, $(...), backticks) could execute arbitrary commands.

2. Temp file not deleted — /tmp/qrcode.png may contain sensitive wallet data and persists after use. While SeedSigner OS uses tmpfs, defense-in-depth requires cleanup.

Solution

Shell injection fix:

• Replaced subprocess.call(cmd, shell=True) with subprocess.run([...], input=data_bytes, capture_output=True)
• Data is passed via stdin rather than as a command-line argument, which avoids both shell metacharacter interpretation and exposure in the process list (/proc/*/cmdline)
• The argument list is constructed with explicit values — no string interpolation reaches the shell

Temp file cleanup:

• Wrapped the Image.open() call in a try/finally block that unconditionally calls os.unlink() on the temp file
• The file is deleted even if Image.open() or the resize/convert chain raises an exception

Input validation:

• Added validation for background_color to ensure it is a 6-character hex string, preventing arbitrary argument injection via the --background flag

Note on existing PR #862: That PR addresses #857 (shell injection only) by switching to an argument list, but passes data as a command-line argument rather than via stdin. This PR takes the approach recommended in #872 — stdin — which also avoids process-list exposure and argument-length limits. This PR additionally addresses the temp file leak and adds input validation, which #862 does not cover.

Additional Information

• The fix preserves the existing fallback behavior: if qrencode returns a non-zero exit code, qrimage_io falls back to the pure-Python qrimage encoder.
• Practical exploitation risk is low (SeedSigner is air-gapped), but the fixes are simple and reduce attack surface.
• Added import os to qr.py (previously not imported).
• Reordered imports to follow stdlib-first convention.

Screenshots

N/A — no UI changes. This is a security hardening of internal subprocess handling.

---

This pull request is categorized as a:

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

---

Checklist

I ran pytest locally

• All tests passed before submitting the PR
• I couldn't run the tests
• N/A

---

I included screenshots of any new or modified screens

Should be part of the PR description above.

• Yes
• No
• N/A

---

I added or updated tests

Any new or altered functionality should be covered in a unit test. Any new or updated sequences require FlowTests.

• Yes
• No, I'm a fool
• N/A

11 new tests in tests/test_qr_security.py covering:

• Subprocess called with argument list (no shell=True)
• Data passed via stdin, not as a command-line argument
• Bytes data passed directly without re-encoding
• Shell metacharacter injection payloads are not interpreted
• Temp file deleted after successful image read
• Temp file deleted even on Image.open() failure
• Fallback to pure-Python encoder on qrencode failure
• Valid hex colors accepted for background_color
• Invalid/malicious background_color values replaced with default
• Border values outside 1-10 default to 3
• capture_output=True is set to suppress stderr leakage

---

I tested this PR hands-on on the following platform(s):

• Raspberry Pi OS Manual Build
• SeedSigner OS on a Pi0/Pi0W board
• Emulator

Tested via mocked unit tests on Windows. The qrencode binary is a Linux/Pi dependency; all behavioral contracts are verified through mocks. Happy to test on Pi hardware if requested.

---

I have reviewed these notes:

• Keep your changes limited in scope.
• If you uncover other issues or improvements along the way, ideally submit those as a separate PR.
• The more complicated the PR, the harder it is to review, test, and merge.
• We appreciate your efforts, but we're a small team of volunteers so PR review can be a very slow process.
• Please only "@" mention a contributor if their input is truly needed to enable further progress.

• I understand

---

Thank you! Please join our Devs' Telegram group to get more involved.

Fixes #872
Also resolves #857

[aphrodoe]

I went through #857 and #872 and ran your test suite locally. The solution feels good to me. I do have a few minor nitpicks though, but mostly very specific edge cases that do not need immediate attention. Would be great from a future standpoint.

1. In line 111 in qr.py

tmp_path = "/tmp/qrcode.png"

Hardcoding /tmp/qrcode.png could probably lead to race conditions if multi-threading is ever done (for background generation). I think it won't cause any issue now though.

It might be safer this way:

import tempfile

with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as tmp_file:
    tmp_path = tmp_file.name

2. In line 135 in qr.py

img = Image.open(tmp_path).resize((width, height), Image.Resampling.NEAREST).convert("RGBA")

Image.open() will retain the underlying file handle descriptors in memory. I think it's cleaner to explicitly exhaust the file handle prior to the unlinking:

try:
    with Image.open(tmp_path) as original_img:
        img = original_img.resize((width, height), Image.Resampling.NEAREST).convert("RGBA")
    return img
finally:

[none34829]

Thanks for the review and for running the tests locally.

Good points on both. Here's my thinking:

Suggestion 2 (context manager on Image.open): Agreed- adopted this in the latest push. The file can hold sensitive data (PSBTs, xpubs), so explicitly closing the handle before unlink makes the intent clearer. On Linux unlink works regardless, but this is better practice.

Suggestion 1 (tempfile.NamedTemporaryFile): Valid in general, but SeedSigner runs single-threaded on a Pi Zero- there's no concurrent QR generation scenario. The hardcoded /tmp/qrcode.png is the original code's behavior; this PR just swaps the shell-injection-vulnerable call path around it. I'd rather keep the diff focused on the security fix and not change the tmp file strategy in the same PR. Happy to do it as a follow-up if maintainers want it.

[aphrodoe]

yeah, just gave suggestion 1 to remain foolproof in case of any future multi-threaded plans, not really a necessity as mentioned in my previous comment. Saw it as a diff and thought it was code from your side. But if it was the original behaviour, no issues.

[none34829]

Yeah, the hardcoded path was already there before this PR. Appreciate the review! :)