inventory plugins: make data obtained from remote unsafe

[felixfontein]

trafficstars

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

• Bugfix Pull Request

COMPONENT NAME

inventory plugins

[ansibullbot]

cc @Ajpantuso @BongoEADGC6 @Charliekenney23 @InTheCloudDan @LBGarber @Thulium-Drake @abarbare @conloos @ddelnano @decentral1se @displague @feldsam @ilijamt @joshainglis @karmab @krauthosting @morph027 @opoplawski @remyleone @rmcintosh @shayrybak @shinuza click here for bot help

[felixfontein]

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.

[opoplawski]

I haven't noticed any trouble with the cobbler plugin.

[bcoca]

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.

[bcoca]

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'.

[felixfontein]

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 add_host and add_group as unsafe.

Also features like constructed work on a completely different level. They read data (usually) from a YAML file you provide, not from some external service you usually only partially control. The YAML file defines which expressions to evaluate. This is very different from, say, VM or container labels or free-text fields provided by another service.

[bcoca]

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.

[felixfontein]

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.

[felixfontein]

If nobody objects, I'll merge this tomorrow.

[patchback[bot]]

Backport to stable-6: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply d62fe154d296f7d738dd4dad0263bd918aa4e607 on top of patchback/backports/stable-6/d62fe154d296f7d738dd4dad0263bd918aa4e607/pr-8098

Backporting merged PR #8098 into main

1. Ensure you have a local repo clone of your fork. Unless you cloned it from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these instructions you'll refer to it by the name upstream. If you don't have it, here's how you can add it:

   $ git remote add upstream https://github.com/ansible-collections/community.general.git
   

3. Ensure you have the latest copy of upstream and prepare a branch that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/stable-6/d62fe154d296f7d738dd4dad0263bd918aa4e607/pr-8098 upstream/stable-6
   

4. Now, cherry-pick PR #8098 contents into that branch:

   $ git cherry-pick -x d62fe154d296f7d738dd4dad0263bd918aa4e607
   

   If it'll yell at you with something like fatal: Commit d62fe154d296f7d738dd4dad0263bd918aa4e607 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x d62fe154d296f7d738dd4dad0263bd918aa4e607
   

5. At this point, you'll probably encounter some merge conflicts. You must resolve them in to preserve the patch from PR #8098 as close to the original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/stable-6/d62fe154d296f7d738dd4dad0263bd918aa4e607/pr-8098
   

7. Create a PR, ensure that the CI is green. If it's not — update it so that the tests and any other checks pass. This is it! Now relax and wait for the maintainers to process your pull request when they have some cycles to do reviews. Don't worry — they'll tell you if any improvements are necessary when the time comes!

🤖 @patchback I'm built with octomachinery and my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback[bot]]

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/d62fe154d296f7d738dd4dad0263bd918aa4e607/pr-8098

Backported as https://github.com/ansible-collections/community.general/pull/8145

🤖 @patchback I'm built with octomachinery and my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback[bot]]

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/d62fe154d296f7d738dd4dad0263bd918aa4e607/pr-8098

Backported as https://github.com/ansible-collections/community.general/pull/8146

🤖 @patchback I'm built with octomachinery and my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@opoplawski @bcoca @russoz @morph027 thanks a lot for reviewing/testing/... this!

[marek1712]

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.

Hi.

Looks like there are at least two plugins affected: community.general.proxmox and servicenow.itsm.