David Rankin
2000-Jan-19 07:39 UTC
Potentially serious (but rare) issue with buffer.c and cipher.c
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;
}
Michael H. Warfield
2000-Jan-19 14:47 UTC
Potentially serious (but rare) issue with buffer.c and cipher.c
Wait a minute here... On Wed, Jan 19, 2000 at 02:39:18AM -0500, David Rankin wrote:> 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.Check me on this... The danger in those routines has always been overlaping regions. If the source and destination do not overlap, you are safe, of course. If the source and destination are the same, you are also safe since you are setting memory back to its original value. Length limited functions such as memcpy were never in danger of runaway conditions like strcpy was, but they could have unpredictable results in memory if the source and destination where overlaping and non-congruent (not identical). If the src and dest can be equal, I don't see the problem. If they can be non-equal but overlapping, then we have a potential problem.> 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.I didn't look at it real close but it didn't look like it did any harm. Is it going to cause any platform support and compatibility issues?> 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. BernardMike -- Michael H. Warfield | (770) 985-6132 | mhw at WittsEnd.com (The Mad Wizard) | (770) 331-2437 | http://www.wittsend.com/mhw/ NIC whois: MHW9 | An optimist believes we live in the best of all PGP Key: 0xDF1DD471 | possible worlds. A pessimist is sure of it!