Re: [PATCH] unifdef: set a secure umask before calling mkstemp()

Re: [PATCH] unifdef: set a secure umask before calling mkstemp()

[Jesper Juhl]

On Sat, 18 Aug 2012, Tony Finch wrote:

> Jesper Juhl <jj@chaosbits.net> wrote:
> 
> > In newer glibc's (versions > 2.06) reasonably secure permissions of
> > 0600 are used when creating a temporary file with mkstemp(). But for
> > older glibc's (versions <= 2.06) 0666 is used which is not secure.
> 
> Thanks for your suggestion! I'm afraid I prefer not to make the change.
> 
> Unifdef is only using mkstemp as a convenient way to open a file with a
> non-clashing name. It isn't trying to be secure, so it's OK just to rely
> on the user's umask. And I find it hard to care about a bug that was fixed
> 15 years ago.
> 
> I'm also trying to reduce the unixisms in the program for portability
> reasons and this is the most awkward part :-/
> 
Fair enough. :-)

Just ignore the patch.

Have a nice day.

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

[PATCH] unifdef: set a secure umask before calling mkstemp()

[Jesper Juhl]

In newer glibc's (versions > 2.06) reasonably secure permissions of
0600 are used when creating a temporary file with mkstemp(). But for
older glibc's (versions <= 2.06) 0666 is used which is not secure.

To ensure that the temporary files created always have reasonably
secure permissions, add a call to umask() that ensures we always set
at most 0600 on the temporary file (and then restore the umask again
so we don't interfere with anything that hapens further on).

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 scripts/unifdef.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/unifdef.c b/scripts/unifdef.c
index 7493c0e..f9188fb 100644
--- a/scripts/unifdef.c
+++ b/scripts/unifdef.c
@@ -342,9 +342,10 @@ main(int argc, char *argv[])
 				    "%.*s/" TEMPLATE,
 				    (int)(dirsep - ofilename), ofilename);
 			else
-				snprintf(tempname, sizeof(tempname),
-				    TEMPLATE);
+				snprintf(tempname, sizeof(tempname), TEMPLATE);
+			mode_t mask = umask(S_IXUSR | S_IRWXG | S_IRWXO);
 			ofd = mkstemp(tempname);
+			umask(mask);
 			if (ofd != -1)
 				output = fdopen(ofd, "wb+");
 			if (output == NULL)
-- 
1.7.11.4


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] unifdef: set a secure umask before calling mkstemp()

[Tony Finch]

Jesper Juhl <jj@chaosbits.net> wrote:

> In newer glibc's (versions > 2.06) reasonably secure permissions of
> 0600 are used when creating a temporary file with mkstemp(). But for
> older glibc's (versions <= 2.06) 0666 is used which is not secure.

Thanks for your suggestion! I'm afraid I prefer not to make the change.

Unifdef is only using mkstemp as a convenient way to open a file with a
non-clashing name. It isn't trying to be secure, so it's OK just to rely
on the user's umask. And I find it hard to care about a bug that was fixed
15 years ago.

I'm also trying to reduce the unixisms in the program for portability
reasons and this is the most awkward part :-/

Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
Forties, Cromarty: East, veering southeast, 4 or 5, occasionally 6 at first.
Rough, becoming slight or moderate. Showers, rain at first. Moderate or good,
occasionally poor at first.

Re: [PATCH] unifdef: set a secure umask before calling mkstemp()

[Bastien ROUCARIES]

On Sat, Aug 18, 2012 at 1:43 AM, Tony Finch <dot@dotat.at> wrote:
> Jesper Juhl <jj@chaosbits.net> wrote:
>
>> In newer glibc's (versions > 2.06) reasonably secure permissions of
>> 0600 are used when creating a temporary file with mkstemp(). But for
>> older glibc's (versions <= 2.06) 0666 is used which is not secure.
>
> Thanks for your suggestion! I'm afraid I prefer not to make the change.
>
> Unifdef is only using mkstemp as a convenient way to open a file with a
> non-clashing name. It isn't trying to be secure, so it's OK just to rely
> on the user's umask. And I find it hard to care about a bug that was fixed
> 15 years ago.
>
> I'm also trying to reduce the unixisms in the program for portability
> reasons and this is the most awkward part :-/

have you tried gnulib for improving portability ?

Bastien
> Tony.
> --
> f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
> Forties, Cromarty: East, veering southeast, 4 or 5, occasionally 6 at first.
> Rough, becoming slight or moderate. Showers, rain at first. Moderate or good,
> occasionally poor at first.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] unifdef: set a secure umask before calling mkstemp()

[Tony Finch]

Bastien ROUCARIES <roucaries.bastien@gmail.com> wrote:
>
> have you tried gnulib for improving portability ?

My strategy is to try to avoid using anything outside the standard C89 library.

Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
Forties, Cromarty: East, veering southeast, 4 or 5, occasionally 6 at first.
Rough, becoming slight or moderate. Showers, rain at first. Moderate or good,
occasionally poor at first.