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
Fixes #32753 - Remote code execution through Sendmail #8599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought: does it make sense to instead limit it to some absolute path where we've verified that the file exists on disk in $PATH or a limited number of them? (/usr/bin, /usr/sbin, /usr/local/bin, etc). Or does that not mitigate the vulnerability.
That is dangerous, there are programs like |
|
Related security site entry: theforeman/theforeman.org#1845 |
|
That's a good point. It also made me think about other possible problems with this. For example, can In other words: is there an option to say "attach Right now I can't find such an option on the sendmail command itself. |
Yeah that is really good concern. From what I was able to research, sendmail, the original, does not support attaching files directly. You have to prepare the MIME content, so you need bash at least, which we ruled out with escaping. However for example "mailx" sendmail replacement does have I have briefly reviewed all man pages of this software and I see no signs of "attach" word in their arguments. However Debian users will have a greater pool of software and chances are there might be something. Also on Debian there is no SELinux so attaching any file that is readable for foreman user would work. I suggest the following - I think this patch is a good response to the problem that can be backported. I still like your idea to moving back to settings.yaml, would you mind doing a patch in puppet changing defaults in a way that this is deployed by default and it overrides the UI? In the UI users will see a nice message "this setting is read only, go to settings.yaml to edit it" so it is actually not bad user experience. Once we have a patch for puppet, we can decide if to backport it or not, I think we should be fine just merging it into develop branch because the immediate threat should be resolved, assuming there are no other sendmail emulators offering direct file attaching. |
I did the same thing and came to the same conclusion. Just thinking out loud: how does it work if you put a setting in As a test case:
|
|
Invalid entries in settings file are not subject of Rails model validations. Editing of them is not allowed in UI or API, not a problem. |
|
Fixed migration, force pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about the various alternatives and I think this now makes sense. My main worry was a lack of flexibility but if users really need that then settings.yaml is a better place.
We should add an upgrade warning to the manual that if users used a value different than an allowed location will no longer be able to send email as they used to.
@tbrisker could you also take a look?
| @@ -19,19 +26,29 @@ def self.default_settings | |||
| set('smtp_password', N_("Password to use to authenticate, if required"), '', N_('SMTP password'), nil, {:encrypted => true}), | |||
| set('smtp_authentication', N_("Specify authentication type, if required"), '', N_('SMTP authentication'), nil, { :collection => proc { {:plain => "plain", :login => "login", :cram_md5 => "cram_md5", '' => _("none")} }}), | |||
| set('sendmail_arguments', N_("Specify additional options to sendmail"), '-i', N_('Sendmail arguments')), | |||
| set('sendmail_location', N_("The location of the sendmail executable"), "/usr/sbin/sendmail", N_('Sendmail location')), | |||
| set('sendmail_location', N_("The location of the sendmail executable"), "/usr/sbin/sendmail", N_('Sendmail location'), nil, { :collection => proc { sendmail_locations_hash } }), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It surprises me that this needs to be a proc given that it's static. There are also no translations involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully something we could get rid off with the settings refactoring (cc: @ezr-ondrej). Should we modify the description to suggest using settings.yaml to use options not allowed by default?
| @@ -0,0 +1,30 @@ | |||
| class FakeSetting < ApplicationRecord | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this fake class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copy-pasted it honestly, others migrations do have it.
Edit: I don't know :-)
Edit 2: I do not want to know :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed in this case, lets let the setting model do it's thing (which is only to reset to default if the value is invalid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if you say so, dropped, amended.
| @@ -19,19 +26,29 @@ def self.default_settings | |||
| set('smtp_password', N_("Password to use to authenticate, if required"), '', N_('SMTP password'), nil, {:encrypted => true}), | |||
| set('smtp_authentication', N_("Specify authentication type, if required"), '', N_('SMTP authentication'), nil, { :collection => proc { {:plain => "plain", :login => "login", :cram_md5 => "cram_md5", '' => _("none")} }}), | |||
| set('sendmail_arguments', N_("Specify additional options to sendmail"), '-i', N_('Sendmail arguments')), | |||
| set('sendmail_location', N_("The location of the sendmail executable"), "/usr/sbin/sendmail", N_('Sendmail location')), | |||
| set('sendmail_location', N_("The location of the sendmail executable"), "/usr/sbin/sendmail", N_('Sendmail location'), nil, { :collection => proc { sendmail_locations_hash } }), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully something we could get rid off with the settings refactoring (cc: @ezr-ondrej). Should we modify the description to suggest using settings.yaml to use options not allowed by default?
| def self.delivery_settings | ||
| options = {} | ||
| all.find_each do |setting| | ||
| extracted = {:smtp => extract_prefix(setting.name, 'smtp'), :sendmail => extract_prefix(setting.name, 'sendmail')} | ||
| ["smtp", "sendmail"].each do |method| | ||
| if Setting[:delivery_method].to_s == method && setting.name.start_with?(method) && setting.value.to_s.present? | ||
| options[extracted[method.to_sym]] = setting.value | ||
| if setting.name == "sendmail_arguments" | ||
| options[extracted[method.to_sym]] = Shellwords.shellescape(setting.value) | ||
| else | ||
| options[extracted[method.to_sym]] = setting.value | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should all move to application mailer and be greatly simplified, but outside the scope of this PR
| Setting.without_auditing do | ||
| existing = FakeSetting.find_by_name("sendmail_location") | ||
| if existing && !Setting::Email::SENDMAIL_LOCATIONS.include?(existing.value) | ||
| say "Sendmail location '#{existing.value}' not allowed, resetting to default" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, suggest using settings.yaml if needed to use non-default options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this.
|
Docs update: theforeman/foreman-documentation#563 I will do upgrade note. |
|
There is probably no better candidate for complete overhaul than our settings framework, no discussion there. Not the right place and time tho :-) So unless you have other suggestions I would push the following change: diff --git a/app/models/setting/email.rb b/app/models/setting/email.rb
index a261e6a91..98cb632a7 100644
--- a/app/models/setting/email.rb
+++ b/app/models/setting/email.rb
@@ -34,7 +34,7 @@ class Setting::Email < Setting
def validate_sendmail_location(record)
if record.value.present? && !SENDMAIL_LOCATIONS.include?(record.value)
- record.errors[:base] << _("Not a valid sendmail location")
+ record.errors[:base] << _("Not a valid sendmail location, use settings.yaml for arbitrary location")
end
end |
|
Can |
Yes: |
|
I have tested this, the UI renders a warning message with info bubble and setting is read only. 🤷🏻 |
|
Amended the message change. |
|
Upgrade note: Foreman no longer accepts sendmail location other than |
Besides |
|
Updated the comment, @upadhyeammit let me know where you want to put it or feel free to pull it into the release notes from here. Tests are green, thanks all. I will create a RM issue for puppet, no need to rush with that one. |
|
For the record: https://projects.theforeman.org/issues/32827 |
app/models/setting/email.rb
Outdated
| ] | ||
| end | ||
|
|
||
| validates :value, :length => {:maximum => 255}, :if => proc { |s| s.name == "email_subject_prefix" } | ||
|
|
||
| def validate_sendmail_location(record) | ||
| if record.value.present? && !SENDMAIL_LOCATIONS.include?(record.value) | ||
| record.errors[:base] << _("Not a valid sendmail location, use settings.yaml for arbitrary location") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| record.errors[:base] << _("Not a valid sendmail location, use settings.yaml for arbitrary location") | |
| record.errors[:base] << _("Invalid sendmail location, use settings.yaml for arbitrary location") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no poor CI, amended :-)
.rubocop_todo.yml
Outdated
| @@ -596,6 +596,7 @@ Security/YAMLLoad: | |||
| - 'db/migrate/20140219183343_migrate_permissions.rb' | |||
| - 'db/migrate/20150312144232_migrate_websockets_setting.rb' | |||
| - 'db/migrate/20190801143210_convert_dns_conflict_timeout_setting.rb' | |||
| - 'db/migrate/20210610131920_restrict_sendmail_location.rb' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased.
CVE-2021-3584: Sendmail location and arguments, available via Administer - Settings, both accept arbitrary strings and pass them into shell. By default, only Foreman super administrator can access settings. Mitigation: Verify the both settings and remove edit_settings permissions to all roles and users until fixed. Alternatively, create settings named sendmail_location and sendmail_arguments in settings.yaml file to override the UI and make the values read-only. Solution: Limit the possible values for location to just expected paths. Use shellescaping for arguments as there is currently no way to pass arguments to the 'mail' gem in a safely manner.
|
It is green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lzap !
CVE-2021-3584: Sendmail location and arguments, available via Administer - Settings, both accept arbitrary strings and pass them into shell. By default, only Foreman super administrator can access settings.
Mitigation: Verify the both settings and remove edit_settings permissions to all roles and users until fixed. Alternatively, create settings named sendmail_location and sendmail_arguments in settings.yaml file to override the UI and make the values read-only.
Solution: Limit the possible values for location to just expected paths. Use shellescaping for arguments as there is currently no way to pass arguments to the 'mail' gem in a safely manner.