Release 1.5.1 - #48
Open
07souravkunda wants to merge 14 commits into
Open
07souravkunda wants to merge 14 commits into
07souravkunda wants to merge 14 commits into
Conversation
The unit suite could not be installed or run on any modern Ruby, and 3 integration tests errored in credential-less environments. This greens the baseline without weakening any test. Dependency/harness rot: - Gemfile/Gemfile.lock used an insecure `http://rubygems.org` source, which no longer serves the spec index -> `bundle install` failed. Switched to `https://`. - The lockfile pinned json 1.8.3 / minitest 5.8.4 / rake 12.3.3 with `BUNDLED WITH 1.11.2`. json 1.8.3 cannot build its native extension on Ruby 3.x, and the pinned Bundler was force-installed. Regenerated the lockfile with current, buildable versions and added the common Linux platforms for CI portability. Integration tests: - test_check_pid, test_is_running and test_multiple_binary start the real BrowserStackLocal binary and open a tunnel, so they require a valid BROWSERSTACK_ACCESS_KEY and network access. They now skip (rather than error) when no access key is present, so the suite stays green in bare environments. When a key is set they run in full, unchanged. Run the suite: bundle install bundle exec rake test Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Gemfile/Gemfile.lock fetched gems over plain http://rubygems.org with BUNDLED WITH 1.11.2 and no CHECKSUMS block, so nothing verified the content of a downloaded gem. rake executes arbitrary code from the Rakefile at test time, so a substituted tarball would run as the developer. Context: CVE-2020-8130 / GHSA-jppv-gw3r-w3q8 is an OS command injection in Rake::FileList, patched in rake 12.3.3. The old lockfile already pinned 12.3.3 so it was not itself vulnerable; the gap was that the *delivery* of that gem was unverifiable. This moves to rake 13.4.2 and makes delivery verifiable. - Gemfile.lock: regenerated with Bundler 2.7.1, adding a CHECKSUMS block with per-gem SHA-256 digests that Bundler verifies on every bundle install. - Gemfile: drop `gem "json"`. lib/ only uses JSON.parse/JSON.dump from the json default gem that ships with Ruby, and the gemspec declares no dependency on it, so a third-party json was a redundant build-time component -- and a native extension that fails to compile against Homebrew ruby@3.2 headers. - .gitignore: ignore .bundle/ and vendor/bundle/. .bundle/config can carry disable_checksum_validation, which would silently switch the new verification off, so it must never be committed. Verified: every digest matches the SHA-256 rubygems.org publishes for that version. Flipping one digest makes bundle install abort with "Bundler found mismatched checksums" (exit 37, nothing installed); with the CHECKSUMS block removed the same install exits 0 and performs no verification at all. Suite: 23 runs, 40 assertions, 0 failures, 0 errors, 3 skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ell (CWE-78)
verify_binary built its command by string concatenation:
IO.popen(bin_path + " --version")
The single-string form of IO.popen hands the whole thing to /bin/sh, so any
shell metacharacter in the binary path is interpreted rather than treated as
part of a filename. bin_path is assembled from @ordered_paths — the expanded
home directory, Dir.pwd and Dir.tmpdir — none of which are sanitised, so a
directory name containing ";" or "$()" turns a routine version check into
arbitrary command execution. Reproduced end to end through the public
LocalBinary#binary_path entry point.
The array form execs the binary directly and never involves a shell, which
also fixes a long-standing benign failure: a path containing a space (common
on macOS and Windows) used to be split by the shell, so verification of a
perfectly good cached binary failed and the binary was deleted and
re-downloaded on every run.
Deliberately not changed here, each tracked separately: the fail-open rescue
in this same method, the TOCTOU window between verification and execution,
and the other shell-string call sites in local.rb. A character allowlist on
the path was considered and rejected — with no shell involved it adds nothing,
and it would reject the legitimate space-containing paths this change fixes.
Adds two regression tests, both verified to fail before this change.
The dangerous-exec rule fires on any non-static first argument to IO.popen and does not model the array form, so it reports the fixed line as well as the vulnerable one it replaced -- the same rule is already open against master on the pre-fix line. Comment-only; no behaviour change. Suppression is scoped to this one rule on this one line, with the reason stated inline, so every other IO.popen/exec finding in this file still reports.
The existing injection test returns false post-fix via Errno::ENOENT rather than by demonstrating that the named file is executed verbatim, so it would also pass under a character-allowlist remediation instead of the array form. Putting the metacharacters in the directory name of a real, executable script asserts both halves at once: the injected command never runs, and the legitimate binary at that hostile-looking path still verifies. This is the same shape as the reproduction that exercises the vulnerability through $HOME. Verified to fail before the fix on the injection assertion.
Local#start created the logfile with
system("echo > #{@logfile}") # Windows
system("echo '' > '#{@logfile}'") # Unix
Both pass the caller-supplied logfile path through /bin/sh (or cmd.exe), so
shell metacharacters in the path are interpreted as commands. A logfile value
like "log' ; touch /tmp/pwned ; echo 'x" (Unix) or "NUL & calc.exe" (Windows)
runs arbitrary OS commands as the user running the gem (CWE-78).
Replace the block with a shell-free create/truncate: mkdir_p the parent dir,
then File.write(@logfile, ""), raising LocalException if the path is unwritable.
File.write treats the path purely as a filename, so no shell is involved. This
also fixes logfile paths containing spaces or a missing subdirectory, which the
old shell form silently failed on. Mirrors the fix already shipped in the python
binding.
Adds BrowserStackLocalLogfileTest with two regression tests (no creds/network):
a metacharacter payload no longer executes, and a space/missing-dir path is
created literally. Both fail on the pre-fix code.
The gem-publish workflow resolved actions/checkout, ruby/setup-ruby and rubygems/configure-rubygems-credentials from mutable tags (@V3 / @v1 / @v2.0.0). A hijacked or force-pushed upstream tag could redirect the resolved action code into the job that publishes the gem (CWE-829, supply-chain injection). Pin all three to full 40-char commit SHAs, with a version comment, matching the pattern the repo's Semgrep.yml already uses. - actions/checkout -> c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 - ruby/setup-ruby -> a0102e0972be65f351c307e2d64b9314a57c8073 # v1.324.0 - rubygems/configure-rubygems-credentials -> 762a4b77c3300434bb57c7ce80b20e36231927aa # v2.0.0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The public `command` method returned the start command string with the BrowserStack access key interpolated verbatim, so any caller that logged it (CI output, test runner logs, APM/error trackers) leaked the credential to a wider audience than the key itself. Ruby's default #inspect had the same problem, dumping @key when a Local instance was logged or raised. - command now returns the command with the key masked as [REDACTED] - start_command takes an optional redact flag; the execution path (start_command_args array, and the string form on legacy Ruby) keeps the real key, so the tunnel is unaffected - add a redacting #inspect so the key is never dumped by object inspection Proxy password is intentionally left visible (existing behaviour/tests). Adds regression tests that fail on the pre-fix code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
build: verify gem integrity at install time + green the unit-test baseline
fix: run the binary directly in verify_binary instead of through a shell (CWE-78)
fix: create the logfile without a shell (CWE-78 command injection)
ci: pin gem-push workflow actions to immutable commit SHAs
fix: redact access key in public command accessor and inspect (CWE-312)
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # and does not model the array form -- which is exactly the fix here, since | ||
| # no shell is spawned at all. Suppressed for this rule only. | ||
| # nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec | ||
| binary_response = IO.popen([bin_path, '--version']).readline |
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
2 participants
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.
No description provided.