Replace `Marshal.load` with a fully-checked safe gemspec loader #6896

segiddins · 2023-08-18T20:37:35Z

What was the end-user or developer problem that led to this PR?

Ruby has historically had many security issues while loading serialized YAML or Marshal data, with the worst outcome being remote code execution. RubyGems and Bundler both load gemspecs from gem source URLs provided by users. To prevent exploits by malicious gem servers against developer machines installing gems, we need to load only a safe subset of Ruby classes and objects.

Previously, we implemented YAML.safe_load to prevent exploits by malicious gem servers via crafted YAML files. Until now, we have not been able to provide the same defenses for marshaled gemspecs due to the lack of a Marshal.safe_load.

This PR implements Marshal.safe_load, a loader for marshaled data that excludes unknown objects and classes. The loader prevents exploits by malicious gem servers, or crafted marshaled data provided to RubyGems or Bundler.

What is your fix for the problem, implemented in this PR?

Implement a new reader / AST visitor for Marshal documents, following https://rubyreferences.github.io/rubyref/builtin/marshal.html and using the Psych ToRuby visitor for inspiration.

As a followup, will conditionally use the new module in Bundler

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

indirect

Thanks for doing this! Ensuring that we can load marshaled specs safely has been on our list of good ideas that seem hard for several years. 🎉

test/rubygems/test_gem_safe_marshal.rb

…tted classes/symbols

Tested with: `ruby -e 'trap("INT") { exit 1 }; TZ=%w[UTC +0000 -0000]; RUBY=%w[ruby-2.7 ruby-3.2.2 jruby-9.4 truffleruby-22 truffleruby-23]; TZ.product(RUBY).each { |t, r| puts ?**120, "TZ=#{t} RUBY=#{r}", "*"*120; system({"TZ"=>t,"RUBY"=>r}, *ARGV) }' zsh -lic 'chruby $RUBY; ruby -vw -Ilib test/rubygems/test_gem_safe_marshal.rb --verbose=progress'`

segiddins · 2023-08-24T20:35:39Z

Specs here finally passed! @deivid-rodriguez if you want to take a look

indirect · 2023-08-24T21:34:26Z

Updated the PR title and description with an explanation of the problem and how this PR solves the problem. 👍🏻

simi · 2023-08-29T19:16:02Z

lib/rubygems/safe_marshal/reader.rb

+
+      private
+
+      MARSHAL_VERSION = [Marshal::MAJOR_VERSION, Marshal::MINOR_VERSION].map(&:chr).join.freeze


If I understand it well, this reader implements Marshal 4.8. Would it make sense to hardcode this value in here instead of following original Marshal version? Latest Marshal minor update happened 21 years ago at ruby/ruby@73a4724, but in any case marshal is being updated, there is no guarantee this reader will work as intended.

simi · 2023-08-29T19:33:21Z

test/rubygems/test_gem_safe_marshal.rb

+    assert_safe_load_marshal(dumped, equality: equality, **kwargs)
+  end
+
+  def with_const(mod, name, new_value, &block)


This is used to temporarily modify PERMITTED_IVARS of Gem::SafeMarshal, since that is used in safe_load method. Wouldn't be easier using Gem::SafeMarshal.load with permitted_ivars attribute for such a purpose directly?

simi · 2023-08-29T19:34:30Z

test/rubygems/test_gem_safe_marshal.rb

+    2**16, 2**16 - 1, 2**20 - 1,
+    2**28, 2**28 - 1,
+    2**32, 2**32 - 1,
+    2**63, 2**63 - 1


Is there any specific reason for those numbers? Maybe comment would be handy.

simi

This is HUUUUUGE! Big applause for reporting found issues in JRuby and Truffleruby, Truffleruby one seems already fixed 💪 (unreleased).

I can imagine packing this as separate gem at some point and vendoring back using Automatiek, since it can be super useful outside of RubyGems/Bundler.

Is there any plan on testing this on real gems in bulk?

segiddins · 2023-08-29T19:40:38Z

I tested it on every gemspec in rubygems.org on ruby 2.7, 3.2, jruby 9.4, and truffleruby 23. only a few gemspecs could not load because they used some random symbols, all were very old (plus another few that can't properly be downloaded)

simi · 2023-08-29T19:43:14Z

I tested it on every gemspec in rubygems.org on ruby 2.7, 3.2, jruby 9.4, and truffleruby 23. only a few gemspecs could not load because they used some random symbols, all were very old (plus another few that can't properly be downloaded)

I was thinking to test this against top 100/1000 gems by download (all versions, to include also old releases) in bulk somehow.

indirect · 2023-08-29T23:25:08Z

@simi It seems like a lot of work to dynamically calculate the most popular gems and download their gemspecs in a test... since we have tested against existing gemspecs, maybe we add a few existing marshaled gemspecs to the system tests, and add some code that verifies gemspecs can go through Marshal.dump/Marshal.safe_load as one of the steps during gem push?

simi · 2023-08-29T23:42:48Z

@simi It seems like a lot of work to dynamically calculate the most popular gems and download their gemspecs in a test... since we have tested against existing gemspecs, maybe we add a few existing marshaled gemspecs to the system tests, and add some code that verifies gemspecs can go through Marshal.dump/Marshal.safe_load as one of the steps during gem push?

I meant this for one off test before release to ensure everything works properly. Clearly this is not good idea to add to test suite or CI. I can craft something locally and report back.

indirect · 2023-08-29T23:44:10Z

@simi ohh ok! I think we are good then, since @segiddins has already tested every gemspec for every gem?

simi · 2023-08-29T23:47:36Z

@simi ohh ok! I think we are good then, since @segiddins has already tested every gemspec for every gem?

Uhh, I have missed that. By every gemspec in rubygems.org I understood all gems used in rubygems.org Gemfile.lock only. 🤦 All good then.

headius · 2023-08-30T06:32:12Z

I have fixed one of the issues reported. Thanks for letting us know! Obviously there's still gaps in Marshal specs and tests.

Replace Marshal.load with a fully-checked safe gemspec loader (cherry picked from commit 622bbdd)

The validate_vendored_ruby test started failing on AIX 7.2 when loading the gemspecs from artifactory: # /opt/puppetlabs/puppet/bin/gem update --system -V GET https://rubygems.org/specs.4.8.gz 304 Not Modified GET https://artifactory.delivery.puppetlabs.net/artifactory/api/gems/rubygems/specs.4.8.gz 304 Not Modified [FATAL] failed to allocate memory It appears the new behavior is triggered when using the new safe marshall code introduced in rubygems-update[1]: # truss -f -i -t kopen /opt/puppetlabs/puppet/bin/gem update --system ... 10813880: 11338039: kopen("/root/.local/share/gem/specs/rubygems.org%443/specs.4.8", 0440001401, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH |S_IWOTH) = 9 10813880: 11338039: kopen("/opt/puppetlabs/puppet/lib/ruby/site_ruby/3.2.0/rubygems/safe_marshal.rb", 0440000004) = 9 ... 10813880: 11338039: kopen("/root/.local/share/gem/specs/artifactory.delivery.puppetlabs.net%443/artifactory/api/gems/rubygems/specs.4.8" , 0440001401, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH) = 10 [FATAL] failed to allocate memory Pin back to an older rubygems-update on AIX 7.2. [1] rubygems/rubygems#6896

The validate_vendored_ruby test started failing on AIX 7.2 when loading the gemspecs from artifactory: # /opt/puppetlabs/puppet/bin/gem update --system -V GET https://rubygems.org/specs.4.8.gz 304 Not Modified GET https://artifactory.delivery.puppetlabs.net/artifactory/api/gems/rubygems/specs.4.8.gz 304 Not Modified [FATAL] failed to allocate memory It appears the new behavior is triggered when using the new safe marshall code introduced in rubygems-update[1]: # truss -f -i -t kopen /opt/puppetlabs/puppet/bin/gem update --system ... 10813880: 11338039: kopen("/root/.local/share/gem/specs/rubygems.org%443/specs.4.8", 0440001401, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH |S_IWOTH) = 9 10813880: 11338039: kopen("/opt/puppetlabs/puppet/lib/ruby/site_ruby/3.2.0/rubygems/safe_marshal.rb", 0440000004) = 9 ... 10813880: 11338039: kopen("/root/.local/share/gem/specs/artifactory.delivery.puppetlabs.net%443/artifactory/api/gems/rubygems/specs.4.8" [FATAL] failed to allocate memory Pin back to an older rubygems-update on AIX 7.2. [1] rubygems/rubygems#6896

The validate_vendored_ruby test started failing on AIX 7.2 when loading the gemspecs from artifactory: # /opt/puppetlabs/puppet/bin/gem update --system -V GET https://rubygems.org/specs.4.8.gz 304 Not Modified GET https://artifactory.delivery.puppetlabs.net/artifactory/api/gems/rubygems/specs.4.8.gz 304 Not Modified [FATAL] failed to allocate memory It appears the new behavior is triggered when using the new safe marshall code introduced in rubygems-update[1]: # truss -f -i -t kopen /opt/puppetlabs/puppet/bin/gem update --system ... 10813880: 11338039: kopen("/root/.local/share/gem/specs/rubygems.org%443/specs.4.8", 0440000002) = 9 10813880: 11338039: kopen("/opt/puppetlabs/puppet/lib/ruby/site_ruby/3.2.0/rubygems/safe_marshal.rb", 0440000004) = 9 ... 10813880: 11338039: kopen("/root/.local/share/gem/specs/artifactory.delivery.puppetlabs.net%443/artifactory/api/gems/rubygems/specs.4.8", 0440000004) = 10 [FATAL] failed to allocate memory Pin back to an older rubygems-update on AIX 7.2. [1] rubygems/rubygems#6896

segiddins requested review from indirect and deivid-rodriguez August 18, 2023 20:37

indirect approved these changes Aug 18, 2023

View reviewed changes

test/rubygems/test_gem_safe_marshal.rb Outdated Show resolved Hide resolved

segiddins added 3 commits August 20, 2023 10:42

Add a Marshal.load replacement that walks an AST to safely load permi…

7e4478f

…tted classes/symbols

Fix UTC time loading

2a4d0a4

Broader version compatibility in marshal tests

6ec518c

segiddins force-pushed the segiddins/real-marshal-safe-load branch from cee54aa to b206cba Compare August 20, 2023 17:43

Verified working on mri/jruby/truffleruby with specs on rubygems.org

4f51741

segiddins force-pushed the segiddins/real-marshal-safe-load branch from b206cba to 4f51741 Compare August 20, 2023 18:05

segiddins added 9 commits August 20, 2023 21:13

Fix bundler specs using safe_load

ccb1bb5

Use safe_load in bundler when available

34d096e

Allow bundler to load from the dependency api

3303957

Bundler error handling

63b422b

safe_load tests passing on jruby & truffleruby

a64b21b

Tests passing on truffleruby 22 in addition to 23

8065530

Ruby 2.6 compat

1a84960

Give up, load Time via Marshal.load

6c92ba2

indirect changed the title ~~Add a Marshal.load replacement that walks an AST to safely load permi…~~ Replace Marshal.load with a fully-checked safe gemspec loader Aug 24, 2023

simi self-requested a review August 24, 2023 23:18

simi reviewed Aug 29, 2023

View reviewed changes

simi approved these changes Aug 29, 2023

View reviewed changes

segiddins mentioned this pull request Aug 30, 2023

Marshal.load error for Time with ivar jruby/jruby#7902

Open

indirect merged commit 622bbdd into master Sep 20, 2023
92 checks passed

indirect deleted the segiddins/real-marshal-safe-load branch September 20, 2023 02:02

deivid-rodriguez added rubygems: security and removed rubygems: security labels Sep 21, 2023

deivid-rodriguez added the rubygems: security label Oct 13, 2023

deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023

Merge pull request #6896 from rubygems/segiddins/real-marshal-safe-load

27ed90c

Replace Marshal.load with a fully-checked safe gemspec loader (cherry picked from commit 622bbdd)

deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023

Merge pull request #6896 from rubygems/segiddins/real-marshal-safe-load

913ce1f

Replace Marshal.load with a fully-checked safe gemspec loader (cherry picked from commit 622bbdd)

deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023

Merge pull request #6896 from rubygems/segiddins/real-marshal-safe-load

b92b1df

Replace Marshal.load with a fully-checked safe gemspec loader (cherry picked from commit 622bbdd)

deivid-rodriguez removed the rubygems: security label Oct 16, 2023

deivid-rodriguez added the rubygems: security label Dec 7, 2023

deivid-rodriguez changed the title ~~Replace Marshal.load with a fully-checked safe gemspec loader~~ Replace Marshal.load with a fully-checked safe gemspec loader Dec 7, 2023

joshcooper mentioned this pull request Jan 3, 2024

Use older rubygems-update on AIX 7.2 puppetlabs/puppet-agent#2457

Merged

rubygems deleted a comment from rondales Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `Marshal.load` with a fully-checked safe gemspec loader #6896

Replace `Marshal.load` with a fully-checked safe gemspec loader #6896

segiddins commented Aug 18, 2023 •

edited by indirect

indirect left a comment

segiddins commented Aug 24, 2023

indirect commented Aug 24, 2023

simi Aug 29, 2023

simi Aug 29, 2023

simi Aug 29, 2023

simi left a comment

segiddins commented Aug 29, 2023

simi commented Aug 29, 2023

indirect commented Aug 29, 2023

simi commented Aug 29, 2023

indirect commented Aug 29, 2023

simi commented Aug 29, 2023

headius commented Aug 30, 2023


		private

		MARSHAL_VERSION = [Marshal::MAJOR_VERSION, Marshal::MINOR_VERSION].map(&:chr).join.freeze

Replace Marshal.load with a fully-checked safe gemspec loader #6896

Replace Marshal.load with a fully-checked safe gemspec loader #6896

Conversation

segiddins commented Aug 18, 2023 • edited by indirect

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

indirect left a comment

Choose a reason for hiding this comment

segiddins commented Aug 24, 2023

indirect commented Aug 24, 2023

simi Aug 29, 2023

Choose a reason for hiding this comment

simi Aug 29, 2023

Choose a reason for hiding this comment

simi Aug 29, 2023

Choose a reason for hiding this comment

simi left a comment

Choose a reason for hiding this comment

segiddins commented Aug 29, 2023

simi commented Aug 29, 2023

indirect commented Aug 29, 2023

simi commented Aug 29, 2023

indirect commented Aug 29, 2023

simi commented Aug 29, 2023

headius commented Aug 30, 2023

Replace `Marshal.load` with a fully-checked safe gemspec loader #6896

Replace `Marshal.load` with a fully-checked safe gemspec loader #6896

segiddins commented Aug 18, 2023 •

edited by indirect