Re: [KJ] critical bug in strncpy()

Re: [KJ] critical bug in strncpy()

[Vicente Feito]

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.


On Monday 28 March 2005 02:34 pm, walter harms wrote:
> int main()
> {
> char *src="15" ;
> char dst[]="123";
>
> K_strncpy(dst,src,500);
>
> }
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] critical bug in strncpy()

[walter harms]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

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 */
Signed-off-by: walter harms <wharms@bfs.de>



/* 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() */



[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] critical bug in strncpy()

[Ryan Anderson]

[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]

On Mon, Mar 28, 2005 at 04:34:16PM +0200, walter harms wrote:
> 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.

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.

> 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 <wharms@bfs.de>
> 
> 
> 
> /* 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
> Kernel-janitors@lists.osdl.org
> http://lists.osdl.org/mailman/listinfo/kernel-janitors


-- 

Ryan Anderson
AutoWeb Communications, Inc.
email: ryan@autoweb.net



[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] critical bug in strncpy()

[walter harms]

[-- Attachment #1: Type: text/plain, Size: 3154 bytes --]

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


Ryan Anderson wrote:
> On Mon, Mar 28, 2005 at 04:34:16PM +0200, walter harms wrote:
> 
>>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.
> 
> 
> 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.
> 
> 
>>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 <wharms@bfs.de>
>>
>>
>>
>>/* 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
>>Kernel-janitors@lists.osdl.org
>>http://lists.osdl.org/mailman/listinfo/kernel-janitors
> 
> 
> 

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] critical bug in strncpy()

[Ryan Anderson]

[-- Attachment #1.1: Type: text/plain, Size: 5119 bytes --]

On Mon, 2005-03-28 at 17:31 +0200, walter harms wrote:
> 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.

> 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.

> 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.

> 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?

> 
> re,
> 	walter
> 
> 
> Ryan Anderson wrote:
> > On Mon, Mar 28, 2005 at 04:34:16PM +0200, walter harms wrote:
> > 
> >>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.
> > 
> > 
> > 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.
> > 
> > 
> >>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 <wharms@bfs.de>
> >>
> >>
> >>
> >>/* 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
> >>Kernel-janitors@lists.osdl.org
> >>http://lists.osdl.org/mailman/listinfo/kernel-janitors
> > 
> > 
> > 
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> http://lists.osdl.org/mailman/listinfo/kernel-janitors
-- 
Ryan Anderson
AutoWeb Communications, Inc.
email: ryan@autoweb.net


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] critical bug in strncpy()

[walter harms]

[-- Attachment #1: Type: text/plain, Size: 3116 bytes --]



Ryan Anderson wrote:
> On Mon, 2005-03-28 at 17:31 +0200, walter harms wrote:
> 
>>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.
> 
> 
>>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.
> 
> 
>>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.
> 
> 
>>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?
> 

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







[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] critical bug in strncpy()

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]


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.

-- 
"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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors