Re: [Security] Possible buffer overrun in a debug function
Date: Sat, 25 Jul 2020 20:50:36 +0200 (CEST)
On Fri, 24 Jul 2020, Nicolas Went via curl-users wrote:
Thanks for your mail!
> /* use the value as file name */
> char fname[CURL_MT_LOGFNAME_BUFSIZE];
> if(strlen(env) >= CURL_MT_LOGFNAME_BUFSIZE)
> env[CURL_MT_LOGFNAME_BUFSIZE-1] = '\0';
> strcpy(fname, env);
> As you can see there is the function strcpy used lign 12 there and lign 113
> in the real file.
> This function is unsafe to use since it can lead to buffer overruns if the
> destination size is smaller than the source size. Which can be the case there
> since we copy a string of size depending on the maccro
> CURL_MT_LOGFNAME_BUFSIZE, which definition can depend (we found one equal to
> 512) and the string env is at destination, which size is not defined is the
> manual of curlx_getenv.
> And only the case when the size of env is higher than the size of fname is
> tested but not the opposit.
It doesn't check the size of 'env' it checks the length of the string in env,
and if it is too long then it truncates it.
If CURL_MEMDEBUG returns a string that is longer than or equal than
CURL_MT_LOGFNAME_BUFSIZE, then it is truncated at that length minus one.
I don't see how that makes strcpy() overflow the target. Do you have a proof
of concept that shows the overflow happen?
> We suggest to use strncpy instead of strcpy to ensure safe use.
I'm sorry, but strncpy() is generally a bad replacement for strcpy() since it
doesn't guarantee a zero terminator. It is better to make sure the strcpy()
can be done without overflowing.
> Since this problem is only located in a debug function, we don't think that
> it is legitimate to post it inside of their bounty bug hunter.
Agreed. It's not code that is shipped in production, only used for testing and
-- / daniel.haxx.se | Commercial curl support up to 24x7 is available! | Private help, bug fixes, support, ports, new features | https://www.wolfssl.com/contact/