[pupnp-tracker] [ pupnp-Bugs-1935694 ] Fixed length buffer for UPNP Action URLs breaks applications

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Bugs item #1935694, was opened at 2008-04-05 23:33
Message generated for change (Comment added) made by leveret
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: Open
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: Nick Leverton (leveret)
Date: 2008-05-05 22: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 20: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 19: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