Re: [PATCH] fix failure of tempnam to null-terminate result

Re: [PATCH] fix failure of tempnam to null-terminate result

[Szabolcs Nagy]

* Felix Janda <felix.janda@posteo.de> [2015-08-08 19:25:13 +0200]:
> tempnam uses an uninitialized buffer which is filled using memcpy and
> __randname. It is therefore necessary to explicitly null-terminate it.

ouch

i think this bug is not exploitable

but in the same function there is a possible overflow issue:

	dl = strlen(dir);
	pl = strlen(pfx);
	l = dl + 1 + pl + 1 + 6;

if l overflows here then memcpy can overwrite the stack.

> ---
>  src/stdio/tempnam.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/stdio/tempnam.c b/src/stdio/tempnam.c
> index 45a5f26..b938b31 100644
> --- a/src/stdio/tempnam.c
> +++ b/src/stdio/tempnam.c
> @@ -33,6 +33,7 @@ char *tempnam(const char *dir, const char *pfx)
>  	s[dl] = '/';
>  	memcpy(s+dl+1, pfx, pl);
>  	s[dl+1+pl] = '_';
> +	s[l] = '\0';
>  
>  	for (try=0; try<MAXTRIES; try++) {
>  		__randname(s+l-6);
> -- 
> 2.4.6

Re: [PATCH] fix failure of tempnam to null-terminate result

[Szabolcs Nagy]

* Szabolcs Nagy <nsz@port70.net> [2015-08-08 18:29:19 +0200]:
> 
> but in the same function there is a possible overflow issue:
> 
> 	dl = strlen(dir);
> 	pl = strlen(pfx);
> 	l = dl + 1 + pl + 1 + 6;
> 
> if l overflows here then memcpy can overwrite the stack.
> 

nevermind.. this cant happen

(largest string size possible is SIZE_MAX/2-PAGE_SIZE)

a comment may be useful there though..

Re: [PATCH] fix failure of tempnam to null-terminate result

[Rich Felker]

On Sat, Aug 08, 2015 at 06:38:52PM +0200, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2015-08-08 18:29:19 +0200]:
> > 
> > but in the same function there is a possible overflow issue:
> > 
> > 	dl = strlen(dir);
> > 	pl = strlen(pfx);
> > 	l = dl + 1 + pl + 1 + 6;
> > 
> > if l overflows here then memcpy can overwrite the stack.
> > 
> 
> nevermind.. this cant happen
> 
> (largest string size possible is SIZE_MAX/2-PAGE_SIZE)
> 
> a comment may be useful there though..

Yes, generally we assume actual_size_1 + actual_size_2 + small_const
cannot overflow for exactly this reason.

Rich

[PATCH] fix failure of tempnam to null-terminate result

[Felix Janda]

tempnam uses an uninitialized buffer which is filled using memcpy and
__randname. It is therefore necessary to explicitly null-terminate it.
---
 src/stdio/tempnam.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/stdio/tempnam.c b/src/stdio/tempnam.c
index 45a5f26..b938b31 100644
--- a/src/stdio/tempnam.c
+++ b/src/stdio/tempnam.c
@@ -33,6 +33,7 @@ char *tempnam(const char *dir, const char *pfx)
 	s[dl] = '/';
 	memcpy(s+dl+1, pfx, pl);
 	s[dl+1+pl] = '_';
+	s[l] = '\0';
 
 	for (try=0; try<MAXTRIES; try++) {
 		__randname(s+l-6);
-- 
2.4.6

Re: [PATCH] fix failure of tempnam to null-terminate result

[Rich Felker]

On Sat, Aug 08, 2015 at 07:25:13PM +0200, Felix Janda wrote:
> tempnam uses an uninitialized buffer which is filled using memcpy and
> __randname. It is therefore necessary to explicitly null-terminate it.
> ---
>  src/stdio/tempnam.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/stdio/tempnam.c b/src/stdio/tempnam.c
> index 45a5f26..b938b31 100644
> --- a/src/stdio/tempnam.c
> +++ b/src/stdio/tempnam.c
> @@ -33,6 +33,7 @@ char *tempnam(const char *dir, const char *pfx)
>  	s[dl] = '/';
>  	memcpy(s+dl+1, pfx, pl);
>  	s[dl+1+pl] = '_';
> +	s[l] = '\0';
>  
>  	for (try=0; try<MAXTRIES; try++) {
>  		__randname(s+l-6);

Thanks! Committed with one small change.

Rich