[Openvpn-devel,4/4,v3] create_temp_file/gen_path: prevent memory leak if gc == NULL

If gc == NULL, the data allocated in the alloc_gc_buf() call in
create_temp_file or the string_mod_const call in gen_path would never
be free'd.

These functions are currently never called that way, but let's prevent
future problems.

While touching create_temp_file, also remove the counter variable, which is
never read, simplify the do-while to a while loop, and truncate the prefix
(if needed) to preserve the random and extension of the created filename.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
v2:
 - change create_temp_file to avoid using a struct buffer (simpler)
 - add gc != NULL check for gen_path (avoid similar memleak pitfall)
v3:
 - Check the return value of openvpn_snprintf()

 src/openvpn/misc.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Hi Steffan,

patch looks good, but I have just one comment below

On 02/11/17 06:03, Steffan Karger wrote:
> If gc == NULL, the data allocated in the alloc_gc_buf() call in
> create_temp_file or the string_mod_const call in gen_path would never
> be free'd.
> 
> These functions are currently never called that way, but let's prevent
> future problems.
> 
> While touching create_temp_file, also remove the counter variable, which is
> never read, simplify the do-while to a while loop, and truncate the prefix
> (if needed) to preserve the random and extension of the created filename.
> 
> Signed-off-by: Steffan Karger <steffan@karger.me>
> ---
> v2:
>  - change create_temp_file to avoid using a struct buffer (simpler)
>  - add gc != NULL check for gen_path (avoid similar memleak pitfall)
> v3:
>  - Check the return value of openvpn_snprintf()
> 
>  src/openvpn/misc.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 25f38003..67011169 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -723,21 +723,26 @@ test_file(const char *filename)
>  const char *
>  create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
>  {
> -    static unsigned int counter;
> -    struct buffer fname = alloc_buf_gc(256, gc);
>      int fd;
>      const char *retfname = NULL;
>      unsigned int attempts = 0;
> +    char fname[256] = { 0 };
> +    const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
> +    const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 * 8));
>  
> -    do
> +    while (attempts < 6)
>      {
>          ++attempts;
> -        ++counter;
>  
> -        buf_printf(&fname, PACKAGE "_%s_%08lx%08lx.tmp", prefix,
> -                   (unsigned long) get_random(), (unsigned long) get_random());
> +        if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len,
> +                              prefix, (unsigned long) get_random(),
> +                              (unsigned long) get_random()))
> +        {
> +            msg(M_WARN, "ERROR: temporary filename too long");
> +            return NULL;
> +        }
>  
> -        retfname = gen_path(directory, BSTR(&fname), gc);
> +        retfname = gen_path(directory, fname, gc);
>          if (!retfname)
>          {
>              msg(M_WARN, "Failed to create temporary filename and path");
> @@ -760,7 +765,6 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
>              return NULL;
>          }
>      }
> -    while (attempts < 6);
>  
>      msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
>      return NULL;
> @@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
>  #else
>      const int CC_PATH_RESERVED = CC_SLASH;
>  #endif
> +
> +    if (!gc)
> +    {
> +        return NULL; /* Would leak memory otherwise */

As far as I understood the ratio behind this change, we are basically
saying that passing gc == NULL is a real programming error and not a
runtime problem. If so, wouldn't it be better to assert(gc) directly?
I am saying so because we should make this failure louder, so that it
can be recognized in an early test.

Or can we have cases when this function is called with NULL because of a
temporary failure (my understanding is that this should not be possible)?


Cheers,

> +    }
> +
>      const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
>  
>      if (safe_filename
>

Hi,

On 09-11-17 04:25, Antonio Quartulli wrote:
>> @@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
>>  #else
>>      const int CC_PATH_RESERVED = CC_SLASH;
>>  #endif
>> +
>> +    if (!gc)
>> +    {
>> +        return NULL; /* Would leak memory otherwise */
> 
> As far as I understood the ratio behind this change, we are basically
> saying that passing gc == NULL is a real programming error and not a
> runtime problem. If so, wouldn't it be better to assert(gc) directly?
> I am saying so because we should make this failure louder, so that it
> can be recognized in an early test.
> 
> Or can we have cases when this function is called with NULL because of a
> temporary failure (my understanding is that this should not be possible)?

I'm a bit in doubt here - if this parameter is NULL, that's a
programming error.  But we've seen before that programming errors
sometimes can be triggered by external input.  In this case, this
function can be called when a client connects.  It *should* never hit
this case, but if it somehow does, that becomes a DoS vuln.  So I think
I prefer 'graceful error handling' in paths that are exposed like this,
unless the error indicates a system integrity error such as an
out-of-bounds read or write.

-Steffan
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

On 09/11/17 21:46, Steffan Karger wrote:
> Hi,
> 
> On 09-11-17 04:25, Antonio Quartulli wrote:
>>> @@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
>>>  #else
>>>      const int CC_PATH_RESERVED = CC_SLASH;
>>>  #endif
>>> +
>>> +    if (!gc)
>>> +    {
>>> +        return NULL; /* Would leak memory otherwise */
>>
>> As far as I understood the ratio behind this change, we are basically
>> saying that passing gc == NULL is a real programming error and not a
>> runtime problem. If so, wouldn't it be better to assert(gc) directly?
>> I am saying so because we should make this failure louder, so that it
>> can be recognized in an early test.
>>
>> Or can we have cases when this function is called with NULL because of a
>> temporary failure (my understanding is that this should not be possible)?
> 
> I'm a bit in doubt here - if this parameter is NULL, that's a
> programming error.  But we've seen before that programming errors
> sometimes can be triggered by external input.  In this case, this
> function can be called when a client connects.  It *should* never hit
> this case, but if it somehow does, that becomes a DoS vuln.  So I think
> I prefer 'graceful error handling' in paths that are exposed like this,
> unless the error indicates a system integrity error such as an
> out-of-bounds read or write.

Yeah, makes sense. I was also worried about the DoS, that's why I was
trying to convince myself that passing NULL was not possible at all. But
I like your defensive approach more.

Acked-by: Antonio Quartulli <a@unstable.cc>

Your patch has been applied to the master branch.

commit bd89ebd6a82856c7939b4ade35d14d0178a96986
Author: Steffan Karger
Date:   Wed Nov 1 23:03:42 2017 +0100

     create_temp_file/gen_path: prevent memory leak if gc == NULL

     Signed-off-by: Steffan Karger <steffan@karger.me>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20171101220342.14648-5-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15703.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot