security: RCE for ikea_tradfri calls

[lian]

just a heads up, was briefly looking over the code:

user controls payload["5851"] (bri) value and can escape the shell call https://github.com/mariusmotea/diyHue/blob/b2885e65326df4a7b9f82f46cef36e642b450d28/BridgeEmulator/HueEmulator3.py#L606

user controls get_parameters values and can escape the shell call https://github.com/mariusmotea/diyHue/blob/b2885e65326df4a7b9f82f46cef36e642b450d28/BridgeEmulator/HueEmulator3.py#L1081

[ghost]

@lian do you have a way to fix this? If so could you submit a PR?

[ghost]

@mariusmotea are you worried about this? Do you have a fix? It doesn't seem like @lian can help explain/fix the bug further. Close this?

[mariusmotea]

I don't understand exactly where is the problem, i believe the security concern is because i execute shell commands with the password, but anyway a person with access to host can get the sensitive data from configuration file. I will close this now.

[lian]

its a remote code execution. any machine on the same network can take over the machine running diyHue.

https://docs.python.org/3/library/shlex.html#shlex.quote

[ghost]

@mariusmotea It looks like there is a fix in the link @lian posted. This is something, I think, that needs fixing soon!

[ghost]

Perhaps the fix is something like this?

check_output("./coap-client-linux -m put -u \"" + bridge_config["lights_address"][light]["identity"] + "\" -k \"" + bridge_config["lights_address"][light]["preshared_key"] + "\" -e '{ \"3311\": [" + json.dumps(quote(payload)) + "] }' \"" + url + "\"", shell=True)

Though I don't have a Tradfri system to test this with

[lian]

you need to quote every argument that can contain user supplied values.

i'm not a python guy, but something like this:

json_payload = '{ "3311": [' + json.dumps(payload) + '] }'

check_output(
  "./coap-client-linux -m put -u " + 
  quote(bridge_config["lights_address"][light]["identity"]) +
  " -k " + 
  quote(bridge_config["lights_address"][light]["preshared_key"])
  " -e " + 
  quote(json_payload) + 
  " " + 
  quote(url)
  ,
  shell=True
)

[ghost]

Ah yes I see what you mean now! What do you think @mariusmotea?

[mariusmotea]

But \" that we currently use is not the same like quote(something) ?

[lian]

\" is the insecure version of quote() which an attacker can easily break out of.

[ghost]

@mariusmotea was this ever fixed in the new repo?

Also, can you set this repo to archived (read-only) so that new issues are only made in the new repo?

[mariusmotea]

I don't know exactly how to fix this and for me this is not the kind of security issue that can be a concern for this project.

[ghost]

Ok well, in that case, ill close this and create something in the todo project board so it can be picked up in the future if required.