Possible security hole
Wayne Davison
wayned at samba.org
Sun Oct 5 09:56:16 EST 2003
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);
More information about the rsync
mailing list