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

Add advisory for SMTP injection vulnerability in mail <2.6.0 #215

Closed
reedloden opened this issue Dec 11, 2015 · 25 comments
Closed

Add advisory for SMTP injection vulnerability in mail <2.6.0 #215

reedloden opened this issue Dec 11, 2015 · 25 comments
Labels

Comments

@reedloden
Copy link
Member

Need an advisory for the mail vulnerability discussed in http://www.mbsd.jp/Whitepaper/smtpi.pdf (section 3.1)

Affects <2.6.0

@reedloden
Copy link
Member Author

@mikel / @bf4 / @jeremy -- any idea which commit fixed this issue? 2.6.0 fixed a ton of stuff. :/

@reedloden
Copy link
Member Author

I did a git bisect using a test case I received from the author, and if I'm understanding the issue correctly, I think mikel/mail@72befdc fixed it. Let me confer with the paper author to make sure I'm seeing the right thing.

@reedloden
Copy link
Member Author

He confirms it's that rev. If the unfold method is commented out, the problem returns.

@reedloden
Copy link
Member Author

The paper author has informed me "BTW, while investigating the source code of Mail, I came to think the fault might be more on Net::SMTP's side. It is difficult to say who is responsible for it, Net::SMTP, Mail or application developers (library users) though."

'mail' guys... thoughts on this? In any case, should add a test to 'mail' to ensure this doesn't pop up again. :)

@skorth
Copy link
Contributor

skorth commented Dec 11, 2015

Always learning something new, thx.

@reedloden
Copy link
Member Author

Request made to MITRE (and also posted to oss-security@) for a CVE. Also requested an ID from OSVDB. Sucks we have to wait for one of them before getting this advisory published.

@jeremy
Copy link
Contributor

jeremy commented Dec 11, 2015

First we've heard of this for the mail lib. Forwarded to security@ruby-lang.org, too.

@bf4
Copy link

bf4 commented Dec 15, 2015

@reedloden how the heck did you find this? Do you know if the author tried to contact anyone before publishing?

@reedloden
Copy link
Member Author

@bf4 The paper was posted on r/netsec. It was indirectly mentioned in an advisory for PHPMailer as well.

I'm in contact with the author, so I will ask him, but I suspect not for Ruby at least, as the issue had been fixed already (though not on purpose, it seems).

reedloden added a commit to reedloden/ruby-advisory-db that referenced this issue Dec 16, 2015
@reedloden
Copy link
Member Author

It was originally posted to the WebAppSec mailing list on 2015-12-09.

reedloden added a commit to reedloden/ruby-advisory-db that referenced this issue Dec 16, 2015
@jeremy
Copy link
Contributor

jeremy commented Apr 6, 2016

The mail 2.6.x fix is a coincidence. Even if it was intentional, it wouldn't be a sufficient fix for the underlying SMTP injection vuln. The referenced paper discusses this a bit: crafting otherwise-legal FWS to inject & exploit specific MTAs.

Furthermore, the fix is a side effect of incorrect behavior. Fixing that behavior in the mail lib would inadvertently re-expose the underlying SMTPi vuln that had been coincidentally masked. No good.

To rule out this risk, we need input validation in stdlib net/smtp.

@reedloden
Copy link
Member Author

@jeremy

Is there a bug filed with ruby upstream to add that validation so that this
doesn't reoccur?

On Tuesday, April 5, 2016, Jeremy Daer notifications@github.com wrote:

The mail 2.6.x fix is a coincidence. Even if it was intentional, it
wouldn't be a sufficient fix for the underlying SMTP injection vuln. The
referenced paper discusses this a bit: crafting otherwise-legal FWS to
inject & exploit specific MTAs.

Furthermore, the fix is a side effect of incorrect behavior. Fixing that
behavior in the mail lib would inadvertently re-expose the underlying SMTPi
vuln that had been coincidentally masked. No good.

To rule out this risk, we need input validation in stdlib net/smtp.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#215 (comment)

@jeremy
Copy link
Contributor

jeremy commented Apr 6, 2016

@reedloden Yep, filed 2015-12-11.

@reedloden
Copy link
Member Author

@jeremy Link to bug report just so I can track it?

@jeremy
Copy link
Contributor

jeremy commented Apr 6, 2016

@reedloden not public; reported to security@ruby-lang.org.

@jeremy
Copy link
Contributor

jeremy commented May 8, 2017

HackerOne report and fix: https://hackerone.com/reports/137631

Resolved in Ruby 2.4.0 release by ruby/ruby@0827a7e

@jeremy
Copy link
Contributor

jeremy commented May 9, 2017

2.5.5.rc1 and 2.6.6.rc1 gems released with SMTP address validation: mikel/mail#1097

@phillmv
Copy link
Member

phillmv commented Jun 9, 2017

@jeremy hey!

Do we need to update anything re: https://github.com/rubysec/ruby-advisory-db/blob/master/gems/mail/OSVDB-131677.yml ? I'm thinking re: the patched or fixed versions.

(Aside from adding better context of course. We could add links, and or move off OSVDB if there's a CVE for it…)

Thanks!

@phillmv phillmv reopened this Jun 9, 2017
@jeremy
Copy link
Contributor

jeremy commented Jun 10, 2017

Hey @phillmv!

Could just bump patched_versions to >= 2.5.5, but the reality is a bit trickier.

Vulnerable:

  • mail < 2.5.5 users on Ruby < 2.4.0
  • all net/smtp users on Ruby < 2.4.0

Non vulnerable:

  • mail >= 2.5.5 users on Ruby < 2.4.0
  • all net/smtp users on Ruby >= 2.4.0

Not sure how this best maps to advisories.

@jeremy
Copy link
Contributor

jeremy commented Jun 12, 2017

We should split this into two advisories: one for the Mail gem not doing output validation and another for Ruby SMTP allowing CRLF injection. I requested a CVE for the Ruby vuln. @reedloden requested back in 2015 as well, so we may have a dupe or that may apply to the Mail lib only.

@phillmv
Copy link
Member

phillmv commented Jun 12, 2017

I'm fine with the most basic "your ass is covered" version range, and thank you for confirming.

I approved the PR and added some useful context: 01c549e

If you think it's more accurate to have a 2nd advisory that's fine by me.

@jeremy
Copy link
Contributor

jeremy commented Jun 12, 2017

PR for Ruby vuln, pending CVE assignment: #293

@jeremy
Copy link
Contributor

jeremy commented Jun 12, 2017

CVE-2015-9096 (Ruby) and CVE-2015-9097 (Mail) assigned. #293 updated.

@jeremy
Copy link
Contributor

jeremy commented Jun 13, 2017

#292 and #293 merged, cool to close this up!

@reedloden
Copy link
Member Author

Woot! Thanks!

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

No branches or pull requests

5 participants