fix: lazy PKG_NATIVE_CACHE_PATH eval + integrity check for cached .node files#228
fix: lazy PKG_NATIVE_CACHE_PATH eval + integrity check for cached .node files#228olegcS wants to merge 1 commit intoyao-pkg:mainfrom
Conversation
…de files Two improvements to native addon extraction in process.dlopen: 1. Read PKG_NATIVE_CACHE_PATH lazily inside process.dlopen instead of capturing it once at bootstrap time. This allows applications to set process.env.PKG_NATIVE_CACHE_PATH at runtime (e.g. in an init script) before any native addon is loaded, which was previously impossible because the IIFE captured the value before any user code ran. 2. Add SHA-256 integrity verification for cached .node files in the single-file code path (the else branch). The node_modules path already had hash verification via copyFolderRecursiveSync, but the single-file path only checked fs.existsSync. A tampered or corrupted cached file is now detected and re-extracted from the snapshot. This closes a privilege escalation vector where native addons are extracted to user-writable directories (e.g. ~/.cache/pkg/) and can be replaced with malicious .node files that execute at the privilege level of the packaged application. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This could have security implications BTW... |
There was a problem hiding this comment.
Pull request overview
Improves native addon extraction in prelude/bootstrap.js by making PKG_NATIVE_CACHE_PATH runtime-overridable and by adding an integrity check to prevent loading tampered cached .node files from the native cache directory.
Changes:
- Lazily re-reads
process.env.PKG_NATIVE_CACHE_PATHon eachprocess.dlopeninvocation (with a startup-captured default fallback). - Adds SHA-256 verification of cached single-file
.nodeaddons and re-extracts from the snapshot if the cache is corrupted/tampered.
| // Native addons will be extracted to: <PKG_NATIVE_CACHE_BASE>/pkg/<hash> | ||
| const PKG_NATIVE_CACHE_BASE = | ||
| // | ||
| // Note: We capture the initial value as the default, but re-read process.env inside | ||
| // process.dlopen so that runtime changes to PKG_NATIVE_CACHE_PATH take effect. | ||
| const PKG_NATIVE_CACHE_DEFAULT = |
There was a problem hiding this comment.
The comment still refers to <PKG_NATIVE_CACHE_BASE>/pkg/<hash>, but the code now uses cacheBase and PKG_NATIVE_CACHE_DEFAULT. Please update the wording to match the current variables/env var behavior to avoid misleading users reading this section.
| const cachedContent = fs.readFileSync(tmpModulePath); | ||
| const cachedHash = createHash('sha256').update(cachedContent).digest('hex'); | ||
| if (cachedHash !== hash) { | ||
| // Cached file was tampered with or corrupted — re-extract from snapshot |
There was a problem hiding this comment.
The existsSync -> readFileSync/hash flow can throw if the cached file is deleted/replaced between the calls (or if permissions/locks prevent reading). Consider wrapping the integrity check in a try/catch and falling back to re-extracting from the snapshot (or treating read errors as a cache miss) to avoid crashing in these race/error cases.
| const cachedContent = fs.readFileSync(tmpModulePath); | |
| const cachedHash = createHash('sha256').update(cachedContent).digest('hex'); | |
| if (cachedHash !== hash) { | |
| // Cached file was tampered with or corrupted — re-extract from snapshot | |
| try { | |
| const cachedContent = fs.readFileSync(tmpModulePath); | |
| const cachedHash = createHash('sha256') | |
| .update(cachedContent) | |
| .digest('hex'); | |
| if (cachedHash !== hash) { | |
| // Cached file was tampered with or corrupted — re-extract from snapshot | |
| fs.copyFileSync(modulePath, tmpModulePath); | |
| } | |
| } catch (_) { | |
| // Treat read/hash failures as a cache miss and restore from snapshot. |
| // Re-read PKG_NATIVE_CACHE_PATH at each call so runtime changes take effect | ||
| const cacheBase = process.env.PKG_NATIVE_CACHE_PATH || PKG_NATIVE_CACHE_DEFAULT; | ||
| const tmpFolder = path.join(cacheBase, 'pkg', hash); |
There was a problem hiding this comment.
This change introduces new runtime behavior (lazy PKG_NATIVE_CACHE_PATH evaluation and integrity verification for cached .node files) but there are no accompanying tests. Please add/extend a test to cover: (1) setting process.env.PKG_NATIVE_CACHE_PATH at runtime before first native addon load, and (2) detecting a tampered cached .node file and forcing re-extraction.
robertsLando
left a comment
There was a problem hiding this comment.
Thanks for the PR — the integrity check (part 2) is a welcome security improvement. However, I have concerns about the lazy PKG_NATIVE_CACHE_PATH evaluation (part 1) which I believe introduces a new security vector.
Lazy eval concern
Currently, PKG_NATIVE_CACHE_PATH is captured once at bootstrap time, before any user or dependency code runs. This is a security-positive property: no JavaScript code in the application can influence where native addons get extracted/loaded from.
With the lazy eval change, process.env.PKG_NATIVE_CACHE_PATH is re-read on every process.dlopen call. This means any code that runs before a native addon require() — including a compromised or malicious dependency — can set process.env.PKG_NATIVE_CACHE_PATH to an attacker-controlled path. This enables:
- Arbitrary directory creation —
fs.mkdirSync(tmpFolder, { recursive: true })creates directories at the attacker-chosen location. - Arbitrary file writes —
fs.copyFileSync(modulePath, tmpModulePath)extracts.nodefiles to that location. - Bypassing the integrity check — the
node_modulespath (mIndex > 0branch) callscopyFolderRecursiveSyncwhich copies to the new path without the SHA-256 verification added in this PR, so a pre-placed malicious file tree there could be loaded.
If users need to set the cache path, they should do so via an OS-level environment variable before the process starts — which is already supported today. Making it mutable at runtime from JS code defeats the purpose of having it frozen at bootstrap.
Suggestion
I'd recommend splitting this into two PRs:
- Accept part 2 (SHA-256 integrity check for cached
.nodefiles) — this is a genuine fix for the cache poisoning vector. - Drop part 1 (lazy eval) — or, if there's a strong use case for runtime override, it needs a much more restrictive design (e.g., only allow it to be set once, before any
dlopencall, and freeze it afterward).
| const PKG_NATIVE_CACHE_BASE = | ||
| // | ||
| // Note: We capture the initial value as the default, but re-read process.env inside | ||
| // process.dlopen so that runtime changes to PKG_NATIVE_CACHE_PATH take effect. |
There was a problem hiding this comment.
Security concern: Re-reading process.env.PKG_NATIVE_CACHE_PATH at each dlopen call means any code (including a compromised dependency) that runs before a native addon require() can redirect where addons are extracted to. This turns an immutable, bootstrap-time config into a mutable runtime setting, opening the door to arbitrary directory creation and file writes at attacker-chosen paths.
The current behavior — capturing the value once before any user code runs — is the safer design. If runtime override is truly needed, consider a freeze-after-first-use pattern instead.
Summary
Two improvements to native addon extraction in
process.dlopen(prelude/bootstrap.js):1. Lazy
PKG_NATIVE_CACHE_PATHevaluationProblem:
PKG_NATIVE_CACHE_PATHis captured in aconstinside the IIFE at bootstrap time (before any user code runs). Applications that need to setprocess.env.PKG_NATIVE_CACHE_PATHat runtime — for example, to redirect native addon extraction to a protected directory — cannot do so because the value is already frozen.Fix: Re-read
process.env.PKG_NATIVE_CACHE_PATHinsideprocess.dlopenon each call, falling back to the value captured at startup. This is fully backward-compatible: if the env var is set before the process starts, behavior is unchanged. If set at runtime before the first native addon load, the new value takes effect.2. SHA-256 integrity verification for cached
.nodefilesProblem: The single-file code path (the
elsebranch inprocess.dlopen) only checksfs.existsSyncbefore loading a cached.nodefile. If a cached file has been replaced (e.g. by a local user placing a malicious.nodein~/.cache/pkg/), it is loaded without any verification. This enables a privilege escalation attack when the packaged application runs at a higher privilege level than the user who controls the cache directory.Note: The
node_modulescode path (mIndex > 0) already has hash verification viacopyFolderRecursiveSync, so this only affects the single-file path.Fix: Before loading a cached
.nodefile, compute its SHA-256 hash and compare it against the expected hash (already computed from the snapshot content on line 2210). If they don't match, re-extract from the snapshot.Security context
Native addons are extracted by default to
~/.cache/pkg/, a user-writable directory. When a pkg-packaged application runs with elevated privileges (e.g. as SYSTEM on Windows during installation), a standard user can pre-place a malicious.nodefile in the cache directory. The application then loads it at its elevated privilege level, granting the user arbitrary code execution as SYSTEM.Test plan
PKG_NATIVE_CACHE_PATHas OS env var before process start — extraction should use the specified path (unchanged behavior)process.env.PKG_NATIVE_CACHE_PATHin application code before first native addonrequire()— extraction should use the new path.nodefile with a different binary — application should re-extract from snapshot instead of loading the tampered filenode_modulespath (mIndex > 0) —copyFolderRecursiveSyncbehavior unchanged🤖 Generated with Claude Code