ensure ruby 4 compatibility and address some security concerns - #162
Open
srosenhamer wants to merge 7 commits into
Open
ensure ruby 4 compatibility and address some security concerns#162srosenhamer wants to merge 7 commits into
srosenhamer wants to merge 7 commits into
Conversation
The build has not run since 2022. Every task was broken in some way: - Gemfile pinned bundler "~> 2.1" and rake "~> 12.0"; the current toolchain is bundler 4.x and rake 13.x, so `bundle install` could not resolve at all. Drop the bundler pin (declaring bundler in a Gemfile is discouraged) and relax rake to >= 13.0. - rdoc stopped being a default gem in Ruby 4.0, so the Rakefile's unconditional `require "rdoc/task"` aborted *every* rake task, including `rake test`. Guard it behind a LoadError rescue and declare rdoc as a development dependency. Also drop the 'hanna' generator, which was never a declared dependency and made `rake rdoc` fail out of the box. - Replace the yanked, unmaintained codecov gem with plain simplecov. - CI targeted ubuntu-18.04, a runner image GitHub deleted in December 2022, so no job has even been scheduled in ~3.5 years. Move to ubuntu-latest, actions/checkout@v4, and bundler-cache, and test 3.1 through 4.0 rather than 2.5 through 3.1 (all of which are now EOL). Add a frozen-string-literal run and a bundle-audit job. - Declare required_ruby_version (previously unset entirely) and put upper bounds on the minitest and mocha development dependencies. Collapse the two unreachable legacy RubyGems branches in the gemspec. - test_start.rb declared a mocha expectation with keywords while Net::SFTP.start passes ssh_options positionally. That match breaks under mocha's strict keyword matching; brace the hash as the sibling test already does. Baseline after this commit: 433 runs, 1201 assertions, 0 failures on Ruby 4.0.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A filename in an FXP_NAME response is chosen entirely by the server, and the
only sanitisation in the library was an exact-string comparison against "."
and "..". Everything else was joined straight onto the local destination and
handed to Dir.mkdir and File.open.
So a malicious or compromised server answering readdir with the single name
"../../.ssh/authorized_keys" during
sftp.download!(remote, "/home/user/dl", recursive: true)
produced File.join("/home/user/dl", "../../.ssh/authorized_keys"), which the
kernel resolves to /home/.ssh/authorized_keys. That is an arbitrary local file
write, with content the server chooses, at the client process's privileges.
Absolute names were not the vector -- File.join keeps those under the root --
".." was.
validate_entry_name! now requires each entry to be a plain path component.
Comparing against File.basename rejects "..", embedded separators and absolute
paths in one check, and is correct on Windows too, where File.basename also
understands "\". "." and ".." keep being skipped silently, since they are
legitimate entries in any listing.
Two related weaknesses at the same sinks:
- Dir.mkdir was guarded by File.directory?, which follows symlinks, so a
symlink planted at the destination silently accepted the check and every
child file was written through it. make_directory now refuses a symlink.
- File.open(path, "wb") followed and truncated an existing symlink target.
open_sink now passes O_NOFOLLOW where the platform defines it.
Verified: removing the validate_entry_name! call fails the six new
unsafe-name tests; restoring it passes all 442.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several SFTP structures are encoded as a 32-bit count followed by that many items. The count is chosen entirely by the server and can be up to 4,294,967,295. Net::SSH::Buffer clamps reads at end-of-buffer and returns nil rather than raising, so these loops did not terminate early on truncated input -- they just spun, allocating. Measured against the previous code: a 12-byte payload declaring 500,000 ACL entries allocated 500,000 structs in 0.617s with no exception. At the maximum count that is ~4.3 billion structs, i.e. hundreds of gigabytes and an OOM kill, from a packet an attacker can send in a single frame. parse_extended is the same shape but flat in memory, so it hangs the SSH event loop instead. Both are reachable from any ordinary stat!, dir.entries, or recursive download. Add Protocol::BoundedRead#read_bounded_count!, which validates the count against the bytes actually remaining in the buffer -- a count larger than the buffer could possibly hold cannot be legitimate -- and apply it to: - V04::Attributes.parse_acl - V01::Attributes.parse_extended - V01::Base#parse_name_packet - V04::Base#parse_name_packet Also cap accepted packet length at 256KiB, matching OpenSSH's sftp-server. Previously session.rb read a raw uint32 length and would buffer toward 4GiB, or wait indefinitely for bytes that never arrive, with no way to reclaim it. The same 500,000-entry ACL input now raises in 0.025ms. The new tests assert both the exception and a wall-clock bound, so a regression surfaces as a failure rather than a hung CI job. 450 runs, 1276 assertions, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four ways a server could disrupt or crash a client: FXP_VERSION was dispatched at any point in the session, not just during the handshake. A second one swapped the protocol driver mid-flight, discarded @pending_requests -- orphaning any caller blocked in Request#wait, forever -- and then raised NoMethodError on the already-cleared @on_ready list. do_version now refuses to run unless the session is still :init. dispatch_request raised "no such request" for any unrecognised response id, so one small unsolicited or duplicated packet tore down the caller's event loop. Unknown ids are now logged and ignored. Version negotiation is min(server_version, 6) and the server alone picks the number, with no floor and no signal to the caller. A server declaring v1 silently put the client on the v1 driver, where rename, readlink, symlink and link are all NotImplementedError. Add an opt-in :min_version, defaulting to nil so existing behaviour is unchanged. Transfer callbacks run inside a Net::SSH channel callback and every one of them can raise StatusException, with no ensure anywhere: the exception unwound through the event loop leaving local file handles open, partial files on disk, and @Active never decremented. Route the handlers through a wrapper that aborts and cleans up before re-raising, and track the files the downloader opened itself so abort! can close them. Caller-supplied IO objects are left alone. Two smaller things in the same code: - The extensions hash parsed out of FXP_VERSION was assigned to a local and thrown away. Expose it as Session#extensions. - Protocol versions 1-3 make the permissions bits optional, so Name#directory? returns nil when the server omits them. Such an entry was attempted as a file and, if it was really a directory, the failed open aborted the entire recursive download. Skip the ambiguous entry and keep going. 456 runs, 1291 assertions, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two confirmed FrozenError sites under --enable-frozen-string-literal: - V04::Name#longname seeded from the literal "d"/"l"/"-" and then appended to it 14 times. This one already emitted a chilled-string deprecation warning on plain Ruby 4.0.1. - Operations::File assigned @buffer = "" at four sites and then did @buffer << data, which killed read, gets, readline and eof? -- the whole synchronous file API. A third site, Dir#glob's e.name.replace(...), reached into a Name and mutated its string in place. It worked only because Net::SSH::Buffer#read_string happens to return an unfrozen string. Fixed in the same shape as the upstream PR (net-ssh#157, open since 2024) rather than a nicer-looking rewrite, so that a future merge from upstream does not conflict: unary plus on the seed literals, and attr_accessor :name on both Name classes so glob can assign instead of mutate. Then add # frozen_string_literal: true to all 56 lib, test and build files. The suite now passes identically with and without --enable-frozen-string-literal, and emits no chilled-string warnings. Two other Ruby 4.0 items found while getting the run clean: - Ruby 4.0 deprecates assigning a non-nil value to $/ and $\. The library only reads them, mirroring ::IO, which is fine; it was test_file.rb's setup that warned on every test. Those assignments are deliberate, so wrap them rather than drop the coverage. - Operations::File#write called data.bytes.length twice per write, each call materialising the whole byte array. bytesize is O(1) and is what download.rb already used. The one remaining warning in the run comes from mocha requiring 'cgi', which Ruby 4.0 removed; that is a dependency issue, not ours. 456 runs, 1291 assertions, 0 failures on both Ruby 3.4.4 and 4.0.1, in both frozen and unfrozen modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
setup.rb was Minero Aoki's 2004 installer, committed once in 2008 and never touched. Nothing referenced it, it had been non-functional since Ruby 2.2 removed ::Config::CONFIG, and it shipped in the published gem -- putting an LGPL-2.1 file inside a gem that declares spec.license = "MIT". If anyone did run it, its `system` calls source their command strings from a config.save in the current directory. Deleted, and also added to the gemspec reject pattern so a stale working copy cannot reintroduce it. Manifest was a hoe-era artifact listing a file that does not exist and omitting most of the ones that do. The gemspec uses `git ls-files`. Deleted. Net::SFTP.start used `rescue Object`, whose handler then called session.shutdown! on a variable that is nil when Net::SSH.start itself raised; the resulting NoMethodError was caught by an empty `rescue ::Exception` and swallowed. Narrow to `rescue ::Exception` with an explicit nil check, and log rather than silently discard a shutdown failure. The broad rescue is deliberate -- an Interrupt during setup must still tear down the session -- and the original exception is always re-raised. `alias :loop_forever :loop` sat above the definition of Session#loop, so it actually aliased Kernel#loop and inherited its private visibility. Calling sftp.loop_forever raised NoMethodError: private method. Moved below the definition. Rakefile: `gem cert --days 365*5` is not shell arithmetic; gem cert coerces the argument with to_i, giving 365. That is exactly why the certificate got a one-year life instead of the intended five. Also `raise Exception` -> a RuntimeError, and drop the duplicated release prerequisite. Document the expired signing certificate in both the README and CHANGES, since the high-security install the README recommends cannot work until someone with the signing key regenerates it. 456 runs, 1291 assertions, 0 failures. bundle-audit: no vulnerabilities found. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FXP_INIT was sent with `@version || HIGHEST_PROTOCOL_VERSION_SUPPORTED`, but
do_version then computed the negotiated version as
`[server_version, HIGHEST_PROTOCOL_VERSION_SUPPORTED].min` -- capping against
the constant rather than against what was actually offered.
Real servers answer FXP_INIT with their own version regardless of the client's
offer; OpenSSH always replies 3. So Net::SFTP.start(host, user, {}, version: 1)
against OpenSSH negotiated min(3, 6) = 3 and loaded the v3 driver. The
:version option pinned nothing.
The existing unit test did not catch this because its scripted server echoes
back the same version the client offered. Found by running against a real
OpenSSH sftp-server.
Cap against the offered version instead.
457 runs, 1293 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
net-sftp: security hardening and Ruby 4.x compatibility
Security fixes
lib/net/sftp/operations/download.rb) - addedvalidate_entry_name!, rejecting any server-supplied filename that isn't a single path component. Previously only"."and".."were filtered, so a malicious server returning../../.ssh/authorized_keysduring a recursive download gotFile.join+File.open(..., "wb")to write outside the target.make_directoryrefuses a pre-existing local symlink;open_sinkusesO_NOFOLLOWwhere the platform defines it.Protocol::BoundedRead#read_bounded_count!validates server item counts against bytes actually remaining. Applied toparse_acl,parse_extended, and bothparse_name_packetimplementations. Measured:parse_aclbuilt 500,000 structs from a 12-byte payload in 0.617s before; 0.025ms and a clean exception after.MAX_PACKET_LENGTH = 256KiB, matching OpenSSH. Previously an unvalidateduint32.FXP_VERSIONrejected -do_versionnow requiresstate == :init, so a mid-session version packet can't swap the protocol driver or orphan pending requests.Bug fixes
:versionnow actually pins - negotiation capped against the library constant instead of the offered version, soversion: 1against OpenSSH silently used v3. Found by the real-server run, not the unit suite (the mock server echoes back whatever the client offers).loop_foreverwas broken - the alias sat aboveSession#loop, so it aliased privateKernel#loopand raisedNoMethodError. Moved below the definition.rescue ObjectinNet::SFTP.startnarrowed, with a nil check onsessionand logging instead of an emptyrescue ::Exception.directory?nil.Ruby 4.x compatibility
FrozenErrorsites:V04::Name#longnameand theOperations::Filebuffer. Matched upstream PR Handle frozen strings #157's exact shape so a future merge won't conflict.attr_accessor :nameon bothNameclasses soDir#globassigns instead of mutating in place.# frozen_string_literal: trueto all 56 lib/test/build files.$/and$\assignments, which Ruby 4.0 deprecates.bytes.lengthtobytesizeinFile#write(was materialising the byte array twice per write).Build and packaging
require "rdoc/task"behindLoadError(rdoc stopped being a default gem in 4.0 and was aborting every rake task); fixed--days 365*5to1825; dropped the undeclaredhannagenerator.bundler "~> 2.1"pin, relaxed rake to>= 13.0, replaced the yankedcodecovwithsimplecov, addedbundler-auditandrdoc.required_ruby_version = ">= 3.1"(previously unset), bounded minitest and mocha, collapsed two unreachable legacy branches.ubuntu-latestandcheckout@v4(the oldubuntu-18.04image was deleted in 2022, so nothing had run in ~3.5 years); matrix 3.1 through 4.0; added a frozen-string-literal run and abundle-auditjob.New API
Net::SFTP.start(..., min_version:)- refuses a server that negotiates below your required version.Session#extensions- exposes what the server advertises inFXP_VERSION. Previously parsed and thrown away.Removed
setup.rb(1,331 lines) - 2004-era installer, broken since Ruby 2.2, referenced by nothing, LGPL inside an MIT gem. Also added to the gemspec reject pattern.Manifest- stale hoe-era artifact.Tests and docs
test/test_parser_limits.rb), +5 session tests, +1 sink-cleanup test.CHANGES.txtrecords everything including the expired-cert known issue.Not fixed, and not fixable here
lib/, and its defaults put the two vulnerable ETM MACs first withchacha20-poly1305at the front of the cipher list. Mitigation is caller-side: pass explicithmac:/encryption:lists excluding those.rake releasestays blocked. The signing cert expired 2023-09-22 and regenerating it needs a private key that isn't in this repo.Commits