[asterisk-commits] murf: trunk r112874 - /trunk/channels/chan_sip.c

[SVN commits to the Asterisk project]

Author: murf
Date: Fri Apr  4 20:33:13 2008
New Revision: 112874

URL: http://svn.digium.com/view/asterisk?view=rev&rev=112874
Log:
Found a little problem with the sip request handling that could lead to a quick crash of asterisk, and a road to a DOS attack if left unfixed.

Attaching to a running asterisk with "telnet hostname 5060", I would input "something", then hit return three times, and asterisk crashes.

I traced it to handle_request_do(), which zeroes out the data (an ast_str ptr) if the string is too short. 
Instead of freeing the struct and nulling the pointer, it now just resets it, because this 
ast_str is expected by the calling routine to still be there after handle_request_do() returns.

This appears to fix the crash. I assume that it was introduced with ast_str's being adopted.  It's a subtle and easy-to-miss sort of problem.

I also found all the places where the req.data is freed, and made sure the ptr is Nulled out as well; 
no good leaving bad ptrs laying around-- I didn't need to do this, but it seemed a good thing to do...



Modified:
    trunk/channels/chan_sip.c

Modified: trunk/channels/chan_sip.c
URL: http://svn.digium.com/view/asterisk/trunk/channels/chan_sip.c?view=diff&rev=112874&r1=112873&r2=112874
==============================================================================
--- trunk/channels/chan_sip.c (original)
+++ trunk/channels/chan_sip.c Fri Apr  4 20:33:13 2008
@@ -2252,8 +2252,11 @@
 	ser = ast_tcptls_session_instance_destroy(ser);
 	if (reqcpy.data)
 		ast_free(reqcpy.data);
-	if (req.data)
+	if (req.data) {
 		ast_free(req.data);
+		req.data = NULL;
+	}
+	
 
 	if (req.socket.lock) {
 		ast_mutex_destroy(req.socket.lock);
@@ -18129,8 +18132,10 @@
 	req.socket.lock = NULL;
 
 	handle_request_do(&req, &sin);
-	if (req.data)
+	if (req.data) {
 		ast_free(req.data);
+		req.data = NULL;
+	}
 
 	return 1;
 }
@@ -18159,8 +18164,7 @@
 		ast_verbose("--- (%d headers %d lines)%s ---\n", req->headers, req->lines, (req->headers + req->lines == 0) ? " Nat keepalive" : "");
 
 	if (req->headers < 2) {	/* Must have at least two headers */
-		ast_free(req->data);
-		req->data = NULL;
+		ast_str_reset(req->data); /* nulling this out is NOT a good idea here. */
 		return 1;
 	}