Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails 4.2.11.2: uninitialized constant Module::DELEGATION_RESERVED_METHOD_NAMES #39301

Closed
bubaflub opened this issue May 15, 2020 · 25 comments
Closed

Comments

@bubaflub
Copy link

@bubaflub bubaflub commented May 15, 2020

Steps to reproduce

  1. Upgraded an existing rails app from 4.2.11.1 to 4.2.11.2.
  2. Started a local dev server and tried to access localhost:3000
  3. Receive 500 Internal Server Error

Logs show:

ActionView::Template::Error (uninitialized constant Module::DELEGATION_RESERVED_METHOD_NAMES):
  /Users/bob/bubaflub/truelink/app/controllers/application_controller.rb:597:in `redacted'
  /Users/bob/bubaflub/truelink/lib/redacted_middleware.rb:13:in `redacted'

Expected behavior

Shouldn't break an existing app.

Actual behavior

Breaks an existing app.

System configuration

Rails version: 4.2.11.2

Ruby version: 2.4.10 via rvm on Mac OS X 10.15.3

@bubaflub
Copy link
Author

@bubaflub bubaflub commented May 15, 2020

I believe the problem is that Module::DELEGATION_RESERVED_METHOD_NAMES only exists in activesupport version 5 and later:

DELEGATION_RESERVED_METHOD_NAMES = Set.new(
RUBY_RESERVED_KEYWORDS + DELEGATION_RESERVED_KEYWORDS
).freeze

Whereas activesupport version 4 doesn't have that method:

RUBY_RESERVED_WORDS = Set.new(
%w(alias and BEGIN begin break case class def defined? do else elsif END
end ensure false for if in module next nil not or redo rescue retry
return self super then true undef unless until when while yield)
).freeze

@dustym
Copy link

@dustym dustym commented May 15, 2020

I have a PR up for this in #39302.

@bubaflub
Copy link
Author

@bubaflub bubaflub commented May 15, 2020

I'm curious to know how this passed through review and automated testing to make it to an official release.

@rbliss
Copy link

@rbliss rbliss commented May 15, 2020

Looks like the release never passed the build in the first place. Which is also impacting PR #39302 @dustym submitted. The build for 4.2.x appears to need updating for lower versions of Ruby.

@tenderlove
Copy link
Member

@tenderlove tenderlove commented May 15, 2020

I am very sorry. I will make a release again.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 15, 2020

Please note that Rails 4.2 is not supported. We are trying to do a favor to people still using that unsupported version by releasing a new version instead of just sending an advisory with a patch. The Rails 4.2 build is not passing anymore given the Ruby versions supported today are different of the versions supported on that series, so we can't expect the build will pass when we make a security release. That is how this passed through review and automated testing to make in to an unsupported official release.

@bubaflub
Copy link
Author

@bubaflub bubaflub commented May 15, 2020

I am very sorry. I will make a release again.

It's alright, mistakes happen! I'm was surprised that Rails 4.2 which is EOL received a security patch. And then I was surprised that it didn't work out of the box. I hope I didn't come off as blaming, I just wanted to see if there were process or automation fixes that should go in as well.

Looks like the release never passed the build in the first place... The build for 4.2.x appears to need updating for lower versions of Ruby.

As someone who is currently running Rails 4.2 + Ruby 2.4 I'd be happy to help get the build green again. I would just need some pointers in the right direction.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 15, 2020

As someone who is currently running Rails 4.2 + Ruby 2.4 I'd be happy to help get the build green again. I would just need some pointers in the right direction.

Thank you for the offer but Rails 4.2 is not supported anymore and even just reviewing changes to the test suite would still require time and energy from the core team on a version that is not supported.

@dustym
Copy link

@dustym dustym commented May 15, 2020

Thanks, @tenderlove!

@tenderlove
Copy link
Member

@tenderlove tenderlove commented May 15, 2020

I shipped 4.2.11.3 with #39302. Thanks for reporting this as well as the PR to fix it.

@VVD
Copy link

@VVD VVD commented Jun 1, 2020

This commit 4c46a15 broke redmine 3.4 (/my/page):

--- rails-4.2.11.1/actionview/lib/action_view/template.rb       2019-03-11 21:21:15.000000000 +0300
+++ rails-4.2.11.3/actionview/lib/action_view/template.rb       2020-05-15 21:34:31.000000000 +0300
@@ -312,8 +312,12 @@
       end
 
       def locals_code #:nodoc:
+        # Only locals with valid variable names get set directly. Others will
+        # still be available in local_assigns.
+        locals = @locals.to_set - Module::DELEGATION_RESERVED_METHOD_NAMES
+        locals = locals.grep(/\A(?![A-Z0-9])(?:[[:alnum:]_]|[^\0-\177])+\z/)
         # Double assign to suppress the dreaded 'assigned but unused variable' warning
-        @locals.each_with_object('') { |key, code| code << "#{key} = #{key} = local_assigns[:#{key}];" }
+        locals.each_with_object('') { |key, code| code << "#{key} = #{key} = local_assigns[:#{key}];" }
       end
 
       def method_name #:nodoc:

production.log:

Started GET "/my/page" for IP at 2020-06-01 14:51:53 +0300
Processing by MyController#page as HTML
  Current user: VVD (id=XXXX)
  Rendered my/blocks/_issues.erb (37.8ms)
  Rendered my/page.html.erb within layouts/base (5375.3ms)
Completed 500 Internal Server Error in 5490ms (ActiveRecord: 541.4ms)

ActionView::Template::Error (undefined local variable or method `block' for #<#<Class:0x0000000817a16200>:0x0000000817a2e300>):
    1: <div class="contextual">
    2:   <%= link_to_function l(:label_options), "$('##{block}-settings').toggle();", :class => 'icon-only icon-settings', :titl
e => l(:label_options) %>
    3: </div>
    4: 
    5: <h3>
  app/views/my/blocks/_issues.erb:2:in `_app_views_my_blocks__issues_erb___1895067040708881672_17382521500'
  app/helpers/my_helper.rb:103:in `render_issuesassignedtome_block'
  app/helpers/my_helper.rb:64:in `render_block_content'
  app/helpers/my_helper.rb:35:in `render_block'
  app/helpers/my_helper.rb:27:in `block in render_blocks'
  app/helpers/my_helper.rb:26:in `each'
  app/helpers/my_helper.rb:26:in `render_blocks'
  app/views/my/page.html.erb:13:in `block in _app_views_my_page_html_erb__4123894328546915933_17378179780'
  app/views/my/page.html.erb:11:in `each'
  app/views/my/page.html.erb:11:in `_app_views_my_page_html_erb__4123894328546915933_17378179780'
  lib/redmine/sudo_mode.rb:63:in `sudo_mode'
  plugins/redmine_dmsf/lib/redmine_dmsf/webdav/custom_middleware.rb:68:in `call'

After manual revert this patch error gone.

@VVD
Copy link

@VVD VVD commented Jun 15, 2020

?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 16, 2020

Yes, it was expected to break this usage. Redmine need to be updated.

@VVD
Copy link

@VVD VVD commented Jun 16, 2020

It's old and unsupported already version of Redmine 3.4 - last version from branch 3.x, which use rails 4. Don't think somebody will fix it now.
So this change just break compatibility and didn't fixed any issue/bug.

@Beuc
Copy link

@Beuc Beuc commented Jun 24, 2020

Hi,

I'm part of the Debian LTS Security Team and I'm looking into fixing CVE-2020-8163 for Debian Jessie and Debian Stretch -- preferably without breaking existing apps :)

Using a local named "block" appears to be the root cause of the regression here.

I see at d9ff835

         # still be available in local_assigns.
-        locals = @locals.to_set - Module::DELEGATION_RESERVED_METHOD_NAMES.dup.delete("block")
+        locals = @locals - Module::RUBY_RESERVED_KEYWORDS
         locals = locals.grep(/\A(?![A-Z0-9])(?:[[:alnum:]_]|[^\0-\177])+\z/)

that the current/master rails checks use RUBY_RESERVED_KEYWORDS (which doesn't include "block") rather than DELEGATION_RESERVED_METHOD_NAMES.
In addition, the previous version explicitly made an exception for "block".
Also, the latest redmine version still uses "block" as a locals name, suggesting that redmine 3.4 needn't be updated.
https://github.com/redmine/redmine/blob/986d487fcd18e6ba49e867e905559b575f003bd2/app/helpers/my_helper.rb#L103-L105

    render :partial => 'my/blocks/issues', :locals => {:query => query, :issues => issues, :block => block}

Would it make sense to align this fix with later rails releases?

(Incidentally I'm not sure how setting local names leads to RCE, so pointers about attacks we want to block would be greatly appreciated, possibly in PM :))

@Beuc
Copy link

@Beuc Beuc commented Jul 4, 2020

FWIW, here are the Debian patches:

4.2.7:

--- rails-4.2.7.1.orig/actionview/lib/action_view/template.rb
+++ rails-4.2.7.1/actionview/lib/action_view/template.rb
@@ -312,8 +312,12 @@ module ActionView
       end
 
       def locals_code #:nodoc:
+        # Only locals with valid variable names get set directly. Others will
+        # still be available in local_assigns.
+        locals = @locals.to_set - Module::RUBY_RESERVED_WORDS
+        locals = locals.grep(/\A(?![A-Z0-9])(?:[[:alnum:]_]|[^\0-\177])+\z/)
         # Double assign to suppress the dreaded 'assigned but unused variable' warning
-        @locals.each_with_object('') { |key, code| code << "#{key} = #{key} = local_assigns[:#{key}];" }
+        locals.each_with_object('') { |key, code| code << "#{key} = #{key} = local_assigns[:#{key}];" }
       end
 
       def method_name #:nodoc:

4.1.8:

--- rails-4.1.8.orig/actionview/lib/action_view/template.rb
+++ rails-4.1.8/actionview/lib/action_view/template.rb
@@ -322,8 +322,12 @@ module ActionView
       end
 
       def locals_code #:nodoc:
+        # Only locals with valid variable names get set directly. Others will
+        # still be available in local_assigns.
+        locals = @locals.to_set - Module::RUBY_RESERVED_WORDS
+        locals = locals.grep(/\A(?![A-Z0-9])(?:[[:alnum:]_]|[^\0-\177])+\z/)
         # Double assign to suppress the dreaded 'assigned but unused variable' warning
-        @locals.map { |key| "#{key} = #{key} = local_assigns[:#{key}];" }.join
+        locals.map { |key| "#{key} = #{key} = local_assigns[:#{key}];" }.join
       end
 
       def method_name #:nodoc:
--- rails-4.1.8.orig/activesupport/lib/active_support/core_ext/module/delegation.rb
+++ rails-4.1.8/activesupport/lib/active_support/core_ext/module/delegation.rb
@@ -1,8 +1,16 @@
+require 'set'
+
 class Module
   # Error generated by +delegate+ when a method is called on +nil+ and +allow_nil+
   # option is not used.
   class DelegationError < NoMethodError; end
 
+  RUBY_RESERVED_WORDS = Set.new(
+    %w(alias and BEGIN begin break case class def defined? do else elsif END
+       end ensure false for if in module next nil not or redo rescue retry
+       return self super then true undef unless until when while yield)
+  ).freeze
+
   # Provides a +delegate+ class method to easily expose contained objects'
   # public methods as your own.
   #

@VVD
Copy link

@VVD VVD commented Jul 5, 2020

@Beuc, these patches break backward compatibility.

@Beuc
Copy link

@Beuc Beuc commented Jul 5, 2020

Hi. What legitimate use case do you think is impacted, exactly?

@VVD
Copy link

@VVD VVD commented Jul 5, 2020

@Beuc, read this comment: #39301 (comment)

@Beuc
Copy link

@Beuc Beuc commented Jul 5, 2020

@VVD do read my comment as well, my patches are different and typically do not break redmine.
If you find a new issue please include full new logs.

@a17levine
Copy link

@a17levine a17levine commented Jul 8, 2020

@VVD Also having this error in production when accessing block. Please keep me in the loop if you create a new issue or submit a patch

Beuc added a commit to Beuc/rails that referenced this issue Jul 8, 2020
Allow again the following non-reserved local names: _ arg args block
Closes: rails#39301
Beuc added a commit to Beuc/rails that referenced this issue Jul 8, 2020
Allow again the following non-reserved local names: _ arg args block
Closes: rails#39301
@Beuc
Copy link

@Beuc Beuc commented Jul 8, 2020

@VVD @a17levine for clarity I created: #39806
It's designed to fix CVE-2020-8163 while preserving block.
Would you mind testing ?

@ others (e.g. with hackerone access), would you mind validating security?

Beuc added a commit to Beuc/rails that referenced this issue Jul 8, 2020
Allow again the following non-reserved local names: _ arg args block
Closes: rails#39301
Beuc added a commit to Beuc/rails that referenced this issue Jul 8, 2020
Allow again the following non-reserved local names: _ arg args block
Closes: rails#39301
@VVD
Copy link

@VVD VVD commented Jul 11, 2020

@Beuc, not any more actual for me - just updated Redmine to 4.1.1 and it uses rails 5.2.

@abepetrillo
Copy link

@abepetrillo abepetrillo commented Sep 2, 2020

This is still breaking existing apps: #40161

@a17levine
Copy link

@a17levine a17levine commented Sep 2, 2020

This is still breaking existing apps: #40161

Did you source 4.2.11.3 from the 4.2 stable branch on Github? The Rubygems one has a regression I believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants