[Nice] patch for libnice-0.1.1/stun/tools/stund.c

[Youness Alaoui]

Hi John,

Thank you for the patches and the detailed report on the issues you've encountered.

One thing though, in the future, you should commit your fixes into small git
commits and use git format-patch to generate the patches, this way I can just
use "git am" and commit them (with you as author, with your commit description,
avoids conflicts when you pull, etc..).


On 02/08/2012 02:18 PM, John Selbie wrote:
> During my interop testing with stuntman (stun server) and its client library
> code, I found a couple of bugs in the "stund" tool that builds with libnice. I
> have provided a patch to fix the issue.
> 
> Issue: If stund tool receives a STUN binding request with an unknown attribute,
> the code will get into an infinite spinning loop and not process any more
> incoming packets.
> 
> Here is what is happening.
> 
> The dgram_process() function receives a datagram with a STUN binding request.
> 
> If stun_agent_validate() returns UNKNOWN_REQUEST_ATTRIBUTE (which it will do for
> almost any message with an attribute.)
> 
> Then stun_age_build_unknown_attributes_error() is called to build the message. 
> Then a "goto send_buf".
> 
> Then send_safe is called. But the iov.iov_len hasn't been properly set. It's
> initial value is 65552, which is longer than the length of a valid UDP
> datagram.  So now sendmsg() is called inside safe_send() and returns -1
> (errno==EMSGSIZE).  And then safe_send gets into an infinite loop and never returns.
> 
> The simple fix is to move the stun_agent_finish_message() call to be below the
> send_buf label instead of directly above it.  Diff below.
Good point! I hadn't noticed that.
However, your fix is wrong, because the
stun_agent_build_unknown_attributes_error already calls stun_agent_finish_message.
The proper fix would be to set the iov.iov_len to the return value of the
build_unknown_attributes_error, before going to send_buf.
http://cgit.collabora.com/git/libnice.git/commit/?id=dba31d4de68dfdc0186d725158251d3e36a20a99

> 
> Two other issues.
> 
> 1) stun/stunmessage.h defines STUN_MAX_MESSAGE_SIZE to be 65552.  For UDP, this
> is impossible, because the maximum value for a 16-bit length field in the UDP
> header is 65535. Subtract off the 8 byte UDP header, and that leaves a
> theoretical maximum for a datagram payload buffer to be 65527 (or 65507 if the
> IP header is factored in).  And realistically, STUN messages don't get anywhere
> near that size.
Yeah, I agee, but I think that was a legacy value that is left from the very
first version. I don't know where that 65552 number comes from, but either way,
I'm leaving it like that to avoid breaking the ABI.
While libnice itself doesn't expose any structure which uses that define, some
other apps might depend on it and use it. If I changed that value, I would be
breaking the ABI of those libraries that depend on libnice.
Either way, I don't believe that 50 bytes are going to make much of a difference
when you allocate 64K...

> 
> 2) Also, safe_send does not look safe to me. Since the socket isn't non-blocking
> or have any other special attributes, it doesn't make sense to keep repeatedly
> trying to call sendmsg() until it succeeds. (At least not without evaluating the
> errno). So in the diff below, I just removed it in favor of a direct call to
> sendmsg.
Yeah, I never understood why stund was originally written with that whole
sendmsg/recvmsg code. I'd love to have a much simpler version of stund written
which just uses normal recvfrom/sendto and no iov, etc... (that would also help
for porting to other platforms/windows).
(5 minutes later)...
Done! :)
http://cgit.collabora.com/git/libnice.git/commit/?id=fe4209094484409edc15e180be21cea85832f75f

> 
> I found this issue with the the stuntman client library code, when it sends a
> binding request, it always sends a CHANGE-REQUEST attribute even if both the ip
> and port flags are both false. The reason I have to do this is because of a bug
> in JSTUN. The JSTUN server code has a bug that will reject any message that
> doesn't have a CHANGE-REQUEST attribute.  But the stund tool with libnice will
> reject any binding request with any attribute in it (because the library call to
> tell the agent the acceptable attributes aren't there.)
Humm.. maybe the proper solution then would be to make stund support this
CHANGE-REQUEST attribute.
If you noticed, stund tries to validate the message using two agents, one for
the new RFC5389 and one for the old RFC3489, but it doesn't actually follow the
specifications (since stund is not actually meant to be used as a stun server,
it's only a "testing tool" and used for unit tests and maybe for testing your
applications).
The new RFC5389 states :
   The client might, in rare situations, include either the RESPONSE-
   ADDRESS or CHANGE-REQUEST attributes.  In these situations, the
   server will view these as unknown comprehension-required attributes
   and reply with an error response
but the old RFC3489 states :
   The server MUST add a SOURCE-ADDRESS attribute to the Binding
   Response, containing the source address and port used to send the
   Binding Response.

   The server MUST add a CHANGED-ADDRESS attribute to the Binding
   Response.  This contains the source IP address and port that would be
   used if the client had set the "change IP" and "change port" flags in
   the Binding Request.  As summarized in Table 1, these are Ca and Cp,
   respectively, regardless of the value of the CHANGE-REQUEST flags.

We are not adding the SOURCE-ADDRESS or CHANGED-ADDRESS or following anything
else from the spec. However, if you want, you could add the CHANGE-REQUEST and
RESPONSE-ADDRESS attributes to the list of known_attributes used to initialize
the old rfc agent. This way at least, you wouldn't get this error.

> 
> Here's the diff for stund.c
> 
> 201,213d200
> < /** Sends a message through a socket */
> < ssize_t send_safe (int fd, const struct msghdr *msg)
> < {
> <   ssize_t len;
> <
> <   do
> <     len = sendmsg (fd, msg, 0);
> <   while ((len == -1) && (recv_err (fd) == 0));
> <
> <   return len;
> < }
> <
> <
> 282d268
> <   iov.iov_len = stun_agent_finish_message (agent, &response, NULL, 0);
> 283a270
>>   iov.iov_len = stun_agent_finish_message (agent, &response, NULL, 0);
> 285,286c272,273
> <   len = send_safe (sock, &mh);
> <   return (len < iov.iov_len) ? -1 : 0;
> ---
>>   len = sendmsg (sock, &mh, 0);
>>   return len;
> 
> 
> John Selbie
> 
> 
> 
> _______________________________________________
> nice mailing list
> nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20120209/aeabc4a9/attachment.pgp>