Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

[RubyGemsGemInstaller] Validate checksums from the compact index #4851

Merged
merged 6 commits into from
Aug 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions lib/bundler/rubygems_gem_installer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,41 @@ def self.at(*args)
def check_executable_overwrite(filename)
# Bundler needs to install gems regardless of binstub overwriting
end

def pre_install_checks
super && validate_bundler_checksum(options[:bundler_expected_checksum])
end

private

def validate_bundler_checksum(checksum)
return true if Bundler.settings[:disable_checksum_validation]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the setting disable.checksum_validation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of the other bool keys have dots in them, and wouldn't that break doing the setting as an env var?

return true unless checksum
return true unless source = @package.instance_variable_get(:@gem)
return true unless source.respond_to?(:with_read_io)
digest = source.with_read_io do |io|
digest = Digest::SHA256.new
digest << io.read(16_384) until io.eof?
io.rewind
digest.send(checksum_type(checksum))
end
unless digest == checksum
raise SecurityError,
"The checksum for the downloaded `#{spec.full_name}.gem` did not match " \
"the checksum given by the API. This means that the contents of the " \
"gem appear to be different from what was uploaded, and could be an indicator of a security issue.\n" \
"(The expected SHA256 checksum was #{checksum.inspect}, but the checksum for the downloaded gem was #{digest.inspect}.)\n" \
"Bundler cannot continue installing #{spec.name} (#{spec.version})."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the principle of empowering users to resolve their own errors, how do you feel about also printing the full path, suggesting deleting the gem with the bad checksum, and explaining that if they are sure they want to install this gem despite the checksum not matching, they can bundle config disable.checksum_validaiton true?

end
true
end

def checksum_type(checksum)
case checksum.length
when 64 then :hexdigest!
when 44 then :base64digest!
else raise InstallError, "The given checksum for #{spec.full_name} (#{checksum.inspect}) is not a valid SHA256 hexdigest nor base64digest"
end
end
end
end
1 change: 1 addition & 0 deletions lib/bundler/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class Settings
BOOL_KEYS = %w(
allow_offline_install
cache_all
disable_checksum_validation
disable_exec_load
disable_local_branch_check
disable_shared_gems
Expand Down
3 changes: 2 additions & 1 deletion lib/bundler/source/rubygems.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ def install(spec, opts = {})
:bin_dir => bin_path.to_s,
:ignore_dependencies => true,
:wrappers => true,
:env_shebang => true
:env_shebang => true,
:bundler_expected_checksum => spec.respond_to?(:checksum) && spec.checksum
).install
end
spec.full_gem_path = installed_spec.full_gem_path
Expand Down
32 changes: 32 additions & 0 deletions spec/install/gems/compact_index_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -695,4 +695,36 @@ def start
expect(File.read(versions)).to start_with("created_at")
expect(the_bundle).to include_gems "rack 1.0.0"
end

describe "checksum validation", :rubygems => ">= 2.3.0" do
it "raises when the checksum does not match" do
install_gemfile <<-G, :artifice => "compact_index_wrong_gem_checksum"
source "#{source_uri}"
gem "rack"
G
expect(exitstatus).to eq(19) if exitstatus
expect(out).
to include("The checksum for the downloaded `rack-1.0.0.gem` did not match the checksum given by the API.").
and include("This means that the contents of the gem appear to be different from what was uploaded, and could be an indicator of a security issue.").
and match(/\(The expected SHA256 checksum was "#{"ab" * 22}", but the checksum for the downloaded gem was ".+?"\.\)/).
and include("Bundler cannot continue installing rack (1.0.0).")
end

it "raises when the checksum is the wrong length" do
install_gemfile <<-G, :artifice => "compact_index_wrong_gem_checksum", :env => { "BUNDLER_SPEC_RACK_CHECKSUM" => "checksum!" }
source "#{source_uri}"
gem "rack"
G
expect(exitstatus).to eq(5) if exitstatus
expect(out).to include("The given checksum for rack-1.0.0 (\"checksum!\") is not a valid SHA256 hexdigest nor base64digest")
end

it "does not raise when disable_checksum_validation is set" do
bundle! "config disable_checksum_validation true"
install_gemfile! <<-G, :artifice => "compact_index_wrong_gem_checksum"
source "#{source_uri}"
gem "rack"
G
end
end
end
7 changes: 6 additions & 1 deletion spec/support/artifice/compact_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,12 @@ def gems(gem_repo = GEM_REPO)
reqs = d.requirement.requirements.map {|r| r.join(" ") }.join(", ")
CompactIndex::Dependency.new(d.name, reqs)
end
CompactIndex::GemVersion.new(spec.version.version, spec.platform.to_s, nil, nil,
checksum = begin
Digest::SHA256.file("#{GEM_REPO}/gems/#{spec.original_name}.gem").base64digest
rescue
nil
end
CompactIndex::GemVersion.new(spec.version.version, spec.platform.to_s, checksum, nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just checksum = Digest::SHA256.file("#{GEM_REPO}/gems/#{spec.original_name}.gem").base64digest rescue nil? lots of lines super far indented makes me feel gross. 😝

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the modifier rescue and rubocop agrees with me :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about checksum = gem_checksum(spec) then?

deps, spec.required_ruby_version, spec.required_rubygems_version)
end
CompactIndex::Gem.new(name, gem_versions)
Expand Down
19 changes: 19 additions & 0 deletions spec/support/artifice/compact_index_wrong_gem_checksum.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true
require File.expand_path("../compact_index", __FILE__)

Artifice.deactivate

class CompactIndexWrongGemChecksum < CompactIndexAPI
get "/info/:name" do
etag_response do
name = params[:name]
gem = gems.find {|g| g.name == name }
checksum = ENV.fetch("BUNDLER_SPEC_#{name.upcase}_CHECKSUM") { "ab" * 22 }
versions = gem ? gem.versions : []
versions.each {|v| v.checksum = checksum }
CompactIndex.info(versions)
end
end
end

Artifice.activate_with(CompactIndexWrongGemChecksum)