Yeah actually here security point is meaningless buf *IF* a attacker 
intentionally breaks the string buffer in edje_cc.
strcpy() is the one of the easy points to hack the program by attackers.

Regardless of it, still, I hate these repeated reports from static code 
analyziers such as coverity.

So changed.

-----Original Message-----
From: "Carsten Haitzler"<ras...@rasterman.com> 
To: "Enlightenment developer list"<enlightenment-devel@lists.sourceforge.net>; 
Cc: <g...@lists.enlightenment.org>; 
Sent: 2016-09-22 (목) 08:24:04
Subject: Re: [E-devel] [EGIT] [core/efl] master 01/01: edje/edje_cc: use 
strncpy() instead of strcpy().
 
On Wed, 21 Sep 2016 09:19:57 -0300 Gustavo Sverzut Barbieri
<barbi...@gmail.com> said:

> On Wed, Sep 21, 2016 at 1:32 AM, ChunEon Park <her...@hermet.pe.kr> wrote:
> > hermet pushed a commit to branch master.
> >
> > http://git.enlightenment.org/core/efl.git/commit/?id=ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
> >
> > commit ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
> > Author: Hermet Park <her...@hermet.pe.kr>
> > Date:   Wed Sep 21 13:30:44 2016 +0900
> >
> >     edje/edje_cc: use strncpy() instead of strcpy().
> >
> >     strncpy() is better for security.
> >     Also, this change avoids annoying coverity detection.
> > ---
> >  src/bin/edje/edje_cc_parse.c  2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/bin/edje/edje_cc_parse.c b/src/bin/edje/edje_cc_parse.c
> > index 525c71d..efabe22 100644
> > --- a/src/bin/edje/edje_cc_parse.c
> > +++ b/src/bin/edje/edje_cc_parse.c
> > @@ -391,7 +391,7 @@ next_token(char *p, char *end, char **new_p, int *delim)
> >               l = sscanf(tmpstr, "%*s %i \"%[^\"]\"", &nm, fl);
> >               if (l == 2)
> >                 {
> > -                  strcpy(file_buf, fl);
> > +                  strncpy(file_buf, fl, sizeof(file_buf));
> >                    line = nm;
> >                    file_in = file_buf;
> >                 }
> 
> 
> the proper function to call is eina_strlcpy(), it will use strlcpy()
> if available, otherwise will ensure the nul byte.

actually the proper thing to do is... not change the code at all. read it. it's
safe. file_buf is 4096. fl is 4096. it CANNOT overflow by definition as the
string being copied into the target cannot be bigger.

people need to be careful of this kind of "well someone told me to use strncpy
because its safe". especially if it becomes a blindly applied "rule". read the
code. there is a chance you actually may break it by doing such changes (this
change doesn't).

in fact if there is an issue it's in the sscanf into the fl buffer which places
no limits on len (and fl is limited). of course ... the file path would have to
be insanely huge for this to happen and it'd be an invalid path in most
environments so things would fail one way or another... what edc files used to
exploit edje_cc? at this point we're compiling stuff so... all bets are off
anyway. so security isn't the issue imho.

anyway. point being - this code fixes NO BUG. it doesnt make code any safer.
what it does do is totally miss the actual potential bug in the sscanf :)

-- 
------------- Codito, ergo sum - "I code, therefore I am" --------------
The Rasterman (Carsten Haitzler)    ras...@rasterman.com


------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Reply via email to