Copied from https://github.com/adamruzicka/receptor-satellite/issues/2 Description of problem: In the worker, several inputs are provided by external entity (not receptor), such as hosts, text_updates, text_update_interval & text_update_full are applied without checks. These values should probably be checked for sanity, as a safety measure (I am aware of the fact that at this point, untrusted input would lead to far more disastrous consequences on the hosts. This current issue is only aiming at protecting Receptor). ``` class Config: def __init__(self, text_updates = False, text_update_interval = 5000, text_update_full = True): self.text_updates = text_updates self.text_update_interval = text_update_interval self.text_update_full = text_update_full ``` ``` class Run: def __init__(self, queue, remediation_id, playbook_run_id, account, hosts, playbook, config, plugin_config): self.queue = queue self.remedation_id = remediation_id self.playbook_run_id = playbook_run_id self.account = account self.playbook = playbook self.config = Config.from_raw(config) self.hosts = [Host(self, None, name) for name in hosts] self.satellite_api = SatelliteAPI.from_plugin_config(plugin_config) ``` In all of the above, I think only queue (created locally) and plugin_config (provided by receptor itself), should be considered trusted from the plugin's point of view. The rest is taken from a JSON payload provided by external source. Examples : * If `text_update_interval` is not a number, I suspect it might be able to trigger a backtrace, leading to a DoS in `await asyncio.sleep(self.run.config.text_update_interval / 1000)` Also the sanity of the number should probably be checked. * Regarding the hosts, from the plugin's point of view, it's probably feasible to send a single host name that contains ',' (e.g.: `[ "host1", "host2,secret", "host3" ]` ): this would be treated as 3 hosts by the worker, but 4 hosts by the satellite (`"host_ids": "name ^ ({})".format(','.join(hosts))`. Version-Release number of selected component (if applicable): python3-receptor-satellite <= 1.0.1-1
NEEDINFO! @ Satellite 6.8 snap 11 Steps: ---------------- Ran my own "fake" c.rh.c, a new receptor instance configured to listen to it, and ran commands that command the fake c.rh.c to send some data. Step by step: 1) In terminal 1 on the Satellite: # receptor --debug --node-id controller -d /tmp/controller node --listen=receptor://0.0.0.0:8888 2) In terminal 2 on the Satellite: Take config.conf and put it to the working directory. 3) In terminal 2 on the Satellite: # receptor --config ./config.conf --debug --node-id node -d /tmp/node node --peer=localhost:8888 --listen=tcp://0.0.0.0:8889 4) In terminal 3 on the Satellite: Copy data.json and put it to the working directory. WHILE it_is_tested_enough DO 5) In terminal 3 on the Satellite: Change data in data.json for testing - try different inputs, valid and non-valid, try to break things - this is what the BZ is about and where you fake the parameters. 6) In terminal 3 on the Satellite: receptor --debug -d /tmp/send send --peer=127.0.0.1:8888 node --directive=receptor_satellite:execute "$(cat data.json)" DONE Observations: ----------------------- 1. Verified: ----- WARNING 2020-08-09 05:22:29,141 node worker Expected the value of text_update_interval '1000' to be an integer greater or equal than 5000 WARNING 2020-08-09 05:32:51,571 node worker Expected the value of text_update_interval '6000' to be an integer greater or equal than 5000 WARNING 2020-08-09 05:36:42,663 node worker Expected the value of text_updates 'true' to be a boolean WARNING 2020-08-09 05:36:42,663 node worker Expected the value of text_update_full 'false' to be a boolean WARNING 2020-08-09 05:40:16,660 node worker Expected the value of text_updates '1' to be a boolean WARNING 2020-08-09 05:40:16,660 node worker Expected the value of text_update_full '0' to be a boolean WARNING 2020-08-09 06:32:06,934 node worker Hostname 'diverse-penguin-00.lxd,jitu,ii' contains a comma, skipping ERROR 2020-08-09 05:38:21,622 node worker Playbook run 471 already known, skipping. 2. FailedQA (Need to confirm by Dev if those are expected): ---- 2.1) Remediation ID and Playbook IDs as Integers are not handled(No warnings displayed): "remediation_id": 1123, "playbook_run_id": 472, 2.2) Account Id is not handled as String (No warnings displayed): "account" : "211233" 2.3) The special characters, underscores are not handled for hostnames. "hosts": ["diverse-pe#@@#$$$%nguin-00.lxd"], "hosts": ["diverse-penguin-00_lxd"], 2.4) No warnings if any of the remediation_id, playbook_run_id, account or hosts fields are not given. 2.5) Playbook issues are not handled. "playbook": "---\n hosts: all\n tasks:\n - shell: ls -la /\n - shell: ls -la /tmp\n - shell: ls -la /etc\n", (Observe hyphen before hosts is missing in playbooks) 2.6) I also observed, that regardless of correct or wrong data in data.json, It always throw this error: ERROR 2020-08-09 05:51:09,598 node work Error encountered while handling the response, replying with an error message ('hosts') Let me know if those FailedQA are expected or not to Verify the Bug.
The original issue on github (see #0) clearly states which fields need to be checked and why and only those fields were covered. 2.1) remediation_id - Receptor-satellite does not care about remediation_id at all. No checks are needed, because we never use the value anywhere, it is just something that the cloud sends but is completely useless to us. playbook_run_id - In general, this should be an uuid, but we don't really care. We take whatever is here and just check for equality against other plabook_run_ids we already know. 2.2) we don't care about account number. It gets passed to us from cloud, but we never use it anywhere. It can be anything and it won't have any effect on anything 2.3) This is not an issue. The original issue was that we get a list of hostnames with cloud, join them with commas and send it to satellite. If any of the hostnames contained a comma, it would be wrongly parsed by satellite. Any other characters are fine, it will just be sent to satellite and there it won't match any host. 2.4) This is the same as 2.1, 2.2 and 2.4. Why mention it again? 2.5) This is not our issue. The cloud wants us to run something so we try it. If the thing that should be run is malformed, ansible will crash, we will tell cloud and that will be it. 2.6) Here I assumed that you didn't have any diverse-penguin-00.lxd on your satellite and therefore you were running into https://bugzilla.redhat.com/show_bug.cgi?id=1867399 TL;DR: I think all of these are expected
Verified! @satellite 6.8 snap 11 Steps: -------------- See comment 1 for steps. Observation: ---------------- See Comment 1 for observation and Comment 2 for ACK from DEV. Changing the state to Verified.
Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory (Important: Satellite 6.8 release), and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHSA-2020:4366