Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPnP 1.1 support is absent. #6

Open
fishface60 opened this issue Sep 6, 2020 · 3 comments
Open

UPnP 1.1 support is absent. #6

fishface60 opened this issue Sep 6, 2020 · 3 comments

Comments

@fishface60
Copy link

From all the places http://www.upnp.org/specs/arch/UPnP-arch-DeviceArchitecture-v1.1.pdf explicitly mentions the 1.1 spec, the differences are:

  1. Devices must format UUIDs in the form XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX

    Clients must support arbitrary text UUIDs for backwards compatibility, and upnplib supported 1.0, so no changes are required for the client.

  2. Page 23. Devices must send UPnP/1.1 in the user agent

    The specification says that UPnP 1.1 clients must accept a higher minor version. upnplib doesn't check the version number in that header so is currently compliant.

  3. Page 24. SSDP response CONFIGID.UPNP.ORG header changes

    1. The value must be in the range 0-16777215.
      This is a requirement on the device so upnplib requires no changes.
    2. 1.1 clients may ignore the header in favour of HTTP cache expiry.
    3. 1.1 clients should if the header is omitted use HTTP cache expiry.
    4. 1.1 clients may use cached results of later requests based on the CONFIGID value.

    It's mandatory that 1.1 devices provide the header, so the possibility of it being absent is future-proofing that probably won't happen.
    It's valid to not implement caching and ignore the header entirley, as upnplib already implements, so this requires no changes to upnplib.

  4. Page 40. UPnP 1.1 clients may include the UPnP version in their HTTP request user-agent header.
    This is specified to change some behaviours of 1.1 devices, but devices explicitly must support it not being present. upnplib is compliant to not send it, if it does other changes are required to bring it into compliance, and some requests are made via jxpath, which it isn't clear how you would change the headers.

  5. Page 42. UPnP 1.1 devices should not offer HTTP redirect responses and must not offer any redirects except for 307 temporary redirect.
    This is to fix a security vulnerability.
    UPnP 1.1 clients should still follow 307 redirects but shouldn't follow others.
    upnplib is compliant as it's mostly a requirement on the devices, and there's no clear way to limit how jxpath fetches.

  6. Page 44. Clients must accept higher minor version numbers.
    I believe the intention is that future revisions of the devices are responsible for providing interfaces that are backwards compatible for clients.
    https://github.com/RPTools/upnplib/blob/main/src/net/sbbi/upnp/devices/UPNPRootDevice.java#L134 is where upnplib needs to relax the version check.

  7. Page 44. UPnP 1.1 devices must not include URLBase.
    This is to fix a security vulnerability.
    UPnP 1.0 clients are required to handle it being omitted, and 1.1 clients are expected to handle it if provided for compatibility with UPnP 1.0 devices, so the device must change but clients stay the same so upnplib is already compliant.

  8. Page 48. UPnP 1.1 Devices may omit the serviceList or leave it empty if a device type doesn't implement any services.
    This should only require changing https://github.com/RPTools/upnplib/blob/main/src/net/sbbi/upnp/devices/UPNPRootDevice.java#L340 to handle it returning null.

  9. Page 57. Service definitions may have complex values.
    UPnP 1.0 didn't provide support for hierarchical or arbitrary length value types. Service designers made do with the string type.
    UPnP 1.1 extends this and calls those types "string equivalents".
    If a client sends UPnP/1.1 in their user agent they must be prepared to parse complex types.
    If a UPnP 1.1 device receives a UPnP/1.1 user-agent then it must use complex values instead of string equivalents.
    If a UPnP 1.1 device implements a service that doesn't have string equivalents it must refuse requests that omit the UPnP/1.1 user-agent.
    upnplib may continue to not send the user-agent to remain compliant.

  10. Page 69. UPnP 1.1 must support SOAP 1.1.
    I'm not familiar enough with SOAP to determine compliance.

  11. Page 72. The root of a request body must be the first child element of the element.
    upnplib appears to already do this.

  12. Page 72. UPnP 1.1 may omit the encodingStyle attribute or set it to http://schemas.xmlsoap.org/soap/encoding/.
    upnplib already does this as setting it is valid and UPnP 1.0 required it.

  13. Page 73. The element is required in SOAP messages.
    upnplib already does this.

  14. Page 73. UPnP 1.1 devices must accept POST requests rather than the M-POST extension.
    upnplib already supports this and never implemented M-POST.

  15. Page 87. Infinite subscription duration is deprecated.
    UPnP 1.1 clients mustn't request to subscribe for infinite duration.
    UPnP 1.1 devices must silently ignore an infinite subscription.
    It's arguable whether upnplib is compliant for allowing the request, since the application plays a part in compliance and would be compliant if it didn't request an infinite duration. https://github.com/RPTools/upnplib/blob/main/src/net/sbbi/upnp/samples/MyStateVariableEventsHandler.java#L49 is the only place infinite is requested. MapTool doesn't appear to use ServicesEventing.

Relaxing the requirement check and fixing the off-by-one error in https://github.com/RPTools/upnplib/blob/main/src/net/sbbi/upnp/services/UPNPService.java#L216 from xpath being 1-indexed instead of 0-indexed makes it work for devices that don't omit serviceList.

@Azhrei
Copy link
Member

Azhrei commented Sep 6, 2020

Wow, excellent work in digging out this list of changes and checking the existing code base for compliance. Thank you!

@Azhrei
Copy link
Member

Azhrei commented Sep 29, 2020

We're getting ready to do a release of MapTool 1.8 which uses this library; were you planning to submit a PR for the four changes you found? (It looks like your items 6, 8, and maybe 15, have updates, plus the off-by-one error in your closing paragraph). Thanks.

@Azhrei
Copy link
Member

Azhrei commented May 25, 2021

Thanks again for doing the research on this.

  1. I loosened the version check in item 6.
  2. I added support for an empty serviceList as in item 8 (all references to deviceCtx.services check for null, so I used null to indicate an empty list).
  3. I will check MapTool to ensure it doesn't make an infinite port mapping, as described in item 15, and I've updated the sample to use a 24-hour timeout.

Now to build a JAR and test it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants