Security hardening: FFI validation, CI script injection fix, Dockerfile improvements - #6
Open
pavanchow wants to merge 1 commit into
Open
Conversation
…le improvements
1. Validate k parameter in AES FFI entrypoints (Cipher, InvCipher,
KeyExpansion) to prevent OOB access and panic-across-FFI UB when
k is not 128/192/256. Add null pointer checks.
2. Fix shell injection in CI scripts: replace find -exec sh -c with
embedded {} (allows filename-based command injection) with safe
positional argument passing. Quote all variable expansions.
3. Narrow chmod in cryptol-tools.Dockerfile to specific solver
binaries instead of chmod a+x /usr/local/bin/*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses three security issues identified during a review of the Foundation repository: unsafe FFI boundary handling in the AES Rust implementations, a shell injection vulnerability in the CI build scripts, and an overly broad file permission change in a Dockerfile.
1. FFI input validation (AES Cipher, InvCipher, KeyExpansion)
The
kparameter in all AES FFI entrypoints was unchecked. Invalid values (e.g. k=0, k=999) cause out-of-bounds array access onw[64]inkey_expansionand division by zero ini % nk. Since these areextern "C"functions, a panic is undefined behavior per Rust's FFI contract.Fix: validate
kis 128, 192, or 256 before proceeding. Add null pointer checks on all raw pointer arguments.2. CI script shell injection and quoting
.ci/build-rust-saw-artifacts.shline 9 usedfind -exec sh -c "mv '{}' ..."which embeds filenames directly into a shell command string. A filename containing shell metacharacters (e.g. single quotes with command substitution) achieves arbitrary command execution.All four CI scripts also used unquoted
$fin for loops, which breaks on paths containing spaces.Fix: pass
{}as a positional argument (sh -c '...' _ {}), usefind -print0withwhile read -d ''for safe iteration.3. Dockerfile chmod scope
cryptol-tools.Dockerfileusedchmod a+x /usr/local/bin/*which marks every file in the directory as executable. Narrowed to specific solver binaries.Files changed
algorithms/AES/rust/Cipher/src/lib.rs- k validation + null checks in cipher_ffi, inv_cipher_ffialgorithms/AES/rust/KeyExpansion/src/lib.rs- k validation + null checks in key_expansion_ffi.ci/build-rust-saw-artifacts.sh- shell injection fix + safe quoting.ci/build-rust-ffi.sh- safe quoting.ci/run-saw-python-files.sh- safe quoting.ci/run-top-level-saw-files.sh- safe quoting.github/dockerfiles/cryptol-tools.Dockerfile- narrow chmod scope