Project

General

Profile

Feature #16131

Remove $SAFE, taint and trust

Added by naruse (Yui NARUSE) about 2 months ago. Updated 4 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:95398]

Description

Ruby had Taint checking which is originally introduced in Perl.
https://en.wikipedia.org/wiki/Taint_checking

It was intended to provide a useful tool for handle objects which are come from outside.
Input data is set as tainted by default and call untaint if you checked or filtered the value.
Some people used this feature in the age of CGI.

But these days, no one use the mechanism and input libraries usually doesn't support it.
For example rack, as following shows its input is not tainted and the mechanism is unusable.

% cat foo.ru
run ->(env) do
  ['200', {'Content-Type' => 'text/plain'}, ["Is QUERY_STRING tainted?: #{env["QUERY_STRING"].tainted?}"]]
end
% rackup foo.ru
[51724] Puma starting in cluster mode...
[51724] * Version 3.12.1 (ruby 2.6.3-p62), codename: Llamas in Pajamas
[51724] * Min threads: 3, max threads: 3
[51724] * Environment: development
[51724] * Process workers: 1
[51724] * Preloading application
[51724] * Listening on tcp://localhost:9292
[51724] Use Ctrl-C to stop
[51737] + Gemfile in context: /Users/naruse/work/td-cdp-api/Gemfile
[51724] - Worker 0 (pid: 51737) booted, phase: 0
% curl http://localhost:9292/\?foo=1
Is QUERY_STRING tainted?: false

Therefore I think Taint checking mechanism is unusable on the current Ruby ecosystem.

On the other hand we experienced multiple vulnerability around $SAFE and taint mechanism.
https://cse.google.com/cse?q=taint&cx=008288045305770251182%3Afvruzsaknew&ie=UTF-8
The cost of maintaining it is expensive.

In conclusion, I think the taint mechanism is too expensive to maintain for the merit of it.
I suggest to remove it.


Related issues

Related to Ruby master - Feature #15998: Allow String#-@ to deduplicate tainted string, but return an untainted oneFeedbackActions
Related to Ruby master - Feature #8468: Remove $SAFEClosed06/01/2013Actions
Related to Ruby master - Bug #9588: program name variables taintedClosedActions

History

#1

Updated by naruse (Yui NARUSE) about 2 months ago

  • Related to Feature #15998: Allow String#-@ to deduplicate tainted string, but return an untainted one added
#2

Updated by naruse (Yui NARUSE) about 2 months ago

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

I agree with the removal of $SAFE and the taint tracking. Proposed timeline:

2.7:

  • Remove taint tracking/mechanism.
  • Non-verbose warning on setting/access of $SAFE
  • taint/trust/untaint/untrust become no-ops, verbose warning when called

3.0:

  • No warning on setting/access of $SAFE, it switches to normal global variable.

3.2:

  • taint/trust/untaint/untrust non-verbose warning when called

3.3:

  • taint/trust/untaint/untrust removed

The reasoning behind the delayed removal of the taint/trust/untaint/untrust methods is that most gems want to support all currently supported Ruby versions, and removing these methods soon may make that more difficult.

Updated by byroot (Jean Boussier) about 2 months ago

3.2 taint/trust/untaint/untrust non-verbose warning when called

Maybe you meant verbose here?

Other than that I agree with the proposed timeline, and as soon as these methods are noop, their cost become mostly null.

Making them noop also allow for easy feature testing: Object.new.taint.tainted? # => wether or not tainting is supported.

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

byroot (Jean Boussier) wrote:

3.2 taint/trust/untaint/untrust non-verbose warning when called

Maybe you meant verbose here?

No. Verbose warning means a warning only printed in verbose mode (ruby -w, or $VERBOSE = true). Non-verbose warning means a warning printed even in regular mode.

Updated by mame (Yusuke Endoh) about 2 months ago

+1 for the removal, and I agree with Jeremy's plan for 2.7 and 3.0.
For 3.2 and 3.3, I think we may keep all the methods as no-op because old not-maintained-well scripts may break, though I'm not so strongly against the removal.
(Anyway, tainted? and trusted? should be also cared.)

Updated by hsbt (Hiroshi SHIBATA) about 2 months ago

I'm also +1 for jeremy's proposal.

I often got the test fails related $SAFE on rubygems. I'm happy to leave them with this proposal.

Updated by Dan0042 (Daniel DeLorme) about 2 months ago

I must admit to using taint sometimes in my code, as a way to keep track of dirty/modified status on an object (mea culpa)

hash.taint[key] = newvalue
...
save(hash.untaint) if hash.tainted?

It's probably not common at all. Still, I think since tainted state has been there for such a long time we should not introduce backwards incompatibility (making it a no-op) right away in 2.7. Adding a deprecation warning in 2.7 and then making it a no-op in 3 should be the usual way of handling deprecation no? Although removing the interaction with $SAFE seems ok to me even for 2.7.

Updated by Dan0042 (Daniel DeLorme) about 2 months ago

jeremyevans0 (Jeremy Evans), by "no-op" did you mean only in the context of $SAFE mode, or did you mean that tainted? and trusted? would always return false? In the second case I think it's better to just remove the method, at least that's an obvious and easy bug to fix.

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

By no-op, I meant they would make no changes and return self. I didn't mention tainted? or trusted? earlier, but I think it may make sense to remove them earlier than taint/trust/untaint/untrust. Maybe a non-verbose warning stating they always return false in 2.7, and then remove them in 3.0. The reason for the different behavior is that taint/trust/untaint/untrust are often called by code without caring what they actually do (other than to make the objects work with certain core methods). tainted?/trusted? are only called when the code wants to have different behavior based on the taint flag.

For tainted?/trusted? to work correctly, we would need to continue to support taint tracking at least in some state. We could reduce the scope of the taint flag, though. For example, we could make it so the taint flag is never checked by any core/stdlib code, and never transfered to another object. However calling taint/trust/untaint/untrust on an object and then calling tainted?/trusted? on the same object will still behave as it does in 2.6. That would allow your abuse of taint for dirty tracking to continue to work in 2.7. If we do that, I think we should still add a non-verbose warning in 2.7 when tainted?/trusted? are called, and remove tainted?/trusted? in 3.0.

Updated by Dan0042 (Daniel DeLorme) about 2 months ago

jeremyevans0 (Jeremy Evans) wrote:

For tainted?/trusted? to work correctly, we would need to continue to support taint tracking at least in some state. We could reduce the scope of the taint flag, though. For example, we could make it so the taint flag is never checked by any core/stdlib code, and never transfered to another object. However calling taint/trust/untaint/untrust on an object and then calling tainted?/trusted? on the same object will still behave as it does in 2.6. That would allow your abuse of taint for dirty tracking to continue to work in 2.7. If we do that, I think we should still add a non-verbose warning in 2.7 when tainted?/trusted? are called, and remove tainted?/trusted? in 3.0.

That sounds good to me. At that point you could even replace the taint/trust bit flags by instance variables.

#12

Updated by ko1 (Koichi Sasada) about 2 months ago

  • Related to Bug #9588: program name variables tainted added

Updated by mame (Yusuke Endoh) about 2 months ago

headius (Charles Nutter) Eregon (Benoit Daloze) brixen (Brian Shirai)

Do you have any opinion about this as developers of other Ruby implementations?

Updated by Eregon (Benoit Daloze) about 2 months ago

I agree it would be best to remove the implicit taint state, and particularly the interaction with $SAFE.

FWIW, TruffleRuby already prevents setting $SAFE to anything else than 0:
https://github.com/oracle/truffleruby/blob/master/doc/user/security.md#unimplemented-security-features

Without $SAFE (which I think most people agree to remove), I think tainting has very few use-cases, which I think doesn't warrant staying a core feature.

Tracking tainting has a performance cost, e.g., String#+ must check if either LHS or RHS is tainted and taint the result in that case.
This can introduce extra polymorphism or branches in code which needs to check for the taint state.

Updated by matz (Yukihiro Matsumoto) about 1 month ago

Basically agreed.
My proposal for the schedule:

2.7:

  • Remove taint tracking/mechanism.
  • Non-verbose warning on setting/access of $SAFE
  • taint/trust/untaint/untrust become no-ops, verbose warning when called

3.0:

  • No warning on setting/access of $SAFE, it switches to normal global variable.
  • taint/trust/untaint/untrust non-verbose warning when called

3.2:

  • taint/trust/untaint/untrust removed

But it's not a big issue.

Matz.

Updated by headius (Charles Nutter) about 1 month ago

I look forward to removing all tainting logic!

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

I've added a pull request that adds warnings to setting/access of $SAFE, as well as public C function that deal with $SAFE: https://github.com/ruby/ruby/pull/2476

As the taint tracking/mechanism is being removed, I was not sure if we want to keep any other features of $SAFE. The pull request does not keep any features, after it is applied, nothing in the core or stdlib uses $SAFE. I think that is what was desired, but I'm not sure, as the log for the last developer meeting hasn't been released yet.

Updated by jeremyevans0 (Jeremy Evans) 28 days ago

I've expanded my pull request to deprecate taint/trust and related methods with verbose warnings, and make the methods no-ops. I believe this implements matz's plan for Ruby 2.7.

The changes involved removing tainting from all included libraries, which includes libraries such as rubygems, bundler, and json, that may want to support older versions of ruby upstream (and may need to keep taint code to work correctly in older ruby versions). I'm not sure how we want to handle this, and I'm open to ideas.

Updated by jeremyevans0 (Jeremy Evans) 18 days ago

I've rebased my pull request against master and fixed the conflicts (https://github.com/ruby/ruby/pull/2476). I've also removed mentions of $SAFE and taint from the documentation.

Due to the extent of the changes, I don't want to wait too long before merging this. Otherwise, there will probably be more conflicts to resolve, and increased chance of a untaint/taint call being introduced. Also due to the extent of the changes, another committer should review.

We still need to decide how we want to handle upstreams that want to support older ruby versions. Do we want to just notify upstreams and request that they fix it? Do we want to recommend a specific approach, such as (for rubygems):

if RUBY_VERSION >= '2.7'
  def Gem.untaint_obj(obj)
  end
else
  def Gem.untaint_obj(obj)
    obj.untaint
  end
end

And changing all the calls? Or wrapping all calls in if RUBY_VERSION < '2.7'

test-bundled-gems is failing with this patch (a single rake test). I submitted a patch upstream to skip that test on Ruby 2.7+: https://github.com/ruby/rake/pull/329

Updated by mame (Yusuke Endoh) 6 days ago

Hi jeremyevans0 (Jeremy Evans),

I've rebased my pull request against master and fixed the conflicts

Thank you for the great work! I've discussed this issue on the developer meeting, and all agreed with the change.

We still need to decide how we want to handle upstreams that want to support older ruby versions.

This should be discussed and agreed with the maintainers for each code (rubygems, bundler, etc). In regard to rubygems and bundler, I hear from hsbt (Hiroshi SHIBATA) that the incompatibility would not matter even if we just remove the code related to $SAFE. (hsbt (Hiroshi SHIBATA), am I correct?)

Updated by jeremyevans0 (Jeremy Evans) 5 days ago

The blocker on merging the pull request is that test-bundled-gems is failing due to the rake test failure. https://github.com/ruby/rake/pull/329 needs to be merged (and I don't have permissions to merge it), and a new rake released and bundled with Ruby.

I checked and Bundler and Rubygems are the only libraries affected that use external upstreams. All other affected libraries (default gems) are under the ruby organization on GitHub. We need to decide how we want to handle these:

Default gems without extensions

fileutils
irb
reline
rexml
rss
webrick

Default gems with extensions:

bigdecimal
date
dbm
etc
fiddle
gdbm
io-console
openssl
psych
stringio
strscan
zlib

Are we OK with just removing the calls to taint/untaint? I'm not sure, but I believe that may cause issues when using previous versions of Ruby. The simplest fix here is to set the required ruby version in the related gemspecs to 2.6.99 to allow 2.7.0 preview/beta versions and above to work. That will mean older versions of Ruby cannot install newer versions of the gems. Is that acceptable?

Updated by mame (Yusuke Endoh) 5 days ago

Are we OK with just removing the calls to taint/untaint?

Each maintainer should determine that.

This is my personal opinion: In principle, we should be conservative against incompatibility. But in regard to $SAFE, we can be flexible because it seems really rare to be used.

Anyway, I'd like to keep no warnings in CI even in verbose mode.

Updated by jeremyevans0 (Jeremy Evans) 5 days ago

mame (Yusuke Endoh) wrote:

Are we OK with just removing the calls to taint/untaint?

Each maintainer should determine that.

This is my personal opinion: In principle, we should be conservative against incompatibility. But in regard to $SAFE, we can be flexible because it seems really rare to be used.

Anyway, I'd like to keep no warnings in CI even in verbose mode.

I agree with your points. Here is my implementation plan:

  • I will submit pull requests upstream to all projects that remove the calls and bump the required ruby version to 2.6.99.

  • For upstreams without a maintainer, I will wait one week to allow input from the community, and assuming no input, I will merge the changes.

  • If the upstream has a maintainer, and the maintainer requests different behavior, I will work with them to implement their desired behavior.

  • If the upstream has a maintainer, and the maintainer doesn't respond in one month, I will merge the changes (assuming I have access to do so).

This plan should ensure that all upstreams are consulted and all maintainers can choose the path they feel is best. It should also ensure the changes can be merged in time for Ruby 2.7. Is this plan acceptable?

Updated by jeremyevans0 (Jeremy Evans) 4 days ago

I have added pull requests for all upstream projects. After some thought, I think many maintainers may consider dropping Ruby <2.7 support not acceptable. So the pull requests I submitted will continue to work on older Ruby versions. In cases where untaint is used, that means using a conditional, because the calling code may want an untainted string. In cases where taint or tainted? is used, those were generally just removed. While that does change behavior slightly, it is unlikely anyone is relying on things being tainted (they may relying on things not being tainted).

Here are links to all pull requests:

Bundled gems with external upstreams:

Default gems with external upstreams:

Default gems without C extensions:

Default gems with C extensions:

Also available in: Atom PDF