thr3ads.net - rsync - Possible security hole [Oct 2003]

If this information is useful, please help other people find it:
Share via:

Timo Sirainen

2003-Oct-05 06:38 UTC

Possible security hole

Maybe security related mails should be sent elsewhere? I didn't notice 
any so here it goes:

sender.c:receive_sums()

	s->count = read_int(f);
..
	s->sums = (struct sum_buf *)malloc(sizeof(s->sums[0])*s->count);
	if (!s->sums) out_of_memory("receive_sums");

	for (i=0; i < (int) s->count;i++) {
		s->sums[i].sum1 = read_int(f);
		read_buf(f,s->sums[i].sum2,csum_length);
..

If I read this right, given high enough s->count makes the malloc() 
parameter wraps around to a few bytes while still reading data past it.

Exploiting is probably pretty difficult if at all possible. There would 
have to be some interesting data after the returned pointer. I didn't 
check if there is. Anyway, exit_cleanup() leads to at least a few 
free() calls which could make it exploitable.

I think there's also some potential problems with 64bit systems. 
There's a few arrays that assume it's size will fit into int. Normally 
you'd get out of memory before getting that high, but in 64bit systems 
with enough memory you won't. Of course this also requires sending 
gigabytes of data, but internet is fast nowadays.

jw schultz

2003-Oct-05 07:13 UTC

head link

Possible security hole

On Sat, Oct 04, 2003 at 11:38:48PM +0300, Timo Sirainen
wrote:> Maybe security related mails should be sent elsewhere? I didn't notice 
> any so here it goes:
> 
> sender.c:receive_sums()
> 
> 	s->count = read_int(f);
> ..
> 	s->sums = (struct sum_buf *)malloc(sizeof(s->sums[0])*s->count);
> 	if (!s->sums) out_of_memory("receive_sums");
> 
> 	for (i=0; i < (int) s->count;i++) {
> 		s->sums[i].sum1 = read_int(f);
> 		read_buf(f,s->sums[i].sum2,csum_length);
> ..
> 
> If I read this right, given high enough s->count makes the malloc() 
> parameter wraps around to a few bytes while still reading data past it.
You read that correctly.  In order to force that to happen
stat would have to report the file as being 21,743,270,000TB
in size or a fixed block-size would have to be specified
such that file-size/block-size > 119,304,600.

A sanity check wouldn't hurt.  Doing so would protect
servers from nuts spoofing an rsync client doing a send to
an rsync server where an admin was foolish enough to allow
public writes.  Also might make sense to do a double check
in generator.c where the values are calculated in the first
place.
> Exploiting is probably pretty difficult if at all possible. There would 
> have to be some interesting data after the returned pointer. I didn't 
> check if there is. Anyway, exit_cleanup() leads to at least a few 
> free() calls which could make it exploitable.
> 
> I think there's also some potential problems with 64bit systems. 
> There's a few arrays that assume it's size will fit into int.
Normally
> you'd get out of memory before getting that high, but in 64bit systems 
> with enough memory you won't. Of course this also requires sending 
> gigabytes of data, but internet is fast nowadays.
If you care to do an audit, ok.  I don't recall any such
arrays where the array member count would not fit into an
int. 

Do include tested patches in your reports.


-- 
________________________________________________________________
	J.W. Schultz            Pegasystems Technologies
	email address:		jw@pegasys.ws

		Remember Cernan and Schmitt

Wayne Davison

2003-Oct-05 09:56 UTC

head link

Possible security hole

On Sat, Oct 04, 2003 at 11:38:48PM +0300, Timo Sirainen
wrote:> 	for (i=0; i < (int) s->count;i++) {
Yeah, that's pretty bad.  Attached is a patch that should fix this and a
number of other related problems where the code assumed that size_t
would fit into an int.  There looks to be a bunch more to do, though.
For instance, a couple months ago I was starting to look at variable
sizes after I had noticed that this struct had a bunch of size_t values
in it that were being stored into int variables for processing:

struct sum_struct {
       OFF_T flength;          /**< total file length */
       size_t count;           /**< how many chunks */
       size_t remainder;       /**< flength % block_length */
       size_t blength;         /**< block_length */
       size_t s2length;        /**< sum2_length */
       struct sum_buf *sums;   /**< points to info for each chunk */
};

The problem variables are remainder, block_length, and s2length.  My
initial reaction was to resize these three values in the struct to be
"int"s, but I haven't verified that they're really guaranteed
to fit.

Anyway, this patch was prepared pretty quickly, so if anyone notices
something wrong with it, let me know.

..wayne..
-------------- next part --------------
Index: checksum.c
--- checksum.c	9 Sep 2003 15:58:48 -0000	1.27
+++ checksum.c	4 Oct 2003 23:41:39 -0000
@@ -30,9 +30,9 @@
   a simple 32 bit checksum that can be upadted from either end
   (inspired by Mark Adler's Adler-32 checksum)
   */
-uint32 get_checksum1(char *buf1,int len)
+uint32 get_checksum1(char *buf1, size_t len)
 {
-    int i;
+    size_t i;
     uint32 s1, s2;
     schar *buf = (schar *)buf1;
 
@@ -49,11 +49,11 @@
 }
 
 
-void get_checksum2(char *buf,int len,char *sum)
+void get_checksum2(char *buf, size_t len, char *sum)
 {
-	int i;
+	size_t i;
 	static char *buf1;
-	static int len1;
+	static size_t len1;
 	struct mdfour m;
 
 	if (len > len1) {
@@ -131,7 +131,7 @@
 }
 
 
-static int sumresidue;
+static size_t sumresidue;
 static char sumrbuf[CSUM_CHUNK];
 static struct mdfour md;
 
@@ -152,9 +152,9 @@
  * @todo Perhaps get rid of md and just pass in the address each time.
  * Very slightly clearer and slower.
  **/
-void sum_update(char *p, int len)
+void sum_update(char *p, size_t len)
 {
-	int i;
+	size_t i;
 	if (len + sumresidue < CSUM_CHUNK) {
 		memcpy(sumrbuf+sumresidue, p, len);
 		sumresidue += len;
Index: generator.c
--- generator.c	11 Sep 2003 04:53:05 -0000	1.57
+++ generator.c	4 Oct 2003 23:41:40 -0000
@@ -192,10 +192,10 @@
 	sum->remainder	= (len % blength);
 
 	if (sum->count && verbose > 2) {
-		rprintf(FINFO, "count=%ld rem=%ld blength=%ld s2length=%ld
flength=%.0f\n",
-			(long) sum->count, (long) sum->remainder,
-			(long) sum->blength, (long) sum->s2length,
-			(double) sum->flength);
+		rprintf(FINFO, "count=%.0f rem=%.0f blength=%.0f s2length=%.0f
flength=%.0f\n",
+			(double)sum->count, (double)sum->remainder,
+			(double)sum->blength, (double)sum->s2length,
+			(double)sum->flength);
 	}
 }
 
@@ -227,7 +227,7 @@
  *
  * Generate approximately one checksum every block_len bytes.
  */
-static void generate_and_send_sums(struct map_struct *buf, OFF_T len, int
f_out)
+static void generate_and_send_sums(struct map_struct *buf, size_t len, int
f_out)
 {
 	size_t i;
 	struct sum_struct sum;
@@ -238,7 +238,7 @@
 	write_sum_head(f_out, &sum);
 
 	for (i = 0; i < sum.count; i++) {
-		int n1 = MIN(len, sum.blength);
+		size_t n1 = MIN(len, sum.blength);
 		char *map = map_ptr(buf, offset, n1);
 		uint32 sum1 = get_checksum1(map, n1);
 		char sum2[SUM_LENGTH];
@@ -247,8 +247,8 @@
 
 		if (verbose > 3) {
 			rprintf(FINFO,
-				"chunk[%ld] offset=%.0f len=%d sum1=%08lx\n",
-				(long)i,(double)offset,n1,(unsigned long)sum1);
+				"chunk[%.0f] offset=%.0f len=%.0f sum1=%08lx\n",
+				(double)i,(double)offset,(double)n1,(unsigned long)sum1);
 		}
 		write_int(f_out, sum1);
 		write_buf(f_out, sum2, sum.s2length);
Index: match.c
--- match.c	22 Aug 2003 21:26:08 -0000	1.56
+++ match.c	4 Oct 2003 23:41:40 -0000
@@ -26,7 +26,7 @@
 typedef unsigned short tag;
 
 #define TABLESIZE (1<<16)
-#define NULL_TAG (-1)
+#define NULL_TAG ((size_t)-1)
 
 static int false_alarms;
 static int tag_hits;
@@ -41,12 +41,12 @@
 
 struct target {
 	tag t;
-	int i;
+	size_t i;
 };
 
 static struct target *targets;
 
-static int *tag_table;
+static size_t *tag_table;
 
 #define gettag2(s1,s2) (((s1) + (s2)) & 0xFFFF)
 #define gettag(sum) gettag2((sum)&0xFFFF,(sum)>>16)
@@ -59,16 +59,16 @@
 
 static void build_hash_table(struct sum_struct *s)
 {
-	int i;
+	size_t i;
 
 	if (!tag_table)
-		tag_table = (int *)malloc(sizeof(tag_table[0])*TABLESIZE);
+		tag_table = (size_t*)malloc(sizeof(tag_table[0])*TABLESIZE);
 
 	targets = (struct target *)malloc(sizeof(targets[0])*s->count);
 	if (!tag_table || !targets)
 		out_of_memory("build_hash_table");
 
-	for (i = 0; i < (int)s->count; i++) {
+	for (i = 0; i < s->count; i++) {
 		targets[i].i = i;
 		targets[i].t = gettag(s->sums[i].sum1);
 	}
@@ -78,7 +78,7 @@
 	for (i = 0; i < TABLESIZE; i++)
 		tag_table[i] = NULL_TAG;
 
-	for (i = s->count-1; i >= 0; i--)
+	for (i = s->count; i-- > 0; )
 		tag_table[targets[i].t] = i;
 }
 
@@ -138,15 +138,15 @@
 static void hash_search(int f,struct sum_struct *s,
 			struct map_struct *buf,OFF_T len)
 {
-	OFF_T offset, end;
-	int j,k, last_i;
+	OFF_T k, offset, end;
+	size_t last_i;
 	char sum2[SUM_LENGTH];
 	uint32 s1, s2, sum;
 	schar *map;
 
 	/* last_i is used to encourage adjacent matches, allowing the RLL coding of
the
 	   output to work more efficiently */
-	last_i = -1;
+	last_i = (size_t)-1;
 
 	if (verbose > 2)
 		rprintf(FINFO,"hash search b=%ld len=%.0f\n",
@@ -160,7 +160,7 @@
 	s1 = sum & 0xFFFF;
 	s2 = sum >> 16;
 	if (verbose > 3)
-		rprintf(FINFO, "sum=%.8x k=%d\n", sum, k);
+		rprintf(FINFO, "sum=%.8x k=%.0f\n", sum, (double)k);
 
 	offset = 0;
 
@@ -173,19 +173,19 @@
 	do {
 		tag t = gettag2(s1,s2);
 		int done_csum2 = 0;
+		size_t j = tag_table[t];
 
-		j = tag_table[t];
 		if (verbose > 4)
 			rprintf(FINFO,"offset=%.0f sum=%08x\n",(double)offset,sum);
 
-		if (j == NULL_TAG) {
+		if (j == NULL_TAG)
 			goto null_tag;
-		}
 
 		sum = (s1 & 0xffff) | (s2 << 16);
 		tag_hits++;
-		for (; j < (int) s->count && targets[j].t == t; j++) {
-			int l, i = targets[j].i;
+		for (; j < s->count && targets[j].t == t; j++) {
+			int l;
+			size_t i = targets[j].i;
 
 			if (sum != s->sums[i].sum1) continue;
 
@@ -194,8 +194,8 @@
 			if (l != s->sums[i].len) continue;
 
 			if (verbose > 3)
-				rprintf(FINFO,"potential match at %.0f target=%d %d sum=%08x\n",
-					(double)offset,j,i,sum);
+				rprintf(FINFO,"potential match at %.0f target=%.0f %.0f
sum=%08x\n",
+					(double)offset,(double)j,(double)i,sum);
 
 			if (!done_csum2) {
 				map = (schar *)map_ptr(buf,offset,l);
@@ -210,8 +210,8 @@
 
 			/* we've found a match, but now check to see
 			 * if last_i can hint at a better match */
-			for (j++; j < (int) s->count && targets[j].t == t; j++) {
-				int i2 = targets[j].i;
+			for (j++; j < s->count && targets[j].t == t; j++) {
+				size_t i2 = targets[j].i;
 				if (i2 == last_i + 1) {
 					if (sum != s->sums[i2].sum1) break;
 					if (memcmp(sum2,s->sums[i2].sum2,s->s2length) != 0) break;
Index: proto.h
--- proto.h	16 Sep 2003 02:49:59 -0000	1.160
+++ proto.h	4 Oct 2003 23:41:40 -0000
@@ -24,11 +24,11 @@
 int read_batch_delta_file(char *buff, int len);
 void show_flist(int index, struct file_struct **fptr);
 void show_argvs(int argc, char *argv[]);
-uint32 get_checksum1(char *buf1,int len);
-void get_checksum2(char *buf,int len,char *sum);
+uint32 get_checksum1(char *buf1, size_t len);
+void get_checksum2(char *buf, size_t len, char *sum);
 void file_checksum(char *fname,char *sum,OFF_T size);
 void sum_init(void);
-void sum_update(char *p, int len);
+void sum_update(char *p, size_t len);
 void sum_end(char *sum);
 void close_all(void);
 void _exit_cleanup(int code, const char *file, int line);

Reasonably Related Threads

Search for more maybe matching threads

rsync - Oct 2003 - Possible security hole

Possible security hole

Possible security hole

Possible security hole

Reasonably Related Threads