critical bug in strncpy()
Hi walter,
I think the main problem is the fact that tmp++ keeps going when src++ stops, this can be fixed like this:
if ((*tmp = *src) != 0) { src++; tmp++; } <--- just adding brackets with that I think you have all bases covered, otherwise tmp keeps incrementing, which doesn't have any sense in this case.
The other issue is the fact that if strlen(src) > count you get a non null terminated string, but that's the way it has been made.
And the final issue afaik is the waste of cpu cycles when the strings have ended copying themselves but the count is still > 0, but that's not a security issue.
Vicente.
Quoted 8 lines · walter harms
int
main()
{
char *src="15" ;
char dst[]="123";
K_strncpy(dst,src,500);}
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
hi all,
The kernel version of strcpy() is buggy. it copies ALWAYS n bytes.
since i have NO idea if this is exploidable somehow, i would still
recomend to REMOVE the code. See example below if you do not believe me.
Also below you find a sniplet from libString/Strncpy.c.
i did not provide a patch since this is NOT my code. (btw: changed the name from Strncopy to strncpy). C&P this in to the kernel/lib/string.c.
re,
walter
/* in case this is needed */
/* linux kernel */
char * K_strncpy(char * dest,const char *src,size_t count)
{
char *tmp = dest;
while (count) {
if ((*tmp = *src) != 0) src++; <-MISSING case == 0
tmp++;
count--;
}
printf("count=%d\n",count);
return dest;
}int main()
{
char *src="15" ;
char dst[]="123";
K_strncpy(dst,src,500);
}
i looked a bit around and found these nice version.
/*
* libString, Copyright (C) 1999 Patrick Alken
* This library comes with absolutely NO WARRANTY
*
* Should you choose to use and/or modify this source code, please
* do so under the terms of the GNU General Public License under which
* this library is distributed.
*
* $Id: Strncpy.c,v 1.1.1.1 2000/10/02 12:02:27 decho Exp $ */
#include <sys/types.h>
/*Strncpy()
Optimized version of strncpy().
Inputs: dest - destination string
source - source string
bytes - number of bytes to copy
NOTE: A terminating \0 character is only copied to 'dest' if
'source' is terminated by one, provided the limit 'bytes' has not yet been reached.
Return: destination string
*/
char *strncpy(char *dest, const char *source, const size_t bytes)
{
register char *end = dest + bytes;
register char *s = dest;
while ((s < end) && (*s++ = *source++))
; return (dest);
} /* Strncpy() */
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 5 lines · walter harms
hi all,
The kernel version ofstrcpy()is buggy. it copies ALWAYS n bytes. since i have NO idea if this is exploidable somehow, i would still recomend to REMOVE the code. See example below if you do not believe me. Also below you find a sniplet fromlibString/Strncpy.c.
The kernel version prevents information leakage by overwriting the rest of the buffer with 0 after finishing the actual copy.
I'm fairly certain the behavior was intendend.
Quoted 88 lines
i did not provide a patch since this is NOT my code. (btw: changed the name from Strncopy to strncpy). C&P this in to the
kernel/lib/string.c.re,
walter/* in case this is needed */
Signed-off-by walter harms <[email protected]>/* linux kernel */
char *K_strncpy(char * dest,const char *src,size_t count){ char *tmp = dest; while (count) { if ((*tmp = *src) != 0) src++; <-MISSING case == 0 tmp++; count--; } printf("count=%d\n",count); return dest; }int
main()
{
char *src="15" ;
char dst[]="123";
K_strncpy(dst,src,500);}
i looked a bit around and found these nice version.
/*
* libString, Copyright (C) 1999 Patrick Alken
* This library comes with absolutely NO WARRANTY
*
* Should you choose to use and/or modify this source code, please
* do so under the terms of the GNU General Public License under which
* this library is distributed.
*
* $Id: Strncpy.c,v 1.1.1.1 2000/10/02 12:02:27 decho Exp $ */#include <
sys/types.h>/*
Strncpy()
Optimized version ofstrncpy().Inputs: dest - destination string
source - source string
bytes - number of bytes to copyNOTE: A terminating \0 character is only copied to 'dest' if
'source' is terminated by one, provided the limit 'bytes' has not yet been reached.Return: destination string
*/char *
strncpy(char *dest, const char *source, const size_t bytes){
register char *end = dest + bytes;
register char *s = dest;while ((s < end) && (*s++ = *source++)) ;return (dest);
} /*Strncpy()*/_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Signature · 3 lines
Ryan Anderson AutoWeb Communications, Inc. email: [email protected]
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
hi all,
i do not think this is intended. surely you copy data from
src to tmp++ but using a large value (> strlen(src) ) for count will access some strange areas where no code should go.
the defined behavier of strncpy is to copy a string that terminates with \0, count describes an upper limit what is reached first should terminate the copy. every version of strncpy works that way. maybe its not an critical bug (as src is not increased) but
1. it *will* access unintended areas beyond \0
2. rule of least surprise is violated
3. other ppl may find other uses to this bug
4. the fix is simple
re,
walter
Quoted 9 lines · Ryan Anderson
Quoted 5 lines · walter harms
hi all,
The kernel version ofstrcpy()is buggy. it copies ALWAYS n bytes. since i have NO idea if this is exploidable somehow, i would still recomend to REMOVE the code. See example below if you do not believe me. Also below you find a sniplet fromlibString/Strncpy.c.
Quoted 97 lines
The kernel version prevents information leakage by overwriting the rest of the buffer with 0 after finishing the actual copy.
I'm fairly certain the behavior was intendend.
Quoted 88 lines
i did not provide a patch since this is NOT my code. (btw: changed the name from Strncopy to strncpy). C&P this in to the
kernel/lib/string.c.re,
walter/* in case this is needed */
Signed-off-by walter harms <[email protected]>/* linux kernel */
char *K_strncpy(char * dest,const char *src,size_t count){ char *tmp = dest; while (count) { if ((*tmp = *src) != 0) src++; <-MISSING case == 0 tmp++; count--; } printf("count=%d\n",count); return dest; }int
main()
{
char *src="15" ;
char dst[]="123";
K_strncpy(dst,src,500);}
i looked a bit around and found these nice version.
/*
* libString, Copyright (C) 1999 Patrick Alken
* This library comes with absolutely NO WARRANTY
*
* Should you choose to use and/or modify this source code, please
* do so under the terms of the GNU General Public License under which
* this library is distributed.
*
* $Id: Strncpy.c,v 1.1.1.1 2000/10/02 12:02:27 decho Exp $ */#include <
sys/types.h>/*
Strncpy()
Optimized version ofstrncpy().Inputs: dest - destination string
source - source string
bytes - number of bytes to copyNOTE: A terminating \0 character is only copied to 'dest' if
'source' is terminated by one, provided the limit 'bytes' has not yet been reached.Return: destination string
*/char *
strncpy(char *dest, const char *source, const size_t bytes){
register char *end = dest + bytes;
register char *s = dest;while ((s < end) && (*s++ = *source++))
;return (dest);
} /*Strncpy()*/_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 5 lines · walter harms
hi all,
i do not think this is intended. surely you copy data from src to tmp++ but using a large value (> strlen(src) ) for count will access some strange areas where no code should go.
It's not a strange area, it's the rest of the destination buffer. The rest of the destination buffer is filled with the value 0. This guarantees that strncpy prevents any information leakage via this buffer. This has the nice side effect that a buffer can be reused without an explicit memset if you know that strncpy will be used to repopulate it between uses.
This is a security feature.
src is not accessed beyond the minimum of strlen(src) and n, so there is no chance of an out of bounds access on that, and the "n" that we are given is the limit to the buffer pointed at by dest, so there is no out of bounds access there.
Quoted 5 lines
the defined behavier of strncpy is to copy a string that terminates with \0, count describes an upper limit what is reached first should terminate the copy. every version of strncpy works that way. maybe its not an critical bug (as src is not increased) but
1. it *will* access unintended areas beyond \0
src is not accessed beyond the minimum of strlen(src) and n, so there is no chance of an out of bounds access on that, and the "n" that we are given is the limit to the buffer pointed at by dest, so there is no out of bounds access there.
Quoted 1 line
2. rule of least surprise is violated
Well, I can't disagree here - but I don't think the surprise is in any way detrimental. There is simply no bug to exploit here.
Quoted 2 lines
3. other ppl may find other uses to this bug
4. the fix is simple
The fix opens a hole for potential information leakage to user space.
Have you audited all callers of strncpy to ensure that they do not implicitly use this clearing of the buffer to guarantee that information is not leaked to userspace?
Quoted 117 lines
re,
walterQuoted 9 lines · Ryan Anderson
Quoted 5 lines · walter harms
hi all,
The kernel version ofstrcpy()is buggy. it copies ALWAYS n bytes. since i have NO idea if this is exploidable somehow, i would still recomend to REMOVE the code. See example below if you do not believe me. Also below you find a sniplet fromlibString/Strncpy.c.Quoted 98 lines
The kernel version prevents information leakage by overwriting the rest of the buffer with 0 after finishing the actual copy.
I'm fairly certain the behavior was intendend.
Quoted 89 lines
i did not provide a patch since this is NOT my code. (btw: changed the name from Strncopy to strncpy). C&P this in to the
kernel/lib/string.c.re,
walter/* in case this is needed */
Signed-off-by walter harms <[email protected]>/* linux kernel */
char *K_strncpy(char * dest,const char *src,size_t count){ char *tmp = dest; while (count) { if ((*tmp = *src) != 0) src++; <-MISSING case == 0 tmp++; count--; } printf("count=%d\n",count); return dest; }int
main()
{
char *src="15" ;
char dst[]="123";
K_strncpy(dst,src,500);}
i looked a bit around and found these nice version.
/*
* libString, Copyright (C) 1999 Patrick Alken
* This library comes with absolutely NO WARRANTY
*
* Should you choose to use and/or modify this source code, please
* do so under the terms of the GNU General Public License under which
* this library is distributed.
*
* $Id: Strncpy.c,v 1.1.1.1 2000/10/02 12:02:27 decho Exp $ */#include <
sys/types.h>/*
Strncpy()
Optimized version ofstrncpy().Inputs: dest - destination string
source - source string
bytes - number of bytes to copyNOTE: A terminating \0 character is only copied to 'dest' if
'source' is terminated by one, provided the limit 'bytes' has not yet been reached.Return: destination string
*/char *
strncpy(char *dest, const char *source, const size_t bytes){
register char *end = dest + bytes;
register char *s = dest;while ((s < end) && (*s++ = *source++))
;return (dest);
} /*Strncpy()*/_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Signature · 3 lines
Ryan Anderson AutoWeb Communications, Inc. email: [email protected]
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 45 lines · Ryan Anderson
Quoted 4 lines · walter harms
hi all,
i do not think this is intended. surely you copy data from
src to tmp++ but using a large value (> strlen(src) ) for count will access some strange areas where no code should go.It's not a strange area, it's the rest of the destination buffer. The rest of the destination buffer is filled with the value 0. This guarantees that strncpy prevents any information leakage via this buffer. This has the nice side effect that a buffer can be reused without an explicit memset if you know that strncpy will be used to repopulate it between uses.
This is a security feature.
src is not accessed beyond the minimum of strlen(src) and n, so there is no chance of an out of bounds access on that, and the "n" that we are given is the limit to the buffer pointed at by dest, so there is no out of bounds access there.
Quoted 3 lines
the defined behavier of strncpy is to copy a string that terminates with \0, count describes an upper limit what is reached first should terminate the copy. every version of strncpy works that way. maybe its not an critical bug (as src is not increased) but
1. it *will* access unintended areas beyond \0src is not accessed beyond the minimum of strlen(src) and n, so there is no chance of an out of bounds access on that, and the "n" that we are given is the limit to the buffer pointed at by dest, so there is no out of bounds access there.
Quoted 1 line
2. rule of least surprise is violated
Well, I can't disagree here - but I don't think the surprise is in any way detrimental. There is simply no bug to exploit here.
Quoted 2 lines
3. other ppl may find other uses to this bug
4. the fix is simple
Quoted 6 lines
The fix opens a hole for potential information leakage to user space.
Have you audited all callers of strncpy to ensure that they do not implicitly use this clearing of the buffer to guarantee that information is not leaked to userspace?
you are right *when* n is the size of the destination buffer. but take a look at this time of code from ./net/irda/ircomm/ircomm_param.c:strncpy(self->settings.port_name, param->pv.c, 32);
if works fine IF port_name[33] is true. but then he can use strcpy(). why does user use actualy strNxxx() because the limit will prevent access beyond \0 and at least n. clearing should be done within strncpy (if you want that) using a clear *foo++=0 so its visible
look at the asm code for m68k (i can not read i386, that the reason :) they stop coppying at \0 and n.
the main problem with strncpy is still:
it has the same name as the POSIX cousin but it does not behave equal.
it does not terminate the dststring automaticly with \0 (neither does POSIX, an error IMHO,strlcpy does).
it will access the dstbuffer beyond \0 if n is large enought, ok it will fill it with \0 but who many people will be aware of it ? i admit falling into a trap when expecting the same behavier as POSIX strncpy() but who will not ? exspecialy if its not documented.
re,
walter
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
There is no bug in lib/string.c's implementation of strncpy(). Most people do not realise that strncpy is a stupid function that is almost impossible to use correctly. Read the manpage:
The strncpy() function is similar, except that not more than n bytes of src are copied. Thus, if there is no null byte among the first n bytes of src, the result will not be null-terminated.
In the case where the length of src is less than that of n, the remain- der of dest will be padded with nulls.
This agrees with POSIX, ANSI C and Single Unix, fwiw.
Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors