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).
<rdar://problem/29889932>
Created attachment 307482 [details] Patch
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"
(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.
(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.
Committed r215521: <http://trac.webkit.org/changeset/215521>
(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.)