| Message ID | 1529535071-14555-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests/powerpc: Fix strncpy usage | expand |
On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote: > - strncpy(prog, argv[0], strlen(argv[0])); > + strncpy(prog, argv[0], sizeof(prog) - 1); strncpy(prog, argv[0], sizeof prog); if (prog[sizeof prog - 1]) scream_bloody_murder(); Silently using the wrong data is a worse habit than not checking for overflows ;-) Segher
Hi Segher, On 06/21/2018 08:18 PM, Segher Boessenkool wrote: > On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote: >> - strncpy(prog, argv[0], strlen(argv[0])); >> + strncpy(prog, argv[0], sizeof(prog) - 1); > > strncpy(prog, argv[0], sizeof prog); > if (prog[sizeof prog - 1]) > scream_bloody_murder(); > > Silently using the wrong data is a worse habit than not checking for > overflows ;-) Completely agree! Thanks for bringing this up. If you don't mind, I would solve this problem slightly different, as it seems to be more readable. - strncpy(prog, argv[0], strlen(argv[0])); + if (strlen(argv[0]) >= LEN_MAX){ + fprintf(stderr, "Very big executable name: %s\n", argv[0]); + return 1; + } + + strncpy(prog, argv[0], sizeof(prog) - 1); return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");
Le 22/06/2018 à 16:43, Breno Leitao a écrit : > Hi Segher, > > On 06/21/2018 08:18 PM, Segher Boessenkool wrote: >> On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote: >>> - strncpy(prog, argv[0], strlen(argv[0])); >>> + strncpy(prog, argv[0], sizeof(prog) - 1); >> >> strncpy(prog, argv[0], sizeof prog); >> if (prog[sizeof prog - 1]) >> scream_bloody_murder(); >> >> Silently using the wrong data is a worse habit than not checking for >> overflows ;-) > > Completely agree! Thanks for bringing this up. > > If you don't mind, I would solve this problem slightly different, as it seems > to be more readable. > > > - strncpy(prog, argv[0], strlen(argv[0])); > + if (strlen(argv[0]) >= LEN_MAX){ wouldn't it be better to use sizeof(prog) instead of LEN_MAX ? > + fprintf(stderr, "Very big executable name: %s\n", argv[0]); > + return 1; > + } > + > + strncpy(prog, argv[0], sizeof(prog) - 1); You have checked before that argv[0] is not too long, so you should not need to use strncpy(), strcpy() would do it. > return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test"); > Christophe
On 06/22/2018 09:43 AM, Breno Leitao wrote: > If you don't mind, I would solve this problem slightly different, as it seems > to be more readable. > > - strncpy(prog, argv[0], strlen(argv[0])); > + if (strlen(argv[0]) >= LEN_MAX){ > + fprintf(stderr, "Very big executable name: %s\n", argv[0]); "Very big" is an observation. "Too big" indicates a problem better. Or, more explicitly "Executable name is too long". PC
On Friday, June 22, 2018, 11:15:29 AM, Paul Clarke wrote: > On 06/22/2018 09:43 AM, Breno Leitao wrote: >> If you don't mind, I would solve this problem slightly different, as it seems >> to be more readable. >> >> - strncpy(prog, argv[0], strlen(argv[0])); >> + if (strlen(argv[0]) >= LEN_MAX){ >> + fprintf(stderr, "Very big executable name: %s\n", argv[0]); > "Very big" is an observation. "Too big" indicates a problem > better. Or, more explicitly "Executable name is too long". Or even better, display the limit that is being exceeded, in case that value changes over time. Something like. - strncpy(prog, argv[0], strlen(argv[0])); + if (strlen(argv[0]) >= LEN_MAX){ + fprintf(stderr, "Executable name exceeds limit (%d): %s\n", + LEN_MAX, + argv[0]);
On Fri, Jun 22, 2018 at 04:51:21PM +0200, Christophe LEROY wrote: > Le 22/06/2018 à 16:43, Breno Leitao a écrit : > >+ fprintf(stderr, "Very big executable name: %s\n", argv[0]); > >+ return 1; > >+ } > >+ > >+ strncpy(prog, argv[0], sizeof(prog) - 1); > > You have checked before that argv[0] is not too long, so you should not > need to use strncpy(), strcpy() would do it. If you don't care about the bytes of prog after the first zero byte, sure. Segher
Hi! On Fri, Jun 22, 2018 at 11:43:44AM -0300, Breno Leitao wrote: > On 06/21/2018 08:18 PM, Segher Boessenkool wrote: > > On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote: > >> - strncpy(prog, argv[0], strlen(argv[0])); > >> + strncpy(prog, argv[0], sizeof(prog) - 1); > > > > strncpy(prog, argv[0], sizeof prog); > > if (prog[sizeof prog - 1]) > > scream_bloody_murder(); > > > > Silently using the wrong data is a worse habit than not checking for > > overflows ;-) > > Completely agree! Thanks for bringing this up. > > If you don't mind, I would solve this problem slightly different, as it seems > to be more readable. > > - strncpy(prog, argv[0], strlen(argv[0])); > + if (strlen(argv[0]) >= LEN_MAX){ > + fprintf(stderr, "Very big executable name: %s\n", argv[0]); > + return 1; > + } > + > + strncpy(prog, argv[0], sizeof(prog) - 1); The strlen reads all of argv[0], which can be very big in theory. It won't matter in this test file -- program arguments cannot be super long, for one thing -- but it's not a good idea in general (that is one of the problems of strlcpy, btw). Best of course is to avoid string length restrictions completely, if you can. Segher
hi Segher, On 06/22/2018 10:10 PM, Segher Boessenkool wrote: >> - strncpy(prog, argv[0], strlen(argv[0])); >> + if (strlen(argv[0]) >= LEN_MAX){ >> + fprintf(stderr, "Very big executable name: %s\n", argv[0]); >> + return 1; >> + } >> + >> + strncpy(prog, argv[0], sizeof(prog) - 1); > > The strlen reads all of argv[0], which can be very big in theory. It won't > matter in this test file -- program arguments cannot be super long, for one > thing -- but it's not a good idea in general (that is one of the problems > of strlcpy, btw). > > Best of course is to avoid string length restrictions completely, if you can. Right, I was thinking about this problem and there is no motivation to have a statically allocated and limited region. I will send a v2 where 'prog' and avoid this restriction completely. Thanks
diff --git a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c index 08a8b95e3bc1..638e0dc717d5 100644 --- a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c +++ b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c @@ -104,6 +104,6 @@ int main(int argc, char *argv[]) exit(1); } - strncpy(prog, argv[0], strlen(argv[0])); + strncpy(prog, argv[0], sizeof(prog) - 1); return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test"); }
There is a buffer overflow in dscr_inherit_test.c test. In main(), strncpy()'s third argument is the lengh of the source, not the size of the destination buffer, which makes strncpy() behaves like strcpy(), causing a buffer overflow if argv[0] is bigger than LEN_MAX (100). This patch simply limit the string copy to sizeof(prog) less 1 (space for \0). CC: Anshuman Khandual <khandual@linux.vnet.ibm.com> Signed-off-by: Breno Leitao <leitao@debian.org> --- tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)