-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
inventory plugins: make data obtained from remote unsafe #8098
Conversation
I would be glad if you could take a close look at this PR and in particular test it (I don't have the means to test almost all of these plugins), so we don't accidentally break users. |
I haven't noticed any trouble with the cobbler plugin. |
I'm not sure this is viable, if you cannot trust your inventory ... there is almost no reason to run Ansible at that point. This is really only a vulnerability in installations where the secrets are kept locally in ansible files, which is the opposite of what most setups that use remote inventory sources (normally they provide the secrets). At the very least i would make this a configuration toggle. |
I'm thinking it is probably a lot simpler to just add this to the base inventory class as an option 'untrusted: false|true` ... just does not make a lot of sense when you add things like 'constructed'. |
I strongly disagree. Usually your inventory, when coming from an external service, is only partially trustable. Some values you have to trust (otherwise you shouldn't use it at all), but others you cannot trust, as they can come from random sources, even from plain customer input (like labels for containers/VMs, or free-text fields). You really don't want to add these things as safe texts that can be evaluated. This is also something that most users will not expect to happen IMO. Even worse is that if hostnames or group names have templates, that they will be evaluated in some situations (see bottom of https://www.die-welt.net/2024/03/remote-code-execution-in-ansible-dynamic-inventory-plugins/). I don't see any reason not to mark strings passed to Also features like |
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.
Not familiar with inventory plugins nor this make_unsafe
function and its semantics, but it LGTM.
The 'constructed' inventory plugin is one thing, but I was thinking about the feature in general (from the base class) which is also used by many other inventory plugins, including aws, azure, openstack and many others. |
That's the same I'm talking about. That one still works fine, it's not affected by this PR. |
If nobody objects, I'll merge this tomorrow. |
Backport to stable-6: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply d62fe15 on top of patchback/backports/stable-6/d62fe154d296f7d738dd4dad0263bd918aa4e607/pr-8098 Backporting merged PR #8098 into main
🤖 @patchback |
Backport to stable-7: 💚 backport PR created✅ Backport PR branch: Backported as #8145 🤖 @patchback |
Make data obtained from remote unsafe. (cherry picked from commit d62fe15)
Backport to stable-8: 💚 backport PR created✅ Backport PR branch: Backported as #8146 🤖 @patchback |
Make data obtained from remote unsafe. (cherry picked from commit d62fe15)
@opoplawski @bcoca @russoz @morph027 thanks a lot for reviewing/testing/... this! |
…lections#8098) Make data obtained from remote unsafe. (cherry picked from commit d62fe15)
Hi. Looks like there are at least two plugins affected: community.general.proxmox and servicenow.itsm. |
SUMMARY
This PR marks (almost) all data set by inventory plugins as unsafe to avoid potential remote code executions on the controller.
Ref: https://www.die-welt.net/2024/03/remote-code-execution-in-ansible-dynamic-inventory-plugins/
Ref: https://forum.ansible.com/t/remote-code-execution-in-ansible-dynamic-inventory-plugins/4332
ISSUE TYPE
COMPONENT NAME
inventory plugins