Safe load all marshaled data #6384
Conversation
| @@ -39,7 +39,7 @@ module Bundler | |||
| environment_preserver.replace_with_backup | |||
| SUDO_MUTEX = Thread::Mutex.new | |||
|
|
|||
| SAFE_MARSHAL_CLASSES = [Symbol, TrueClass, String, Array, Hash].freeze | |||
| SAFE_MARSHAL_CLASSES = [Symbol, TrueClass, String, Array, Hash, Gem::Version, Gem::Specification].freeze | |||
There was a problem hiding this comment.
can we add a test that covers loading these two classes?
There was a problem hiding this comment.
I'm not 100% sure it is good idea to allow those classes everywhere. Initial effort was to limit structures parsed from Dependency API. I don't see any reason why to allow Gem::Version and Gem::Specification as valid objects coming from Dependency API.
Are those needed to read gemspecs? 🤔 Are those the only valid gemspecs classes? I think at least Gem::Requirement is valid for gemspecs as well. Also I'm not sure this doesn't change across the versions.
Thinking about this, wouldn't it make sense to make special loader for gemspecs?
There was a problem hiding this comment.
I believe the idea is that we need to avoid any classes that contain a remote execution path. If we are sure Gem::Specification does not allow remote execution, I think it is ok to allow that class everywhere—if a server returns it from the Dependency API, there will definitely be an exception somewhere else in the code.
It might make sense to add a special loader just for gemspecs if that makes it easier to separate out, but personally I would be ok with one SafeMarshal loader as long as the list of allowed classes does not include any exploitable classes.
There was a problem hiding this comment.
Happy to go whatever direction is decided here
There was a problem hiding this comment.
I have no big preference. The only concern is still regarding Gem::Requirement being added. I would be really surprised see specs passing without Gem::Requirement in this list. Are we missing some code coverage? Or am I totally missing something in here and it is not needed? 🤔
There was a problem hiding this comment.
I do have some linting issues to fix, but all the tests pass without Gem::Requirement in this list. However, I'm not familiar enough with things to say whether that's a b/c there is missing test coverage or whether it is not needed.
mercedesb
force-pushed
the
iss-6353/safe-load-all-marshaled-data
branch
from
8cf85fe
to
2ea2ead
Compare
deivid-rodriguez
changed the title
…ed-data Issue 6353: safe load all marshaled data (cherry picked from commit 0f3d7db)
What was the end-user or developer problem that led to this PR?
Resolves #6353.
In #6141, we landed a SafeMarshal class to deserialize data returned from the dependency API. We want to expand the use of SafeMarshal to include every place in RubyGems and Bundler where we load marshaled files, to remove the possibility of an attack by a malicious server.
What is your fix for the problem, implemented in this PR?
Use the
safe_load_marshalmethod when fetching Marshal.specs.4.8.gz and when inflating gem specifications.Made the legacy
Bundler.load_marshalmethod private so that others will be less tempted to use it directly.Make sure the following tasks are checked