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

Handle too long dates passed to DateTime.parse #1469

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rosa
Copy link
Contributor

@rosa rosa commented Dec 20, 2021

Since Ruby 3.0.3, this method raises an ArgumentError with a different message from the invalid date message, as mitigation for CVE-2021-41817.

ArgumentError: string length (150) exceeds the limit 128

This PR handles this error to return nil, just like when the date is invalid.

def with_invalid_date_time_error_handling(&block)
yield
rescue ArgumentError => e
raise unless e.message =~ /\A(invalid date|string length \(\d+\) exceeds the limit \d+)\z/
Copy link
Contributor Author

@rosa rosa Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should live in Mail::Utilities so it's used from here and from ReceivedElement.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please move it into utilities.

Copy link
Owner

@mikel mikel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall nice catch, thanks, please move to utility as you suggested

def with_invalid_date_time_error_handling(&block)
yield
rescue ArgumentError => e
raise unless e.message =~ /\A(invalid date|string length \(\d+\) exceeds the limit \d+)\z/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please move it into utilities.

@@ -27,7 +27,7 @@ def to_s(*args)
def datetime_for(received)
::DateTime.parse("#{received.date} #{received.time}")
rescue ArgumentError => e
raise e unless e.message == 'invalid date'
raise unless e.message =~ /\A(invalid date|string length \(\d+\) exceeds the limit \d+)\z/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call the utility method you will create. One place to keep this up to date makes sense.

@rosa
Copy link
Contributor Author

rosa commented Dec 6, 2022

Thanks a lot for the review, @mikel! I'll get these changes implemented this week, I hope.

Since Ruby 3.0.3, this method raises an ArgumentError
with a different message from the invalid date message,
as mitigation for CVE-2021-41817.
https://www.ruby-lang.org/en/news/2021/11/15/date-parsing-method-regexp-dos-cve-2021-41817/

Example:
  ArgumentError: string length (150) exceeds the limit 128

In this case we should just return nil, like when the date itself is
invalid.
@rosa
Copy link
Contributor Author

rosa commented Dec 8, 2022

Implemented the changes requested 😊

@zealot128
Copy link

Unfortunately, that is still an issue in 2024 with Mail 2.8.1 on 3.3.0.

That exception also breaks the POP functionality, as it crashes without option to rescue the individual mail:

mails = Mail.all delete_after_find: true

ArgumentError: string length (226) exceeds the limit 128 (ArgumentError)

          datetime = ::DateTime.parse(stripped)
                                      ^^^^^^^^
  from mail (2.8.1) lib/mail/fields/common_date_field.rb:17:in `parse'
  from mail (2.8.1) lib/mail/fields/common_date_field.rb:17:in `normalize_datetime'
  from mail (2.8.1) lib/mail/fields/common_date_field.rb:31:in `initialize'
  from mail (2.8.1) lib/mail/fields/common_field.rb:12:in `new'
  from mail (2.8.1) lib/mail/fields/common_field.rb:12:in `parse'

Any chance, this PR gets merged?
For now, we need manual intervention and delete the mail from the inbox.

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

Successfully merging this pull request may close these issues.

3 participants