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

Need patch for OSVDB-131677 against 2.5.x #944

Closed
mattolson opened this issue Dec 18, 2015 · 7 comments
Closed

Need patch for OSVDB-131677 against 2.5.x #944

mattolson opened this issue Dec 18, 2015 · 7 comments

Comments

@mattolson
Copy link

A security advisory was issued for the mail gem recently: https://github.com/rubysec/ruby-advisory-db/blob/master/gems/mail/OSVDB-131677.yml

It indicates that the vulnerability was fixed in 2.6.0. However, actionmailer 3.2 (part of rails 3.2) has a dependency on ~> 2.5.4. See https://github.com/rails/rails/blob/3-2-stable/actionmailer/actionmailer.gemspec#L23

According to rails/rails#22631, the rails project is unwilling to bump the version to 2.6.0.

How hard would it be to backport that fix to 2.5 and cut a new release?

@cswilliams
Copy link

+1, would also be great to have a 2.4.x backport as well for rails 3.1.

@wktk
Copy link

wktk commented Jan 7, 2016

Here's a monkey patch.

require 'mail'

if Gem::Version.new(Mail::VERSION.version) < Gem::Version.new('2.6')
  Mail::Field.class_exec do
    original_method = instance_method(:create_field)
    define_method(:create_field) do |name, value, charset|
      value = value.gsub(/[\r\n \t]+/m, ' ') if value.is_a?(String)
      original_method.bind(self).call(name, value, charset)
    end
  end
end

@Davidslv
Copy link

Davidslv commented Feb 5, 2016

Is there anything preventing this from being fixed and released?

@sgerrand
Copy link

sgerrand commented Mar 1, 2016

👋 @jeremy: Are there any blockers to #949 and #950 being merged into their relevant version branch targets? I'm interested in getting this applied to the 2.5-stable branch.

aleksanb added a commit to Samfundet/Samfundet that referenced this issue Mar 15, 2016
This adresses mikel/mail#944 until we can
upgrade to rails 4.x
@jeremy
Copy link
Collaborator

jeremy commented Apr 6, 2016

This is backporting a change that's not meant to fix the underlying vuln (with input validation on net/smtp) but happens to mask it with a coincidental behavior change. It was called out the sec advisory only because it bisected as the fix.

We can introduce stronger input validation at the mail handling layer and backport it without breaking compat, but 1. this isn't that change and 2. it won't conclusively address the original vuln, SMTP injection via email address.

Will keep this issue open to track resolution.

@GUI
Copy link

GUI commented Jun 14, 2016

@jeremy: Could you elaborate on what other input validations would be needed to address the vulnerability in older versions of the mail gem? I'd be happy to try and lend a hand getting things back-ported, since I'd also find this very useful in dealing with older Rails version

In reading the whitepaper, it seems like stripping the line break characters (as @wktk's original patches did) would largely mitigate the primary issue, or am I missing something else? The one other item I see is "CRLF-less attack" in section 4.2, which seems like it could be mitigated by enforcing length limits on the input.

Are there other issues you're aware of that would need to be addressed in the older mail versions? They do recommend validating for compliance with RFC 5321, but I'm not sure that's strictly necessary as long as line breaks are rejected or stripped, since they also mention:

Of course, a little more relaxed rule can be an option, as a small proportion of existing and actually used addresses is known to be incompliant to the standard. Needless to say, when a relaxed rule is employed, addresses containing line-breaks must not be allowed.

@jeremy
Copy link
Collaborator

jeremy commented May 9, 2017

Fixed in #1097. Backported to 2.6.x in #1098 and 2.5.x in #1099.

@jeremy jeremy closed this as completed May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants