From: Tracker i. u. n. <pup...@li...> - 2008-05-25 19:19:53
|
Bugs item #1935694, was opened at 2008-04-05 19:33 Message generated for change (Settings changed) made by mroberto You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=841026&aid=1935694&group_id=166957 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: upnp Group: trunk >Status: Pending Resolution: None Priority: 9 Private: No Submitted By: Nick Leverton (leveret) Assigned to: Marcelo Roberto Jimenez (mroberto) Summary: Fixed length buffer for UPNP Action URLs breaks applications Initial Comment: (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: http://192.168.0.1:2869/upnphost/udhisapi.dll?control=uuid:xxxxxxxx-b4cb-41ac-827d-xxxxxxxxxxxx+urn:upnp-org:serviceId:ContentDirectory 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. ---------------------------------------------------------------------- >Comment By: Marcelo Roberto Jimenez (mroberto) Date: 2008-05-25 16:19 Message: 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. ---------------------------------------------------------------------- Comment By: Marcelo Roberto Jimenez (mroberto) Date: 2008-05-05 19:42 Message: 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. ---------------------------------------------------------------------- Comment By: Nick Leverton (leveret) Date: 2008-05-05 18:45 Message: 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 ---------------------------------------------------------------------- Comment By: Nick Leverton (leveret) Date: 2008-04-10 16:45 Message: 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. ---------------------------------------------------------------------- Comment By: Marcelo Roberto Jimenez (mroberto) Date: 2008-04-10 15:46 Message: 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. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=841026&aid=1935694&group_id=166957 |