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 CVE-2020-14334 to security page #1654

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

ezr-ondrej
Copy link
Member

No description provided.

security.md Outdated
*Mitigation:* change cache directory mode `o-r`.

* Affects RPM installations
* Fix released in Foreman 2.1
Copy link
Member

Choose a reason for hiding this comment

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

the fix will be in Foreman 2.2.0, 2.1.1 and 2.0.2 and higher

security.md Outdated
#### <a id="2020-14334"></a>CVE-2020-14334: World readable cache could expose sensitive settigs
Even encrypted settings have their raw values cached. Too permissive mode on cache dir caused, that anyone with access to the hosting system could read this encrypted settings.

*Mitigation:* change cache directory mode `o-r`.
Copy link
Member

Choose a reason for hiding this comment

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

please specify which directories

security.md Outdated

*Mitigation:* change cache directory mode `o-r`.

* Affects RPM installations
Copy link
Member

Choose a reason for hiding this comment

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

since which foreman version?

Copy link
Member

Choose a reason for hiding this comment

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

It was part of theforeman/foreman-packaging@847450f which is titled Import foreman. Prior to that RPM specs were in foreman.git and was introduced in theforeman/foreman@d30ac55. So my guess is Foreman 1.3 when using RPMs and the file cache backend. That means users of memcached are unaffected by this particular bug, though that does listen on a TCP port which is usually also accessible by local users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks ❤️ I was not able to find it out, I did not know about the switch to packaging repo :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Altough back then there might not be any sensitive information in the cache... 🤔

Copy link
Member

Choose a reason for hiding this comment

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

OAuth was added in theforeman/foreman@fceb1f8, released in version 1.1 and already used settings. Caching of settings was added in theforeman/foreman@d9a2eba, also in 1.1.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some passwords, those should be considered compromised as well if there was a possibility to use this vulnerability.
https://github.com/search?q=org%3Atheforeman+%22encrypted+%3D%3E+true%22&type=Code

security.md Outdated
#### <a id="2020-14334"></a>CVE-2020-14334: World readable cache could expose sensitive settigs
Even encrypted settings have their raw values cached. Too permissive mode on cache dir caused, that anyone with access to the hosting system could read this encrypted settings.

*Mitigation:* change cache directory mode `o-r` on `/run/foreman`.
Copy link
Member

Choose a reason for hiding this comment

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

This is not a correct mitigation because it's lost at reboot. You must manually change the tmpfiles as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change that to advise the temfile rewrite from /etc/tmpfiles.d

@ekohl
Copy link
Member

ekohl commented Jul 30, 2020

Do we want to suggest to rotate OAuth keys, or at least provide instructions? Are there other secrets there that might have leaked?

@ezr-ondrej
Copy link
Member Author

I've tried to suggest the rotation of OAuth keys and passwords, but I'm not sure if we should provide more information.

List of settings that might have been affected: https://github.com/search?q=org%3Atheforeman+%22encrypted+%3D%3E+true%22&type=Code

security.md Outdated
d /run/foreman 0750 foreman foreman -
```

Additionaly it's better to rotate the OAuth keys and Remote Execution passwords
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Additionaly it's better to rotate the OAuth keys and Remote Execution passwords
In case the system may have been accessed locally by an un-trusted user, it may be prudent to change and secrets stored in the settings, such as OAuth keys or remote execution passwords.

security.md Outdated
Even encrypted settings have their raw values cached. Too permissive mode on cache dir caused, that anyone with access to the hosting system could read this encrypted settings.

*Mitigation:* override directory mode to `750`.
To do so, create a file `/etc/tmpfiles.d/foreman.conf` with following content and reboot the system.
Copy link
Member

Choose a reason for hiding this comment

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

This file should already be present if i'm not mistaken? Also, iiuc this modification is only needed so the change survives reboots, the user can also manually chmod the folder for the currently running instance without requiring a full reboot.

Copy link
Member

Choose a reason for hiding this comment

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

No, we ship to /usr/lib/tmpfiles.d/foreman.conf. The downside of creating /etc/tmpfiles.d/foreman.conf is that if we want to make further changes in the future, then users won't get them. However, users probably also shouldn't change /usr/lib.

Changing in /usr/lib/tmpfiles.d means they get updated in the future, but if they update from unsupported-x.y to unsupported-x.z then they lose the protection. OTOH, they should just update to a supported version.

I'm conflicted on the best advise, but slightly leaning to /usr/lib/tmpfiles.d.

@theforeman/packaging what would you advise?

Copy link
Member

Choose a reason for hiding this comment

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

etc is for overrides if package defaults, and thats exactly whats needed here. So IMHO etc and the hint to remove that file after upgrading to a fixed release.

Copy link
Member

Choose a reason for hiding this comment

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

systemd-tmpfiles.d does not allow for drop-ins like with unit files, but it explicitly mentions splitting the configuration for easier overwriting of configuration.

Each configuration file shall be named in the style of package.conf or package-part.conf. The second variant should be used when it is desirable to make it easy to override just this part of configuration. 

So perhaps it would make sense to split it up and store values in /usr/lib/tmpfiles.d where packages should install the files? Not sure if this would mess things up, but perhaps an idea for the future. But I am also pro /usr/lib/tmpfiles.d.

For those already having created local configuration in /etc/tmpfiles.d, it makes sense to advice them to adjust configuration, but I would leave this to documentation and perhaps it is possible to have a migration in the foreman-installer which checks for the CVE, links to it and provides a simple sed for mitigation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the suggestion to update of the /usr/lib/tmpfile.d as @ekohl is mentioning, the next upgrade should be to a supported version, so this solution will leave the least traces in the system.

@ezr-ondrej ezr-ondrej force-pushed the cve-2020-14334 branch 2 times, most recently from 9e8af50 to e6e8d24 Compare August 7, 2020 09:49
security.md Show resolved Hide resolved
security.md Outdated
```
d /run/foreman 0750 foreman foreman -
```
For the change to have effect immediatelly run `systemd-tmpfiles --create` or change the directory mode manually by running `chmod 755 /run/foreman`.
Copy link
Member

Choose a reason for hiding this comment

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

I like the systemd-tmpfiles --create suggestion since you verify the state post-reboot. I'd leave off the chmod command. Also note you have 755 in the current command

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping the chmod 👍

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Small language nit, otherwise 👍

security.md Outdated
Even encrypted settings have their raw values cached. Too permissive mode on cache dir caused, that anyone with access to the hosting system could read this encrypted settings.

*Mitigation:* override `/run/foreman` directory mode to `0750`.
To do so in a manner that survives reboot, update a file `/usr/lib/tmpfiles.d/foreman.conf`.
Copy link
Member

Choose a reason for hiding this comment

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

It feels more natural to use the definitive article here since we know what we're talking about.

Suggested change
To do so in a manner that survives reboot, update a file `/usr/lib/tmpfiles.d/foreman.conf`.
To do so in a manner that survives reboot, update the file `/usr/lib/tmpfiles.d/foreman.conf`.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 thanks, commited.

@ezr-ondrej
Copy link
Member Author

@tbrisker, any more comments? :)

security.md Outdated
```
For the change to have effect immediatelly run `systemd-tmpfiles --create`.

In case the system may have been accessed locally by an un-trusted user, it may be prudent to change and secrets stored in the settings, such as OAuth keys or remote execution passwords.
Copy link
Member

Choose a reason for hiding this comment

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

just a tiny typo:

Suggested change
In case the system may have been accessed locally by an un-trusted user, it may be prudent to change and secrets stored in the settings, such as OAuth keys or remote execution passwords.
In case the system may have been accessed locally by an un-trusted user, it may be prudent to change any secrets stored in the settings, such as OAuth keys or remote execution passwords.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Done.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @ezr-ondrej !

@tbrisker tbrisker merged commit b309f80 into theforeman:gh-pages Aug 11, 2020
@ezr-ondrej ezr-ondrej deleted the cve-2020-14334 branch September 23, 2020 13:10
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

Successfully merging this pull request may close these issues.

None yet

6 participants