Remote arbitrary code execution (shell incjection)

[mkuratczyk]

additionalPlugins property allows arbitrary code execution through shell injection. The command to enable plugins is constructed by string concatenation, including user input (the value of additionalPlugins). This means that a user who can create RabbitmqCluster resource can also execute any command inside a RabbitMQ pod, even though that should be controlled through pods/exec privilege.

To Reproduce

1. Deploy a RabbitmqCluster foo
2. kubectl edit rabbitmqcluster foo
3. Add the following to the spec:

spec:
  rabbitmq:
    additionalPlugins:
    - rabbitmq_shovel; rabbitmqctl delete_vhost /

4. Validate that not only the plugin was enabled but also the vhost was deleted:

kubectl exec  foo-rabbitmq-server-0 -- rabbitmqctl list_vhosts
Listing vhosts ...

Expected behavior
TBD how exactly this should be handled. Options include:

1. We could have a list of "whitelisted" plugins and ignore/reject any item on the additionalPlugins list that is not one of them (the list would include all plugins shipped as part of RabbitMQ at the time a given operator version was released, unfortunately we would have to make sure we keep it up to date when new plugins are added)
2. We could use regular expressions to validate plugin names only contain alphanumerics and the underscore (that should cover all existing plugins and likely all future plugins)
3. The command could be constructed in a safe(er) way

[Zerpet]

I would prefer to use a whitelist approach using a regex during the command constructions. If we can construct a regex so that it matches and allows strings starting with rabbitmq_ followed by any number alphanumerical characters and underscores, we should cover all the cases.

[ChunyiLyu]

List of plugins for each versions of rabbitmq is not documented fully. For example, the documentation on core plugins from rabbitmq.com is incomplete and does not include rabbitmq_peer_discovery_backend plugin that we rely on doc. We will most likely need to contact the core team to get the comprehensive list, and I don't think that's a reasonable dependency on them.

The main purpose is to prevent shell injection. I think it makes sense to match each of the plugins as ^\w+$. I don't think we need to enforce all plugins to start with rabbitmq_, because that might change in the future and there are also community plugins that do not comply with that.

Option 2, using regex is the most straightforward way to solve this IMHO. Thoughts?

[mkuratczyk]

I think that's the right call. I don't want us to be in the business of updating a list of known plugins. A regexp should be good enough.

[ChunyiLyu]

Context: I am looking into CRD structure schema for this regex check. I believe this check belongs in the validation stage, and will be a better user experience if we can fail early.