Bug 170994 – Stop using strcpy() in WebKit::EnvironmentUtilities::stripValuesEndingWithString()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Kilzer (:ddkilzer)]

WebKit::EnvironmentUtilities::stripValuesEndingWithString() uses strcpy(), which causes static analysis and other tools to warn of its use.

In this case, the contents of a const char* buffer is only ever being shrunk, but we can switch to use strlcpy() instead (since EnvironmentUtilities.cpp is only used on Darwin-based platforms).

[David Kilzer (:ddkilzer)]

<rdar://problem/29889932>

[David Kilzer (:ddkilzer)]

Created attachment 307482 [details]
Patch

[Brent Fulgham]

Comment on attachment 307482 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307482&action=review

This looks great! But since I'm greedy, what about replacing instances of "strstr" in that method with "strnstr"?

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:46
> +    size_t environmentValueLength = strlen(environmentValue) + 1;

What about adding some "ASSERT_WITH_SECURITY_IMPLICATIONS" that our later string iteration does not exceed this amount? (e.g., the various 'match' results?)

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:77
> +            strlcpy(componentStart, match + searchLength, environmentValueLength - (componentStart - environmentValue));

I think this is good, but we should also fix "strstr" -> "strnstr"

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:85
>          match = strstr(componentStart, searchValueWithColon);

I think these should be "strnstr"

[David Kilzer (:ddkilzer)]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 307482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307482&action=review
> 
> This looks great! But since I'm greedy, what about replacing instances of
> "strstr" in that method with "strnstr"?

There is no security issue with strstr() as long as the string is NULL-terminated, but I can switch to strnstr().  However, note the compatibility issues from the strnstr(1) manpage:

     The strnstr() function locates the first occurrence of the null-terminated string needle in the string
     haystack, where not more than len characters are searched.  Characters that appear after a `\0' charac-
     ter are not searched.  Since the strnstr() function is a FreeBSD specific API, it should only be used
     when portability is not a concern.

> > Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:46
> > +    size_t environmentValueLength = strlen(environmentValue) + 1;
> 
> What about adding some "ASSERT_WITH_SECURITY_IMPLICATIONS" that our later
> string iteration does not exceed this amount? (e.g., the various 'match'
> results?)

Good idea.  I'll add these.

[Brent Fulgham]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> There is no security issue with strstr() as long as the string is
> NULL-terminated, but I can switch to strnstr().  However, note the
> compatibility issues from the strnstr(1) manpage:

Oh! Then don't bother switching to strnstr. I don't want to break Linux or other ports.

[David Kilzer (:ddkilzer)]

Committed r215521: <http://trac.webkit.org/changeset/215521>

[David Kilzer (:ddkilzer)]

(In reply to Brent Fulgham from comment #5)
> (In reply to David Kilzer (:ddkilzer) from comment #4)
> > There is no security issue with strstr() as long as the string is
> > NULL-terminated, but I can switch to strnstr().  However, note the
> > compatibility issues from the strnstr(1) manpage:
> 
> Oh! Then don't bother switching to strnstr. I don't want to break Linux or
> other ports.

This code was only used on the Mac port, so I switched to strnstr() anyway.

Also, I added RELEASE_ASSERT() instead of ASSERT_WITH_SECURITY_IMPLICATION() since we really do want to crash if the pointer math is incorrect.  (I only added these RELEASE_ASSERT() statements before the actual buffer was modified as well.)