Potentially serious (but rare) issue with buffer.c and cipher.c

[David Rankin]

While rototilling packet.c, I did some looking at cipher_encrypt in
cipher.c. It ends up that for SSH_CIPHER_NONE in cipher_encrypt, it
uses memcpy. However, it also appears that dest and src can be equal
in cipher_encrypt.

On most sane libc implementations, memcpy == memmove. However, ANSI C
makes no such guarantee, and some implementations out there are bound
to try to optimize memcpy eventually.

Therefore, I've hacked out the following patch to change what may be
"dangerous" memcpy's to memmove. Take a look and see what you think.

Thanks,
David

-- 
David W. Rankin, Jr.     Husband, Father, and UNIX Sysadmin. 
   Email: drankin at bohemians.lexington.ky.us   Address/Phone Number: Ask me.
"It is no great thing to be humble when you are brought low; but to be humble
when you are praised is a great and rare accomplishment." St. Bernard
-------------- next part --------------
Index: cipher.c
===================================================================
RCS file: /usr/local/cvs/openssh/cipher.c,v
retrieving revision 1.7
diff -u -r1.7 cipher.c
--- cipher.c	2000/01/17 17:27:31	1.7
+++ cipher.c	2000/01/19 07:18:49
@@ -45,16 +45,16 @@
 {
 	des_cblock iv1;
 
-	memcpy(&iv1, iv2, 8);
+	memmove(&iv1, iv2, 8);
 
 	des_cbc_encrypt(src, dest, len, ks1, &iv1, DES_ENCRYPT);
-	memcpy(&iv1, (char *)dest + len - 8, 8);
+	memmove(&iv1, (char *)dest + len - 8, 8);
 
 	des_cbc_encrypt(dest, dest, len, ks2, iv2, DES_DECRYPT);
-	memcpy(iv2, &iv1, 8);	/* Note how iv1 == iv2 on entry and exit. */
+	memmove(iv2, &iv1, 8);	/* Note how iv1 == iv2 on entry and exit. */
 
 	des_cbc_encrypt(dest, dest, len, ks3, iv3, DES_ENCRYPT);
-	memcpy(iv3, (char *)dest + len - 8, 8);
+	memmove(iv3, (char *)dest + len - 8, 8);
 }
 
 void
@@ -66,16 +66,16 @@
 {
 	des_cblock iv1;
 
-	memcpy(&iv1, iv2, 8);
+	memmove(&iv1, iv2, 8);
 
 	des_cbc_encrypt(src, dest, len, ks3, iv3, DES_DECRYPT);
-	memcpy(iv3, (char *)src + len - 8, 8);
+	memmove(iv3, (char *)src + len - 8, 8);
 
 	des_cbc_encrypt(dest, dest, len, ks2, iv2, DES_ENCRYPT);
-	memcpy(iv2, (char *)dest + len - 8, 8);
+	memmove(iv2, (char *)dest + len - 8, 8);
 
 	des_cbc_encrypt(dest, dest, len, ks1, &iv1, DES_DECRYPT);
-	/* memcpy(&iv1, iv2, 8); */
+	/* memmove(&iv1, iv2, 8); */
 	/* Note how iv1 == iv2 on entry and exit. */
 }
 
@@ -214,7 +214,7 @@
 	/* Get 32 bytes of key data.  Pad if necessary.  (So that code
 	   below does not need to worry about key size). */
 	memset(padded, 0, sizeof(padded));
-	memcpy(padded, key, keylen < sizeof(padded) ? keylen : sizeof(padded));
+	memmove(padded, key, keylen < sizeof(padded) ? keylen : sizeof(padded));
 
 	/* Initialize the initialization vector. */
 	switch (cipher) {
@@ -265,7 +265,7 @@
 
 	switch (context->type) {
 	case SSH_CIPHER_NONE:
-		memcpy(dest, src, len);
+		memmove(dest, src, len);
 		break;
 
 	case SSH_CIPHER_3DES:
@@ -299,7 +299,7 @@
 
 	switch (context->type) {
 	case SSH_CIPHER_NONE:
-		memcpy(dest, src, len);
+		memmove(dest, src, len);
 		break;
 
 	case SSH_CIPHER_3DES:
Index: buffer.c
===================================================================
RCS file: /usr/local/cvs/openssh/buffer.c,v
retrieving revision 1.2
diff -u -r1.2 buffer.c
--- buffer.c	2000/01/17 16:52:52	1.2
+++ buffer.c	2000/01/19 07:17:46
@@ -59,7 +59,7 @@
 {
 	char *cp;
 	buffer_append_space(buffer, &cp, len);
-	memcpy(cp, data, len);
+	memmove(cp, data, len);
 }
 
 /*
@@ -115,7 +115,7 @@
 {
 	if (len > buffer->end - buffer->offset)
 		fatal("buffer_get trying to get more bytes than in buffer");
-	memcpy(buf, buffer->buf + buffer->offset, len);
+	memmove(buf, buffer->buf + buffer->offset, len);
 	buffer->offset += len;
 }