Summary: | Stop using strcpy() in WebKit::EnvironmentUtilities::stripValuesEndingWithString() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | WebKit2 | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bfulgham, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=171494 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 171010 | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2017-04-19 09:08:56 PDT
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.) |