276845 - We need PL_strlcpy as replacement for PL_strcpy/PL_strncpy

Status: NEW

(NSPR :: NSPR, enhancement)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) (KHTML, like Gecko)
Build Identifier: 

- strcpy is unsafe 
- strncpy is also unsafe it does guarantee to terminate the string 
  when browsing the code you find (PL_)strncpy usage without explicit string 
termination 
- (PL_strcpy)/(PL_)strncpy do return a more or less useless pointer to the 
destination buffer. The length of the destination and the information if the 
destination is too short is both discarded. 
- the save replacement PR_snprintf() is slow 
 
So, I implemented PL_strlcpy which is fast and gives us the lengh of the source 
string 'for free'. And with this the possibility to quickly check for a 
truncation. This function is well known and heavily used in the BSD operating 
system. 
There is a function description in plstr.h. 
I also added test cases within tests/string.c 
 
Example usage: 
char buf[BUFSIZE]; 
PRUint32 src_len; 
 
src_len=PL_strlcpy(buf,src,sizeof(buf)); 
if (src_len >= sizeof(buf)) { 
  /* print out error and return  or  allocate a bigger buffer */ 
} 
Now, we could easily append another string without using 
(PL_)strcat/(PL_strncat) 
 
 
I found lot's of security issues / possible buffer overlows regarding 
strncpy/strcpy/strcat/strncat and would like to fix them using PL_strlcpy. 
 
As well I found a lot of strcpy/strncpy + strlen combinations which can be 
replaced by one strlcpy call. These are just performance issues but should also 
be done in the near future. 
 
I start fixing the found issues as soon this 'bug' is 'FIXED'. 
 

Reproducible: Always

Comment 1

[Tim Ruehsen]

Attachment: contains patches to plstr.h, strcpy.c, tests/string.c

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

note that NSPR has its own product

Comment 3

[Wan-Teh Chang]

Tim, thank you very much for the bug report and the
patch.

I am, however, concerned about adding a function
that's similiar to the existing PL_strncpyz function.
I know PL_strlcpy returns information that PL_strncpyz
doesn't return.  Do you know if there is any standardization
activity on making Standard C Library's str* functions
safe?  I want to make sure that the new function's
interface has received support from the experts.

Could you show me the Mozilla code where
strcpy/strncpy + strlen combinations can be 
replaced by one strlcpy call?  I think this would be the
main reason I would consider strlcpy is superior to
PL_strncpyz.

Comment 4

[Tim Ruehsen]

about standardization: It is a 'standard' for BSD programmers. See man page at 
http://www.rocketaware.com/man/man3/strlcpy.3.htm 
 
As far as I know, strlcpy is not a POSIX standard. That is why it is not 
included in the GNU glibc. 
 
You can find many discussions if you google for 'strlcpy'. 
 
Some examples: 
http://lxr.mozilla.org/mozilla1.7/source/embedding/tests/os2Embed/os2Embed.cpp#376 
Usage of PL_strncpyz without checking the length of the result! That means, 
truncation could occur and the result is still used for further processing. 
 
Same here: 
http://lxr.mozilla.org/mozilla1.7/source/embedding/tests/winEmbed/winEmbed.cpp#311 
http://lxr.mozilla.org/mozilla1.7/source/gfx/src/gtk/nsDeviceContextSpecG.cpp#407 
http://lxr.mozilla.org/mozilla1.7/source/ipc/ipcd/daemon/src/ipcdUnix.cpp#412 
http://lxr.mozilla.org/mozilla1.7/source/mailnews/base/util/nsMsgI18N.cpp#496 
http://lxr.mozilla.org/mozilla1.7/source/xpinstall/wizard/unix/src2/nsXIEngine.cpp#1053 
 

Comment 5

[Wan-Teh Chang]

Brendan, Brian, Darin, could you review the proposed
PL_strlcpy function?  Thanks.

Comment 6

[Wan-Teh Chang]

Attachment: contains patches to plstr.h, strcpy.c, tests/string.c

I got rid of the white space changes in the
original patch.

Comment 7

[Wan-Teh Chang]

Comment on attachment 177677 [details] [diff] [review]
contains patches to plstr.h, strcpy.c, tests/string.c

In PL_strlcpy, we have

>+    if ( !dst || !max ) {
>+        while ( *src_tmp++ );
>+        return src_tmp - src - 1;
>+    }

Can we simply do the following instead?

>+    if ( !dst || !max ) {
>+        return strlen(src);
>+    }

I found that OpenBSD also has strlcat.	Why didn't
you propose adding PL_strlcat?

Comment 8

[Wan-Teh Chang]

Attachment: patch after wtchang's editing

I made the following changes.

1. In plstr.h, I edited the comments.  I found a key
difference between PL_strncpyz and PL_strlcpy: they
not only differ in the return value but also in the
requirement on the source string.  For PL_strncpyz,
the source string doesn't need to be null-terminated.
For PL_strlcpy, the source string must be null-terminate
because the functions returns the length of the source
string.  Because of this key difference, replacing
PL_strncpyz by PL_strlcpy cannot be done mechanically,
so I removed the comment "should replace PL_strncpyz"
and added a comment to highlight this key difference.

2. I tried to emulate the (unusual) coding style of
the original author of the PL_str functions.  I also
made PL_strlcpy look as close to PL_strncpyz as
possible.  This makes it easier for me to convince
myself that the PL_strlcpy code is correct.

3. I added PL_strlcpy to plc.def and plc_symvec.opt.

4. No changes to the new test test_032 in string.c.

Comment 9

[Tim Ruehsen]

#7: I also thought about that. It is good to use strcpy() as you propose, since 
most libraries/compilers have a (assembler) hand-optimized routine for that. 
 
#8: I personally did not want to use the 'unusual' coding style. It makes the 
code less readable and there is the danger that the code spreads into other 
source codes/functions (as it did now). I would be convinced if you e.g. prove, 
that the PL_strncpyz code is somehow faster. But the reason 'I feel better' 
does not convince me to be happy with the old coding style. 
 

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.