Skip to content

utils: delete the TLS key-log scratch file after loading it - #5131

Open
KernelClint wants to merge 1 commit into
secdev:masterfrom
KernelClint:pcapng-delete-keylog-scratch
Open

utils: delete the TLS key-log scratch file after loading it#5131
KernelClint wants to merge 1 commit into
secdev:masterfrom
KernelClint:pcapng-delete-keylog-scratch

Conversation

@KernelClint

Copy link
Copy Markdown
Contributor

A pcapng capture can carry Decryption Secrets Blocks holding TLS key material. load_nss_keys()
reads a key log from a path, so
scapy/utils.py:1994-2010
writes each block out to a temporary file first.

get_temp_file() registers what it creates for deletion at interpreter exit
(scapy/utils.py:194-207, scapy/config.py:1255-1265). Nothing deletes it earlier, so the file
survives the reader that made it. A capture with many secret blocks leaves one file per block for
the life of the process: a 1,991-byte gzipped capture containing 200 blocks left 200 files and
232,600 bytes on disk after the reader was closed. The same capture with only the block type
changed left none.

The file is needed only for the duration of one call, so the change scopes it to that:

-                filename = get_temp_file()
-                with open(filename, "wb") as fd:
-                    fd.write(secrets_data)
-                keys = load_nss_keys(filename)
+                filename = get_temp_file(keep=True)
+                try:
+                    with open(filename, "wb") as fd:
+                        fd.write(secrets_data)
+                    keys = load_nss_keys(filename)
+                finally:
+                    os.unlink(filename)

keep=True stops the exit-time registration; the finally removes the file immediately. Valid key
logs load exactly as before.

The added regression reads a capture with several secret blocks and asserts the temporary directory
is unchanged afterwards. Without the source change it fails.

Performance was measured on one computer, before and after the fix: reading a capture took 147.6 µs
before and 150.7 µs after. Repeat runs moved by about 2%, so that difference is smaller than the
test can distinguish.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.12%. Comparing base (b3bbcc8) to head (510b411).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5131      +/-   ##
==========================================
- Coverage   80.63%   80.12%   -0.52%     
==========================================
  Files         390      390              
  Lines       96936    96937       +1     
==========================================
- Hits        78168    77672     -496     
- Misses      18768    19265     +497     
Files with missing lines Coverage Δ
scapy/utils.py 72.55% <100.00%> (-0.04%) ⬇️

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread scapy/utils.py
filename = get_temp_file(keep=True)
try:
with open(filename, "wb") as fd:
fd.write(secrets_data)
@KernelClint

Copy link
Copy Markdown
Contributor Author

Two CI notes, both worth stating rather than leaving for a reviewer to work out.

CodeQL flags scapy/utils.py:2011 as clear-text storage of a secret. That is the fd.write(secrets_data) line, and it is pre-existing behaviour: the key log has always been written to a temporary file so that load_nss_keys() can open it by path. It appears as a new alert only because this change re-indents that line into a try block, so the diff touches it.

The change makes the exposure shorter, not longer. Before, the file was created through get_temp_file() and removed at interpreter exit; now it is removed as soon as the keys have been parsed. The alert is fair about the underlying design, and removing the temporary file entirely would mean giving load_nss_keys() a way to accept the key log as bytes rather than a path. That is a larger change to a public function, so I have kept it out of this one — happy to do it here instead if you would prefer.

The macOS TLS job failure looks unrelated to this change. It is Testing TLS client against ssl.SSLContext server with TLS 1.3 and a post-handshake authentication, a live client/server exchange. This patch cannot affect it: load_nss_keys() parses the file into a dictionary and keeps no reference to the path, so unlinking afterwards has no bearing on a later handshake. The same job passes on the other pull requests in this batch. A re-run would confirm it.

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.

2 participants