Bug 170994

Summary: Stop using strcpy() in WebKit::EnvironmentUtilities::stripValuesEndingWithString()
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: 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 Flags
Patch bfulgham: review+, bfulgham: commit-queue-

Description David Kilzer (:ddkilzer) 2017-04-19 09:08:56 PDT
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).
Comment 1 David Kilzer (:ddkilzer) 2017-04-19 09:09:13 PDT
<rdar://problem/29889932>
Comment 2 David Kilzer (:ddkilzer) 2017-04-19 09:24:55 PDT
Created attachment 307482 [details]
Patch
Comment 3 Brent Fulgham 2017-04-19 09:38:01 PDT
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"
Comment 4 David Kilzer (:ddkilzer) 2017-04-19 09:55:09 PDT
(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.
Comment 5 Brent Fulgham 2017-04-19 10:09:10 PDT
(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.
Comment 6 David Kilzer (:ddkilzer) 2017-04-19 11:17:21 PDT
Committed r215521: <http://trac.webkit.org/changeset/215521>
Comment 7 David Kilzer (:ddkilzer) 2017-04-19 11:21:41 PDT
(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.)