fix(security): verify SHA-256 checksum on OpenShell binary download#697
fix(security): verify SHA-256 checksum on OpenShell binary download#697
Conversation
📝 WalkthroughWalkthroughTwo installer scripts now download an SHA‑256 checksum manifest and verify the downloaded OpenShell archive with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/install-openshell.sh (2)
82-98: Checksum verification correctly implemented with proper cleanup.This script has the EXIT trap at line 80, ensuring
$tmpdiris cleaned up regardless of success or failure. The checksum verification logic is consistent withscripts/install.sh.Same minor note about grep precision applies here:
♻️ Optional: Use fixed-string grep for precision
- (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && grep -F "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-openshell.sh` around lines 82 - 98, The checksum verification uses grep "$ASSET" "$CHECKSUM_FILE" which can mis-match substrings; update the verification command to use a fixed-string or exact-match grep (e.g., use grep -F or grep -x) so CHECKSUM_FILE is searched precisely for the ASSET entry before piping to shasum -a 256 -c -; modify the line that runs (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) to reference the chosen grep option and ensure variables GH_TOKEN, ASSET, CHECKSUM_FILE and tmpdir remain unchanged.
96-97: Consider handling missing checksum entry explicitly.If
grep "$ASSET"finds no match (asset not listed in checksum file), the pipeline will fail with "SHA-256 checksum verification failed." This is technically correct, but the error message could be misleading—the failure isn't a mismatch but a missing entry.♻️ Optional: More informative error handling
info "Verifying SHA-256 checksum..." -(cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ - || fail "SHA-256 checksum verification failed for $ASSET" +(cd "$tmpdir" && { + checksum_line=$(grep -F "$ASSET" "$CHECKSUM_FILE") \ + || fail "Asset $ASSET not found in $CHECKSUM_FILE" + echo "$checksum_line" | shasum -a 256 -c - +}) || fail "SHA-256 checksum verification failed for $ASSET"This is optional since the current behavior is safe (fails closed), just less descriptive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-openshell.sh` around lines 96 - 97, The failure message is ambiguous when the checksum file lacks an entry for ASSET; before piping into shasum, test for a matching line in CHECKSUM_FILE (e.g., use grep -q -- "$ASSET" "$CHECKSUM_FILE" or capture the grep output into a variable), and if no match call fail with a clear message like "No checksum entry for $ASSET in $CHECKSUM_FILE"; otherwise feed the matching line to shasum -a 256 -c - and keep the existing failure path for actual checksum mismatches.scripts/install.sh (1)
339-356: Good security improvement with checksum verification.The implementation correctly downloads and verifies the SHA-256 checksum before extraction. A few observations:
Missing tmpdir cleanup on failure: Unlike
install-openshell.shwhich hastrap 'rm -rf "$tmpdir"' EXIT(line 80), this script does not set up a trap. If checksum verification fails,$tmpdiris left behind.Grep pattern could be more precise:
grep "$ASSET"could match multiple lines if a filename is a substring of another (e.g.,openshell-x86_64matching bothopenshell-x86_64-apple-darwin.tar.gzand a hypotheticalopenshell-x86_64-v2-apple-darwin.tar.gz). Consider anchoring the pattern.♻️ Suggested improvements
tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT CHECKSUM_FILE="openshell-checksums-sha256.txt"For more precise grep matching (optional):
- (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && grep -F "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \Using
-Ftreats the pattern as a fixed string rather than a regex, which is safer and slightly faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 339 - 356, The script currently downloads checksums into $tmpdir but doesn't register a cleanup trap (leaving tmpdir if verification fails) and uses grep "$ASSET" which can match substrings; add a trap like the one in install-openshell.sh (trap 'rm -rf "$tmpdir"' EXIT) immediately after creating tmpdir to ensure removal on exit/failure, and make the verification grep precise by using fixed/anchored matching (e.g., use grep -F or anchor the pattern to the exact filename when reading CHECKSUM_FILE) in the (cd "$tmpdir" && grep ... "$CHECKSUM_FILE" | shasum -a 256 -c -) step so only the exact ASSET entry is checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install-openshell.sh`:
- Around line 82-98: The checksum verification uses grep "$ASSET"
"$CHECKSUM_FILE" which can mis-match substrings; update the verification command
to use a fixed-string or exact-match grep (e.g., use grep -F or grep -x) so
CHECKSUM_FILE is searched precisely for the ASSET entry before piping to shasum
-a 256 -c -; modify the line that runs (cd "$tmpdir" && grep "$ASSET"
"$CHECKSUM_FILE" | shasum -a 256 -c -) to reference the chosen grep option and
ensure variables GH_TOKEN, ASSET, CHECKSUM_FILE and tmpdir remain unchanged.
- Around line 96-97: The failure message is ambiguous when the checksum file
lacks an entry for ASSET; before piping into shasum, test for a matching line in
CHECKSUM_FILE (e.g., use grep -q -- "$ASSET" "$CHECKSUM_FILE" or capture the
grep output into a variable), and if no match call fail with a clear message
like "No checksum entry for $ASSET in $CHECKSUM_FILE"; otherwise feed the
matching line to shasum -a 256 -c - and keep the existing failure path for
actual checksum mismatches.
In `@scripts/install.sh`:
- Around line 339-356: The script currently downloads checksums into $tmpdir but
doesn't register a cleanup trap (leaving tmpdir if verification fails) and uses
grep "$ASSET" which can match substrings; add a trap like the one in
install-openshell.sh (trap 'rm -rf "$tmpdir"' EXIT) immediately after creating
tmpdir to ensure removal on exit/failure, and make the verification grep precise
by using fixed/anchored matching (e.g., use grep -F or anchor the pattern to the
exact filename when reading CHECKSUM_FILE) in the (cd "$tmpdir" && grep ...
"$CHECKSUM_FILE" | shasum -a 256 -c -) step so only the exact ASSET entry is
checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aed8a69f-7e86-4cd5-94b6-bd88862a8fe3
📒 Files selected for processing (3)
scripts/install-openshell.shscripts/install.shtest/runner.test.js
cv
force-pushed
the
fix/openshell-checksum-verify
branch
from
4233787 to
223e46e
Compare
wscurran
added
enhancement
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The checksum verification works for detecting transport corruption (truncated downloads, bit flips). However, both the binary and openshell-checksums-sha256.txt are fetched from the same GitHub release — an attacker who can compromise the release artifacts can replace both. This doesn't protect against the supply chain threat that fix(security) implies.
Filed upstream: NVIDIA/OpenShell#542 requests artifact attestations, which would provide cryptographic provenance verification.
Two items before merge:
install.shis missing atrap 'rm -rf "$tmpdir"' EXIT—install-openshell.shhas one (line 80), butinstall.shdoes not. On checksum failure, the downloaded binary persists on disk.- Consider
grep -Finstead ofgrepfor the checksum lookup —$ASSETcontains dots and hyphens that are regex metacharacters.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.sh (1)
358-360: Use exact field matching for checksum file lookup.At line 359,
grep "$ASSET"matches by regex/substring. For more robust checksum verification, match the exact filename field and verify exactly one entry exists.Proposed patch
info "Verifying SHA-256 checksum..." - (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && \ + awk -v asset="$ASSET" ' + $2 == asset { print; count++ } + END { exit(count == 1 ? 0 : 1) } + ' "$CHECKSUM_FILE" | shasum -a 256 -c -) \ || fail "SHA-256 checksum verification failed for $ASSET"This ensures the checksum file lookup matches the exact filename field rather than substring matching, preventing potential ambiguity if similarly named assets are added in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 358 - 360, The checksum lookup currently uses grep "$ASSET" which can match substrings; instead, in the block that runs in (cd "$tmpdir" && ... | shasum -a 256 -c -) use a strict field match against the filename (ASSET) in CHECKSUM_FILE and verify exactly one matching line is returned before piping to shasum. Concretely, replace the grep step with a precise extractor (e.g., awk or grep -F with field anchoring) that matches the filename field equal to the ASSET variable, count matches and fail if not exactly one, then pass that single line to shasum -a 256 -c -. Keep references to tmpdir, ASSET and CHECKSUM_FILE when locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.sh`:
- Around line 358-360: The checksum lookup currently uses grep "$ASSET" which
can match substrings; instead, in the block that runs in (cd "$tmpdir" && ... |
shasum -a 256 -c -) use a strict field match against the filename (ASSET) in
CHECKSUM_FILE and verify exactly one matching line is returned before piping to
shasum. Concretely, replace the grep step with a precise extractor (e.g., awk or
grep -F with field anchoring) that matches the filename field equal to the ASSET
variable, count matches and fail if not exactly one, then pass that single line
to shasum -a 256 -c -. Keep references to tmpdir, ASSET and CHECKSUM_FILE when
locating code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d2bfc6e-5547-4eb2-a3d4-80ccfa89a488
📒 Files selected for processing (3)
scripts/install-openshell.shscripts/install.shtest/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/install-openshell.sh
- test/runner.test.js
3 participants
Summary
Both
scripts/install.shandscripts/install-openshell.shdownload the OpenShell binary viacurl/ghand install it (often as root) without any integrity verification. OpenShell releases includeopenshell-checksums-sha256.txtalongside the tarballs — verified present in v0.0.10 through v0.0.13.Both files now:
openshell-checksums-sha256.txtalongside the tarball (bothghandcurlpaths)shasum -a 256 -cbefore extractionTest plan
openshell-checksums-sha256.txtandshasum -a 256 -cnemoclaw onboardinstalls OpenShell with checksum verificationSummary by CodeRabbit
New Features
Tests