feat: asar integrity digest#1890
Conversation
erickzhao
force-pushed
the
feat/asar-integrity-digest
branch
2 times, most recently
from
99dcc60 to
9700342
Compare
Co-authored-by: Noah Gregory <noahmgregory@gmail.com>
erickzhao
force-pushed
the
feat/asar-integrity-digest
branch
from
9700342 to
24441f3
Compare
The "does not write digest when asar is disabled" test wrapped its only assertion in `if (sentinelIndex !== -1)`, silently passing with zero assertions if the sentinel was missing. Both sibling tests in the same describe block explicitly assert the sentinel exists — this one drifted. With test/config.json on 41.1.0 the sentinel is guaranteed present, so the guard only masked failures.
Previously setIntegrityDigest loaded the entire Electron Framework binary (150-500 MB) into memory, scanned it, modified 34 bytes, and wrote the whole thing back. Now it scans in 4 MB chunks with two concurrent workers (I/O overlaps Buffer.indexOf CPU) and patches only the 34-byte digest slot(s) via handle.write at the found position(s). Each chunk overreads sentinel.length-1 bytes so a sentinel straddling a boundary is still detected. Peak memory drops from ~binary-size to ~8 MB. Same approach as electron/fuses#96, which benched 10-15x faster on 150-300 MB binaries. Adds two synthetic-binary tests: one plants the sentinel across a 4 MB boundary (verified load-bearing via mutation), one plants a sentinel in each of two chunks to cover the universal-binary multi-write path. Both skip packager() and run in ~10ms.
setIntegrityDigest had grown to ~155 lines after the chunked-scan change. Pull the scan/validate/write block out to a module-level helper alongside isAsarIntegrity; the method now just gates on version, resolves integrity, computes the hash, and calls through. Sentinel is passed as a parameter rather than referenced via MacApp.INTEGRITY_DIGEST_SENTINEL to avoid a no-use-before-define disable comment.
nmggithub
left a comment
nmggithub
left a comment
There was a problem hiding this comment.
Tentatively approving this. As long as it works, I think it's good. My only concern would be how difficult this may be to refactor if we ever add a new version, but we might just have to cross that bridge when we come to it.
| await this.setIntegrityDigest(); | ||
| await this.signAppIfSpecified(); | ||
| await this.notarizeAppIfSpecified(); | ||
| return this.move(); |
There was a problem hiding this comment.
Minor, but would it be worth extracting a helper function or a comment on the order of operations here? I see the duplicated code in universal. Asking as I think it'd require contextual knowledge (or AI cross-referencing?) to know that the app can't be modified post-signing.
| ); | ||
| return; | ||
| } | ||
| if (!semver.gte(this.opts.electronVersion, '41.0.0-alpha.1')) { |
There was a problem hiding this comment.
Minor nit: Can we add a debug log here to match the log pattern for "Cannot determine Electron version"?
| for (const base of writePositions) { | ||
| await handle.write(payload, 0, payload.length, base); | ||
| } |
There was a problem hiding this comment.
Full disclosure: Claude is a close comrade.
Based on this loop, multiple sentinels would be patched, not specifically selected, when the runtime only reads one.
The runtime declares exactly one kIntegrityDictionaryDigest in __DATA_CONST,__asar_integrity, so the expected match count is 1 (or 2 for fat binaries containing both slices).
Calling this out as maybe we could also have a sentinels.length check to mirror the functionality/expectations/contract that electron/fuses L148-L154 utilizes
if (sentinels.length > 2) {
throw new Error(
`Found ${sentinels.length} copies of the fuse sentinel in the provided Electron binary. ` +
'At most 2 are expected (one per slice of a universal macOS binary). ' +
'This may indicate a corrupted binary or an unsupported build configuration.',
);
}
There was a problem hiding this comment.
I much prefer an open loop. Fat Mach binaries can technically support any number of binary images. I've seen some with three: 1 x86 image and 2 ARM ones (each for slightly different CPU configurations). Even if that would never happen with us, limiting it to 2 is completely unnecessary.
There was a problem hiding this comment.
Fair point! I guess the callout was to mirror the logic approach - should fuses restriction be relaxed then or are they completely decoupled? Maybe I misunderstood something here.
The high entropy sentinel should effectively minimize a chance of a stay collision, but if one were to happen, it'd be a fun adventure for someone to trace back. Maybe could worth validating expected account versus slice count to avoid setting a hard limit? Or maybe just adding a debug log here to call out how many slices were identified?
There was a problem hiding this comment.
I think the sentinel is high entropy enough to where it won't matter. I don't think the limit of 2 for the Fuses was a collision(false positive)-reduction feature, but just a simplification of the logic. If it were a collision-reduction feature, it, as a count, wouldn't even be a good one as it's would technically be possible for two matches in the same binary image.
I understand your paranoia, though. I'd prefer simply debug logging each time we write a digest instead of once at the end of the loop. I thought we already did the former, but perhaps not.
| async setIntegrityDigest() { | ||
| if (!this.opts.electronVersion || !semver.valid(this.opts.electronVersion)) { | ||
| debug( | ||
| `Cannot determine Electron version (got "${this.opts.electronVersion}"), skipping integrity digest`, | ||
| ); | ||
| return; | ||
| } | ||
| if (!semver.gte(this.opts.electronVersion, '41.0.0-alpha.1')) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Interesting callout from Claude. There are multiple instances of warning(...); return; blocks, but that doesn't mirror the approach of writePositions.length === 0 that throws an Error. These three preconditions warn-and-continue and also can be --quieted to be fully suppressed from dev logs.
Calling this out as there's no throw, so signAppIfSpecified can then sign and notarize an app whose framework slot still has used = 0.
await this.setIntegrityDigest();
await this.signAppIfSpecified();
await this.notarizeAppIfSpecified();
According to Claude, the runtime explicitly fails open in that case (w/ // No digest to validate against, fail open in integrity_digest.mm), creating a no-op that might be difficult for devs to debug.
if (kIntegrityDictionaryDigest.used == false)
return true; // No digest to validate against, fail open
There was a problem hiding this comment.
Runtime fails open on purpose because this is an optional feature. If it fails to apply, it's up to the packager to decide if it wants to continue.
4 participants
Description
This PR implements the logic from electron/asar#380 and electron/forge#4159 directly into the Packager pipeline.
The pivot to implementing this logic in
@electron/packagermeans that we get to extend the existingAsarIntegrityhandling logic and provide a zero-config way of implementing the integrity digest for consumers. Packager and Forge users will need to just bump up the version of Packager they're consuming to get this feature.For more details, see the original Electron PR: electron/electron#48587.