[SR-Users] [PATCH] modules_k/uac: fix from/to restore for small original URI

Timo Teräs timo.teras at iki.fi
Wed Apr 6 08:45:06 CEST 2011


Seems that the URI length check is superfluous and fails under
certain conditions. It does not make sense for the URI to have
zero bytes, so just use the first seen zero byte as end marker.

I have a reproducible test case where the restore inserts URI
with multiple zero-bytes to wire. This happens if the original
URI is smaller than the one we rewrote it to using uac_replace_from.

Signed-off-by: Timo Teräs <timo.teras at iki.fi>
---
 modules_k/uac/from.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

However, I think the delta encoding used for the RR attribute
is flawed. Hostile remote server could rewrite the RR attribute
and/or From/To headers in a way to forge it to something it was not
in the first place. Additionally the delta-encoded RR attribute
breaks if the From/To header isn't exact copy of what we sent.

Would it not make more sense to just send the real original
header (possibly encrypted) but with a checksum? We could then
verify if someone had clobbered the RR attribute and ignore it.
And we could always restore the original URI even if the URI
we are swapping was modified unexpectedly.

diff --git a/modules_k/uac/from.c b/modules_k/uac/from.c
index 4657e11..50822b6 100644
--- a/modules_k/uac/from.c
+++ b/modules_k/uac/from.c
@@ -463,15 +463,17 @@ int restore_from( struct sip_msg *msg, int *is_from )
 		LM_ERR("new URI shorter than old URI\n");
 		goto failed;
 	}
-	for( i=0 ; i<old_uri.len ; i++ )
+	for( i=0 ; i<old_uri.len ; i++ ) {
 		new_uri.s[i] ^= old_uri.s[i];
-	if (new_uri.len==old_uri.len) {
-		for( ; new_uri.len && (new_uri.s[new_uri.len-1]==0) ; new_uri.len-- );
-		if (new_uri.len==0) {
-			LM_ERR("new URI got 0 len\n");
-			goto failed;
+		if (new_uri.s[i] == 0) {
+			new_uri.len = i;
+			break;
 		}
 	}
+	if (new_uri.len==0) {
+		LM_ERR("new URI got 0 len\n");
+		goto failed;
+	}
 
 	LM_DBG("decoded uris are: new=[%.*s] old=[%.*s]\n",
 		new_uri.len, new_uri.s, old_uri.len, old_uri.s);
-- 
1.7.1




More information about the sr-users mailing list