2012-08-17 19:38:29

by Jesper Juhl

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

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 <[email protected]>
---
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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.


2012-08-17 23:43:25

by Tony Finch

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

Jesper Juhl <[email protected]> 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 <[email protected]> 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.

2012-08-20 08:50:59

by Bastien Roucariès

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

On Sat, Aug 18, 2012 at 1:43 AM, Tony Finch <[email protected]> wrote:
> Jesper Juhl <[email protected]> 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 <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2012-08-20 09:35:56

by Tony Finch

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

Bastien ROUCARIES <[email protected]> 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 <[email protected]> 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.

2012-08-19 21:43:03

by Jesper Juhl

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

On Sat, 18 Aug 2012, Tony Finch wrote:

> Jesper Juhl <[email protected]> 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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.