#14 Fixed length buffer for UPNP Action URLs breaks applications
(forwarded from a Debian user by Debian libupnp package maintainer, http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=353169\)
When using the UpnpSendActionAsync method (and possibly other Async methods), the SDK stores the URL for the action in a struct UpnpNonblockParam. This has a fixed length array for storing the action URL of 100 characters. Some UPNP servers routinely generate control URLs longer than 100 characters:
at src/api/upnpapi.c:2694, the SDK then proceeds to strcpy the control URL to the Param struct, resulting in stack-overwritey-badness.
Our user provided a patch (attached) and adds:
Please find attached a patch which goes some way to fixing this issue. I've only tested it in my current configuration, so I don't know how this impacts the webserver. I've had to introduce two new functions to free structures that may be returned by the framework. Not sure how necessary they are, as I think mostly it frees up what it gives you.
Patch for fix against pupnp 1.6.5
Logged In: YES
user_id=67568
Originator: NO
Hi Nick,
First, please accept my apologies for taking so long to answer.
I have read your users complaints and I see the problems.
I don't know what is the current library revision you are using, but since the first version of libupnp after we started maintaining it (1.4.0) the size of the buffer is 256 bytes. Not that it matters that much, because the overflowing problem remains, but you seem to be using a version of the library that is too old and of course has many already corrected bugs. I would advise to go to 1.6.x series as quick as possible.
Also, somewhere in the post it has been said that libupnp is a C++ package and that the patch should have new/delete. This is incorrect, libupnp is plain C, not C++.
Without thinking too much, we have two solutions to this problem. Either malloc a new buffer at each request, or use a fixed size and a hard limited copy. I would prefer the last solution, but only if the UPnP standard poses the limitation, otherwise we will have to go with the first. Does anyone know anything about this?
Regards,
Marcelo.
Logged In: YES
user_id=46394
Originator: YES
Hello Marcelo
I formally took over the package "ownership" in Debian only a week ago, so thanks for your reply ! Unfortunately the user in question stopped using pupnp whilst there were no updates to it in Debian. However I thought it still worth submitting his fix to you. I've now brought Debian up to 1.6.5 and am eagerly watching your 1.6.6 comments in CVS changelog :-)
As to fixed buffer sizes, I tried to understand the upnp specifications but I can't remember if it even specified any limits or not. I would normally prefer to use dynamically allocated buffers for flexibility towards the users, but I can see that would be a major piece of work for pupnp due to the way the Intel library was written with fixed buffers. I can't take on too much extra work myself at the moment regrettably :-(
I hope I can provide more contributions in due course. I have been concentrating on the packaging rather than the functioning to begin with, because Debian will soon be going into freeze for its next release. The comment about "C++" was my error, I'm sorry.
Logged In: YES
user_id=46394
Originator: YES
Hi Marcelo
You are quite correct, the problem was originally raised against v1.2.1. Like you I couldn't see any limit on the length of strings in uPnP so I just updated the patch and passed it on. Debian now contains v1.6.5 and will be updated to 1.6.6 shortly.
The C/C++ confusion was my error based on a poor memory when I wrote the response to the user, I apologise !
I see in SVN you have some changes planned for release 1.8.0 which address the problem by using dynamically allocated strings with included length. I have only had a quick look, but I think it is a safe approach which should work reliably in the long term.
Regards
Nick
Logged In: YES
user_id=67568
Originator: NO
Hi Nick,
Thank you for taking the time to look at the new code.
Indeed, we are already using dynamically allocated buffers in the SVN repository. We hope that any buffer overflow issues remaining be fixed with this approach.
Also, we are now using opaque data types, so that developers can change the internal library data structures without breaking the library API. And the new 1.8.0 code will be IPv6 enabled. IPv6 is functional in the SVN repository.
There have been lots of changes and now the code needs some testing.
Nevermind about the C/C++ confusion :)
Do you mind if I close this issue? Or would you prefer to leave this open until 1.8.0 is released and you have tested it?
Regards,
Marcelo.
Logged In: YES
user_id=67568
Originator: NO
Hi again Nick,
We are about to release the 1.8.x code, and I would like to know it this is still an issue. We have addressed (I hope) all the external variable length strings issues by using a new type, UpnpString.
I will leave this as pending, feel free to reopen if there is any problem.
Regards,
Marcelo.
Logged In: YES
user_id=1312539
Originator: NO
This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).
Logged In: YES
user_id=46394
Originator: YES
Hi Marcelo
One of my users found the patch I supplied causes segfaults (probably some fault introduced in updating it for 1.6). I was trying to debug it and got deeper and deeper into the scheduler and its object management ! So instead of replicating all your work on 1.8 I just reverted that patch for my 1.6.6 which is now awaiting upload to Debian.
There is some application porting required to test out 1.8 for the API naming changes, so I haven't tried that branch yet. I am confident it will be better tested than my 1.6 patch already though :-) Happy to leave this issue closed for now, I will re-open if needed.
Reopened upon request from Nick.