[RFC] Warning for potentially unbound writes to function parameters

[RFC] Warning for potentially unbound writes to function parameters

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

In some real-world code, I noticed a curious pattern: using the unsafe 
string functions on function parameter arguments.  This leads to 
gets()-style unsafe APIs.

I've looked at how to implement a warning for this, and came up with the 
attached patch.  Do you think this makes sense?

      1	#include <string.h>
      2	
      3	const char *data (void);
      4	
      5	void test (char *target)
      6	{
      7	  strcpy(target, data ());
      8	}
      9	
     10	
     11	void test_2 (char *target)
     12	{
     13	  char *p = target;
     14	  strcpy(p, data ());
     15	}
     16	

/tmp/t.c: In function ‘test’:
/tmp/t.c:7:9: warning: potentially unbound write to function parameter 
‘target’ [-Wunbound-parameter-write]
    strcpy(target, data ());
          ^
/tmp/t.c: In function ‘test_2’:
/tmp/t.c:14:9: warning: potentially unbound write to function parameter 
‘target’ [-Wunbound-parameter-write]
    strcpy(p, data ());
          ^

Obviously, the warning and its name need adjusting, and more functions 
need to be covered.  But I want to check first if you think the warning 
makes sense at all, and if I've found the right place to implement it 
(this approach seems to require optimization, alas).

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: unbound_parameter_write.patch --]
[-- Type: text/x-patch, Size: 1443 bytes --]

commit 324c7189c9cf871584da988f12d1a686df0d6e0c
Author: Florian Weimer <fweimer@redhat.com>
Date:   Fri Aug 17 18:19:13 2012 +0200

    Implement -Wunbound-parameter-write (proof of concept)

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 4b177c4..dc90484 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3274,6 +3274,14 @@ expand_builtin_strcpy (tree exp, rtx target)
    {
      tree dest = CALL_EXPR_ARG (exp, 0);
      tree src = CALL_EXPR_ARG (exp, 1);
+     if (TREE_CODE (dest) == SSA_NAME)
+       {
+	 tree dest_var = SSA_NAME_VAR (dest);
+	 if (TREE_CODE (dest_var) == PARM_DECL)
+	   warning_at (EXPR_LOCATION (exp), OPT_Wunbound_parameter_write,
+		       "potentially unbound write to function parameter %qD",
+		       dest_var);
+       }
      return expand_builtin_strcpy_args (dest, src, target);
    }
    return NULL_RTX;
diff --git a/gcc/common.opt b/gcc/common.opt
index deb89e3..fe892b7 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -562,6 +562,10 @@ Wlarger-than=
 Common RejectNegative Joined UInteger Warning
 -Wlarger-than=<number>	Warn if an object is larger than <number> bytes
 
+Wunbound-parameter-write
+Common Var(warn_unbound_parameter_write) Warning
+Warn if a function without array bounds checking writes to a pointer passed as an parameter
+
 Wunsafe-loop-optimizations
 Common Var(warn_unsafe_loop_optimizations) Warning
 Warn if the loop cannot be optimized due to nontrivial assumptions.

Re: [RFC] Warning for potentially unbound writes to function parameters

[Gabriel Dos Reis]

On Fri, Aug 17, 2012 at 11:22 AM, Florian Weimer <fweimer@redhat.com> wrote:
> In some real-world code, I noticed a curious pattern: using the unsafe
> string functions on function parameter arguments.  This leads to
> gets()-style unsafe APIs.
>
> I've looked at how to implement a warning for this, and came up with the
> attached patch.  Do you think this makes sense?
>
>      1  #include <string.h>
>      2
>      3  const char *data (void);
>      4
>      5  void test (char *target)
>      6  {
>      7    strcpy(target, data ());
>      8  }
>      9
>     10
>     11  void test_2 (char *target)
>     12  {
>     13    char *p = target;
>     14    strcpy(p, data ());
>     15  }
>     16
>
> /tmp/t.c: In function ‘test’:
> /tmp/t.c:7:9: warning: potentially unbound write to function parameter
> ‘target’ [-Wunbound-parameter-write]
>    strcpy(target, data ());
>          ^
> /tmp/t.c: In function ‘test_2’:
> /tmp/t.c:14:9: warning: potentially unbound write to function parameter
> ‘target’ [-Wunbound-parameter-write]
>    strcpy(p, data ());
>          ^
>
> Obviously, the warning and its name need adjusting, and more functions need
> to be covered.  But I want to check first if you think the warning makes
> sense at all, and if I've found the right place to implement it (this
> approach seems to require optimization, alas).
>
> --
> Florian Weimer / Red Hat Product Security Team

Hmm, I think it help a little bit if you could expand on where exactly
the danger the patch is trying to prevent is, and where what
does "unbound parameter" refer to or mean?  (I don't know what
an unbound parameter is)

-- Gaby

Re: [RFC] Warning for potentially unbound writes to function parameters

[Florian Weimer]

On 08/17/2012 09:15 PM, Gabriel Dos Reis wrote:

> Hmm, I think it help a little bit if you could expand on where exactly
> the danger the patch is trying to prevent is, and where what
> does "unbound parameter" refer to or mean?  (I don't know what
> an unbound parameter is)

Sorry for being unclear.  I haven't found good terminology yet.

I have seen some interfaces which behave very similar to gets(char *). 
You pass in a buffer, and the called function writes data to it, and you 
don't know how much will be written.  A common pattern among those 
functions was that they wrote to the argument buffer using strcpy or 
sprintf.  It seems to me that this always warrants a warning because any 
false positive in the strcpy case could easily be optimized: if you 
check the size of the target buffer before writing to it, you know the 
length of the string you're going to write, so you can use memcpy 
instead of strcpy.  For sprintf, you'd just use snprintf directly, 
removing the separate length check.

Function pointers come into play because these are supplied by the 
caller.  Ideally, we would flag any write to a pointer which does not 
escape to the caller (because it is a pre-existing buffer), but this is 
much more difficult to implement.  (In C++ mode, the warning would be 
disabled for non-constant reference-to-pointer-to-char parameters.)

Historically, there have been some APIs which had implied upper bounds 
on buffer arguments (mainly PATH_MAX), but these have not fared well can 
turn out very difficult to use correctly over time (readdir_r being an 
example).

-- 
Florian Weimer / Red Hat Product Security Team