search

5kyc0d3r / upnpy

Lightweight UPnP client library for Python.
https://upnpy.readthedocs.io
MIT License
60 stars 18 forks source link

Issue when scpd_url contains something strange #9

Open hotab opened 4 years ago

hotab commented 4 years ago

The library will break in different ways when scpd_url contains something other than url part beginning with /. In my specific example, a router prefixes the url with full url.

Specifically, in this line: https://github.com/5kyc0d3r/upnpy/blob/9b5e005257e92351385defb4ff6079133f2d4dbd/upnpy/ssdp/SSDPDevice.py#L293

If you have base_url == http://[router_ip]:1900 and scpd_url = http://[router_ip]:1900/some.xml - you will get a name resolution error.

Second error from here: https://github.com/5kyc0d3r/upnpy/issues/7 may be related.

5kyc0d3r commented 4 years ago

Hi, thank you for your issue. Does 1be5dab2f6356a625980aef067cf1ec285305982 fix this? Please upgrade upnpy:

pip install upnpy --upgrade
hotab commented 4 years ago

Yes, the commit seems to fix the immediate issue. But, in general it may be a somewhat bad idea. Normally, services would not be allowed to issue a url outside of base_url.

but if say base_url = http://192.168.0.1, but ssdp_url is http://google.com/some_url - traffic would go to google.com

Then we may pass another scheme, like file://.

I think we need a bit more of whitelisting here.

5kyc0d3r commented 4 years ago

Ah yes, I think I see what you mean. I'm currently adding some more checks.

5kyc0d3r commented 4 years ago

I pushed some changes in https://github.com/5kyc0d3r/upnpy/commit/eb798af6d2e8fcce0c99cbe4363ecf408ff82a1e which should (hopefully) fix these issues.

hotab commented 4 years ago 
>>> urlparse("http://192.168.0.1:1000").hostname
'192.168.0.1'
>>> urlparse("http://192.168.0.1:2000").hostname
'192.168.0.1'

There is still an issue with ports. I think best way is to have a configurable strict mode which you can set to:

  1. Allow only http/https scheme, host, and port
  2. Allow only http/https scheme and host but allow to change ports
  3. Allow only http/https scheme, but allow to vary host and port.
  4. Allow all base_urls.

Also, scheme may go up from http to https, but prob not downwards.

Default I think makes sense to stay at what it is now - changing ports allowed, but not hostname or scheme downgrade. Not sure if this will follow standards, tho.

Also, I think it would make sense to reconstruct scpd_url from base components instead of allowing it to stay the same to avoid issues with different url parsing used (e.g. scheme://username:password@host:port/path...). E.g. these slides: https://www.slideshare.net/codeblue_jp/a-new-era-of-ssrf-exploiting-url-parser-in-trending-programming-languages-by-orange-tsai

5kyc0d3r commented 4 years ago

Sounds good. I will start implementing these as soon as I get back to the PC :smiley:.