[E-devel] FW: [EGIT] [core/efl] master 01/01: edje/edje_cc: use strncpy() instead of strcpy().

[Hermet Park]

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 \"%[^\"]\"", , 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

Re: [E-devel] [EGIT] [core/efl] master 01/01: edje/edje_cc: use strncpy() instead of strcpy().

[The Rasterman]

On Wed, 21 Sep 2016 09:19:57 -0300 Gustavo Sverzut Barbieri
 said:

> On Wed, Sep 21, 2016 at 1:32 AM, ChunEon Park  wrote:
> > hermet pushed a commit to branch master.
> >
> > http://git.enlightenment.org/core/efl.git/commit/?id=ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
> >
> > commit ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
> > Author: Hermet Park 
> > 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 \"%[^\"]\"", , 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

Re: [E-devel] [EGIT] [core/efl] master 01/01: edje/edje_cc: use strncpy() instead of strcpy().

[Gustavo Sverzut Barbieri]

On Wed, Sep 21, 2016 at 1:32 AM, ChunEon Park  wrote:
> hermet pushed a commit to branch master.
>
> http://git.enlightenment.org/core/efl.git/commit/?id=ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
>
> commit ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
> Author: Hermet Park 
> 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 \"%[^\"]\"", , 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.



-- 
Gustavo Sverzut Barbieri
--
Mobile: +55 (16) 99354-9890

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

[EGIT] [core/efl] master 01/01: edje/edje_cc: use strncpy() instead of strcpy().

[ChunEon Park]

hermet pushed a commit to branch master.

http://git.enlightenment.org/core/efl.git/commit/?id=ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6

commit ab1a72f5e7df6fe0adef54bdcddd9867a2ebe3a6
Author: Hermet Park 
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 \"%[^\"]\"", , fl);
  if (l == 2)
{
-  strcpy(file_buf, fl);
+  strncpy(file_buf, fl, sizeof(file_buf));
   line = nm;
   file_in = file_buf;
}

--