more cleanup, this time to stun_handle_packet(). Among other things:
authorLuigi Rizzo <rizzo@icir.org>
Thu, 12 Jul 2007 16:21:12 +0000 (16:21 +0000)
committerLuigi Rizzo <rizzo@icir.org>
Thu, 12 Jul 2007 16:21:12 +0000 (16:21 +0000)
+ mark a potentially dangerous write-past-end-of-buffer
+ localize some variables in the block generating stun replies.

As before, not ready yet for a merge to 1.4

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@74850 65c4cc65-6c06-0410-ace0-fbb531ad65f3

main/rtp.c

index c8042ca..0bac1ce 100644 (file)
@@ -482,24 +482,29 @@ void ast_rtp_stun_request(struct ast_rtp *rtp, struct sockaddr_in *suggestion, c
  */
 static int stun_handle_packet(int s, struct sockaddr_in *src, unsigned char *data, size_t len)
 {
-       struct stun_header *resp, *hdr = (struct stun_header *)data;
+       struct stun_header *hdr = (struct stun_header *)data;
        struct stun_attr *attr;
        struct stun_state st;
        int ret = STUN_IGNORE;  
-       unsigned char respdata[1024];
-       int resplen, respleft;
-       
+       int x;
+
+       /* On entry, 'len' is the length of the udp payload. After the
+        * initial checks it becomes the size of unprocessed options,
+        * while 'data' is advanced accordingly.
+        */
        if (len < sizeof(struct stun_header)) {
                ast_debug(1, "Runt STUN packet (only %d, wanting at least %d)\n", (int) len, (int) sizeof(struct stun_header));
                return -1;
        }
+       len -= sizeof(struct stun_header);
+       data += sizeof(struct stun_header);
+       x = ntohs(hdr->msglen); /* len as advertised in the message */
        if (stundebug)
-               ast_verbose("STUN Packet, msg %s (%04x), length: %d\n", stun_msg2str(ntohs(hdr->msgtype)), ntohs(hdr->msgtype), ntohs(hdr->msglen));
-       if (ntohs(hdr->msglen) > len - sizeof(struct stun_header)) {
-               ast_debug(1, "Scrambled STUN packet length (got %d, expecting %d)\n", ntohs(hdr->msglen), (int)(len - sizeof(struct stun_header)));
+               ast_verbose("STUN Packet, msg %s (%04x), length: %d\n", stun_msg2str(ntohs(hdr->msgtype)), ntohs(hdr->msgtype), x);
+       if (x > len) {
+               ast_debug(1, "Scrambled STUN packet length (got %d, expecting %d)\n", x, (int)len);
        } else
-               len = ntohs(hdr->msglen);
-       data += sizeof(struct stun_header);
+               len = x;
        memset(&st, 0, sizeof(st));
        while (len) {
                if (len < sizeof(struct stun_attr)) {
@@ -507,29 +512,44 @@ static int stun_handle_packet(int s, struct sockaddr_in *src, unsigned char *dat
                        break;
                }
                attr = (struct stun_attr *)data;
-               if (ntohs(attr->len) > len) {
-                       ast_debug(1, "Inconsistent Attribute (length %d exceeds remaining msg len %d)\n", ntohs(attr->len), (int)len);
+               /* compute total attribute length */
+               x = ntohs(attr->len) + sizeof(struct stun_attr);
+               if (x > len) {
+                       ast_debug(1, "Inconsistent Attribute (length %d exceeds remaining msg len %d)\n", x, (int)len);
                        break;
                }
                if (stun_process_attr(&st, attr)) {
                        ast_debug(1, "Failed to handle attribute %s (%04x)\n", stun_attr2str(ntohs(attr->attr)), ntohs(attr->attr));
                        break;
                }
-               /* Clear attribute in case previous entry was a string */
+               /* Clear attribute id: in case previous entry was a string,
+                * this will act as the terminator for the string.
+                */
                attr->attr = 0;
-               data += ntohs(attr->len) + sizeof(struct stun_attr);
-               len -= ntohs(attr->len) + sizeof(struct stun_attr);
-       }
-       /* Null terminate any string */
+               data += x;
+               len -= x;
+       }
+       /* Null terminate any string.
+        * XXX NOTE, we write past the size of the buffer passed by the
+        * caller, so this is potentially dangerous. The only thing that
+        * saves us is that usually we read the incoming message in a
+        * much larger buffer in the struct ast_rtp
+        */
        *data = '\0';
-       resp = (struct stun_header *)respdata;
-       resplen = 0;
-       respleft = sizeof(respdata) - sizeof(struct stun_header);
-       resp->id = hdr->id;
-       resp->msgtype = 0;
-       resp->msglen = 0;
-       attr = (struct stun_attr *)resp->ies;
-       if (!len) {
+
+       /* Now prepare to generate a reply, which at the moment is done
+        * only for properly formed (len == 0) STUN_BINDREQ messages.
+        */
+       if (len == 0) {
+               unsigned char respdata[1024];
+               struct stun_header *resp = (struct stun_header *)respdata;
+               int resplen = 0;        /* len excluding header */
+               int respleft = sizeof(respdata) - sizeof(struct stun_header);
+
+               resp->id = hdr->id;
+               resp->msgtype = 0;
+               resp->msglen = 0;
+               attr = (struct stun_attr *)resp->ies;
                switch (ntohs(hdr->msgtype)) {
                case STUN_BINDREQ:
                        if (stundebug)