warnings on compile
Klein, Christopher
freebsd 4.1 I am getting the following warning repeated many times on the
initial make (I did run the configure step first, and I consulted the
appropriate readme documents)
Warning: mktemp() possibly used unsafely; consider using mkstemp
As the security on microsoft products are suspect at best, I do not
want to add to the insecurity of the network by adding insecure binaries on
the unix platform if it can be avoided. Is there some kind of switch or
other modification that will allow the compile to use mkstemp() and not
generate this message. any other suggestions
Thanks
Christopher Klein
Social and Scientific Systems
ckl...@s-3.com
Jeremy Allison
>
> Hi there, I am trying to install samba on an ftp server running
> freebsd 4.1 I am getting the following warning repeated many times on the
> initial make (I did run the configure step first, and I consulted the
> appropriate readme documents)
> Warning: mktemp() possibly used unsafely; consider using mkstemp
> As the security on microsoft products are suspect at best, I do not
> want to add to the insecurity of the network by adding insecure binaries on
> the unix platform if it can be avoided. Is there some kind of switch or
> other modification that will allow the compile to use mkstemp() and not
> generate this message. any other suggestions
The warning is wrong. mktemp is being used securely in Samba.
Every use of the generated filename uses the O_EXCL flag,
which prevents /tmp races.
mkstemp doesn't do what we need here, as it returns a file
descriptor which is not what we want - we want a filename
that is *potentially* unique. We take care of the security
issue ourselves.
Regards,
Jeremy Allison,
Samba Team.
--
--------------------------------------------------------
Buying an operating system without source is like buying
a self-assembly Space Shuttle with no instructions.
--------------------------------------------------------
Tim Potter
> > Hi there, I am trying to install samba on an ftp server running
> > freebsd 4.1 I am getting the following warning repeated many times on the
> > initial make (I did run the configure step first, and I consulted the
> > appropriate readme documents)
> > Warning: mktemp() possibly used unsafely; consider using mkstemp
> > As the security on microsoft products are suspect at best, I do not
> > want to add to the insecurity of the network by adding insecure binaries on
> > the unix platform if it can be avoided. Is there some kind of switch or
> > other modification that will allow the compile to use mkstemp() and not
> > generate this message. any other suggestions
>
> The warning is wrong. mktemp is being used securely in Samba.
Well the warning is technically right - mktmp() may *possibly* be
used unsafely but it isn't in this case. (-:
It's a pretty annoying error though. I wonder if it's possible
to patch gcc to determine whether the O_EXCL flag is not being
used and then print out the warning rather than always doing it.
Tim.
Kenichi Okuyama
TP> Well the warning is technically right - mktmp() may *possibly* be
TP> used unsafely but it isn't in this case. (-:
TP> It's a pretty annoying error though. I wonder if it's possible
TP> to patch gcc to determine whether the O_EXCL flag is not being
TP> used and then print out the warning rather than always doing it.
How can you possibly do such thing when you don't even know whether
the passed string WILL EVEN BE USED AS FILENAME?
I think we should rather create mktemp() and mkstemp() of our own,
without using mktemp() of library. As far as I know, what we really
need is mkstemp() with filename not having ':', and because we can
see the glibc codes, I think we can do this.
I don't think we really need mktemp() at all.
# Do we really/truely? Where??
----
Kenichi Okuyama@Tokyo Research Lab, IBM-Japan. Co.
Anders C. Thorsen
Why bother. Do we really/truely need printf(). We could always create
our own samba edt. of libc, right?
I always get a bit upset when people suggests to make their own implementations
of something which make perfect sense to use. A mktemp() for samba
would do the same thing, and would have the same (potential) problems.
Oh well.. As long as mktemp() is used safely, why not continue to
do so.
If we want people to realize that it is safe, then add
something like:
#ifdef ANNOYING_GCC_VER
#warning "the gcc compiler will complain about mktemp() beeing broken/unsecure"
#warning "samba make use of this function in a safe manner, please ignore the warning"
#warning "you might want to have a look at the samba FAQ www.samba.org"
#endif
--
--Anders
Anders C. Thorsen
PGP Key: http://www.aae.wisc.edu/~anders/anders-pgp.asc
----------------------------------------
Only two things are infinite.
The universe and human stupidity.
Although, I am unsure of the former.
Albert Einstein
Kenichi Okuyama
>> I don't think we really need mktemp() at all.
>> # Do we really/truely? Where??
ACT> do so.
Because of three reason.
1) The fact that we are using safely, this doesn't mean we are
proven to keep using them safely even from now on.
2) We are really using mktemp() inside smbd_mktemp() like this:
i ) get filename using mktemp().
ii) look at the filename, to see if there's no ':' characters.
iii) If we have them, try changing the character to different
character and see if we have such filename under use or not,
until we find pattern not being used.
iv) return that filename.
As you can see, this function named "smbd_mktemp()" is really
mktemp() of our own. WE ALREADY HAVE MADE MKTEMP OF OUR OWN.
Returned value of mktemp() being UNIQUE, is not being kept
because we change it inside smbd_mktemp().
What I'm saying is that it's nonsense to keep using mktemp()
inside this function, especially when many implementation
claiming that "mktemp() is unsafe."
Why should we bother about this warning/errors when
using mktemp() is not a good idea from beginning.
3) Most of the place where smbd_mktemp() is being called,
can be replaced with something we may call "smbd_mkstemp()".
We can keep our "safe-guard" inside smbd_mkstemp(), to make
our implementation more secure. So, we don't need mktemp()
anyway.
I know that re-inventing a "Wheel" is nonsense. I agree with you.
But "Steeling Wheel" is not "Wheel". So, we should invent "Steeling
Wheel". We should not create "Steeling Wheel" from "Wheel".
----
Kenichi Okuyama@Tokyo Research Lab, IBM-Japan, Co.
Jeremy Allison
>
> 1) The fact that we are using safely, this doesn't mean we are
> proven to keep using them safely even from now on.
Yes it does, 'cos we're not going to ship a production
version unless all open calls have been audited for just
this problem. We did this back when the problem was first
brought to our attention, and we've done it ever since.
Eternal vigillance is the price of security. I have no
problem with delaying a release until Andrew or I have
examined the code for obvious problems like this.
Jeremy.
Kenichi Okuyama
>> 1) The fact that we are using safely, this doesn't mean we are
>> proven to keep using them safely even from now on.
JA> version unless all open calls have been audited for just
JA> this problem. We did this back when the problem was first
JA> brought to our attention, and we've done it ever since.
Jeremy, are you saying that you and Andrew are GOD or something? Or
are you simply talking about "doing your best".
If you are GOD, go ahead. Even as a GOD, I don't recommend the way
we do now, but word 'GOD' stands for "infinite resource", so I wont'
stop you. But I can give you an oracle that you will fail.
# At least, I am as GOD as you are, for this point ;)
# GOD enough to give you an oracle.
If you are only talking about "best efforts" ( well, I wish you
are), then story differs. The very fact is, that you only do your
best, and Andrew do his best. There's chance that you two both make
mistake, no matter how small it may be.
SO THERE'S NO PROVE.
We have to face this very fact.
If there's no prove, then I do recommend throwing the problem point
into single function smbd_mkstemp(), where every problem exists. And
not to use any other function for temporary file. This way, we can
have less possibility of making mistakes, for it's bit more visible.
I don't recommend dividing problem point into two, smbd_mktemp() and
open().
And still, even with smbd_mkstemp(), we loose no performance.
And that's what Warning/Error message about mktemp() is talking
about. I do think we should obay the suggestion, for this case.
----
Kenichi Okuyama@Tokyo Research Lab, IBM-Japan, Co.
[Binbou-Gami]: In Japan, there's said to be 8 Million Gods.
In those god, there's god name 'Binbou-Gami', he's god of poverty.
What's so great about him, is that he can eat up entire resource of
any kind, no matter how you may earn it. Even if other GOD tries to
generate infinit resource, Binbou-Gami can waste all the resources
instantly. He's one of the strongest God of all 8Millions.
So, if you believe in 'Binbou-Gami's existance, you should not think
about infinite resource, never. If you think there's no such a god,
.... well .... look at your wallet (^^;)
Steve Langasek
> Jeremy, are you saying that you and Andrew are GOD or something? Or
> are you simply talking about "doing your best".
Are computers God? We expect them to flawlessly execute the Samba code
every time without opening up any root holes. I think the task Jeremy and
Andrew have to carry out, to make sure there aren't any identifiable security
holes in the Samba code itself, is much less demanding. :)
> If you are only talking about "best efforts" ( well, I wish you
> are), then story differs. The very fact is, that you only do your
> best, and Andrew do his best. There's chance that you two both make
> mistake, no matter how small it may be.
> SO THERE'S NO PROVE.
Of course there is. Programming is completely deterministic in nature. If we
have complete information about the behavior of a function, it's possible to
prove whether or not the function is used safely. If we don't have complete
information, then it doesn't matter if the function is used once or 20
times. I think this is a point that too many people (programmers and laymen)
miss, because as a society we've come to accept security holes as inevitable.
But there's no excuse for security holes based on programming errors that have
been widely recognized for more than a decade.
If Andrew and Jeremy perform this audit with every release, that's likely to
/decrease/ the chances of them making a mistake, not increasing it, because
they have more experience and can spot problems more quickly and more
accurately.
Jeremy also said they audit *every* use of the open() function. Changing
smbd_mkstemp() into a full-fledged mkstemp() function won't change all the
other open() calls that would still need auditing.
Despite disagreeing with you about the divine nature of the Samba project
leaders :), it seems to me that pulling the mktemp code entirely into Samba
would be a good idea. There's enough variation in the mktemp(), mkstemp(),
tmpnam(), etc. functions available on different Unices, and it's simple enough
to reimplement correctly, that it might just reduce the Samba code size to do
it all internally.
Steve Langasek
postmodern programmer
Jeremy Allison
> If you are GOD, go ahead. Even as a GOD, I don't recommend the way
> we do now, but word 'GOD' stands for "infinite resource", so I wont'
> stop you. But I can give you an oracle that you will fail.
No, that's not what I meant as I'm sure you know :-).
What I meant is that we grep for *every* use of open/fopen
and check that it cannot be used for mktemp races. It's
not so difficult.
Jeremy Allison
> Despite disagreeing with you about the divine nature of the Samba project
> leaders :), it seems to me that pulling the mktemp code entirely into Samba
> would be a good idea. There's enough variation in the mktemp(), mkstemp(),
> tmpnam(), etc. functions available on different Unices, and it's simple enough
> to reimplement correctly, that it might just reduce the Samba code size to do
> it all internally.
That's a good idea, we should look into grabbing the
code from glibc or something and modifying it for our
own use. Any volenteers ? :-).
Michael Sweet
> ...
> That's a good idea, we should look into grabbing the
> code from glibc or something and modifying it for our
> own use. Any volenteers ? :-).
Here's the code from CUPS that replaces the mkstemp() functionality
with something that is fairly portable; feel free to rip out the
Windows code... :)
/*
* 'cupsTempFile()' - Generate a temporary filename.
*/
char * /* O - Filename */
cupsTempFile(char *filename, /* I - Pointer to buffer */
int len) /* I - Size of buffer */
{
int fd; /* File descriptor for temp file */
#ifdef WIN32
char tmpdir[1024]; /* Windows temporary directory */
#else
char *tmpdir; /* TMPDIR environment var */
#endif /* WIN32 */
struct timeval curtime; /* Current time */
static char buf[1024] = ""; /* Buffer if you pass in NULL and 0 */
/*
* See if a filename was specified...
*/
if (filename == NULL)
{
filename = buf;
len = sizeof(buf);
}
/*
* See if TMPDIR is defined...
*/
#ifdef WIN32
GetTempPath(sizeof(tmpdir), tmpdir);
#else
if ((tmpdir = getenv("TMPDIR")) == NULL)
{
/*
* Put root temp files in restricted temp directory...
*/
if (getuid() == 0)
tmpdir = CUPS_REQUESTS "/tmp";
else
tmpdir = "/var/tmp";
}
#endif /* WIN32 */
/*
* Make the temporary name using the specified directory...
*/
do
{
/*
* Get the current time of day...
*/
gettimeofday(&curtime, NULL);
/*
* Format a string using the hex time values...
*/
snprintf(filename, len - 1, "%s/%08x%05x", tmpdir,
curtime.tv_sec, curtime.tv_usec);
/*
* Open the file in "exclusive" mode, making sure that we don't
* stomp on an existing file or someone's symlink crack...
*/
#ifdef O_NOFOLLOW
fd = open(filename, O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, 0600);
#else
fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0600);
#endif /* O_NOFOLLOW */
}
while (fd < 0);
/*
* Close the temp file - it'll be reopened later as needed...
*/
close(fd);
/*
* Return the temp filename...
*/
return (filename);
}
--
______________________________________________________________________
Michael Sweet, Easy Software Products mi...@easysw.com
Printing Software for UNIX http://www.easysw.com
Andrew Tridgell
If we switched to using something based on mkstemp() then we would
actually open up a security hole! That is because some platforms open
the file in mkstemp() with permissions of 0666, which allows an
attacker to modify the file contents. That's why recent Linux man
pages recommend NOT using mkstemp() and instead using tmpfile().
So now of couse people will ask why we don't use tmpfile(). We don't
because it is fundamentally broken as it uses a FILE* pointer. On some
major platforms FILE* is limited to 8 bit file descriptors which means
tmpfile() would fail when Samba has more than 255 files open. Not
good.
Despite the stupid compiler warnings mktemp() (when used properly) is
the most secure option available. When something better comes along we
can consider using it, but meanwhile just put up with the stupid
compiler warnings.
Cheers, Tridge
okuy...@dd.iij4u.or.jp
>>>>> "SL" == Steve Langasek <vor...@netexpress.net> writes:
>> Jeremy, are you saying that you and Andrew are GOD or something? Or
>> are you simply talking about "doing your best".
SL> Are computers God? We expect them to flawlessly execute the Samba code
SL> every time without opening up any root holes. I think the task Jeremy and
SL> Andrew have to carry out, to make sure there aren't any identifiable security
SL> holes in the Samba code itself, is much less demanding. :)
Am I a God? No. So, I don't use the way Jeremy and Andrew does.
Rather, I simply obey the rule of Quality Control. Keeping
dangerous point into single function.
>> SO THERE'S NO PROVE.
SL> Of course there is. Programming is completely deterministic in nature.
No there's none. Because we can never have complete and bugless
information about behavior of a function. This is because we have to
RECOGNIZE the code. We still do not have any AI, nor any 'semantic
analyzer' which helps us with debugging.
# I do wish to know if there's any. I need it (^^;).
What we can have, is behavior of a function that "WE BELIEVE" how it
works. Since so, at very last moment, we have possibility of
mis-understanding. We sometime mis-understand, and according to
that, well have chance of enbugging the code. The very fact that
Jeremy and Andrew, as well as every person who have ever seen the
code, did not make any mistake, is simply due to luck.
We might have 99.99999999999999999999999999% of proof. But this will
never become 100%. And that's where Quality Control starts from.
# If there's way that we never make mistake, why bother Quality?
SL> Jeremy also said they audit *every* use of the open() function.
But that's nothing more than what he believe. We believe that he did
his best, and we know that what he've done is lot better than what I
can do. But still, that statement will not make any PROOF.
# Please keep in mind that PROVE requires 100%.
# It's not 100.0% ( which means you have 0.05% of error chance ).
SL> Changing
SL> smbd_mkstemp() into a full-fledged mkstemp() function won't change all the
SL> other open() calls that would still need auditing.
I'm talking about MAKING smbd_mkstemp(), which we right now don't
have. Forget about full-fledged mkstemp(), for it does not meet
Samba's requirement anyway.
Also, forget about mktemp()'s filename uniqueness, for we're not
using that characteristics now. What we have now as function named
smbd_mktemp(), is more dangerous than simply using mktemp().
# We are rejecting that danger at open()'s option. And this is
# only kept by J&A's contiguous work.
SL> Despite disagreeing with you about the divine nature of the Samba project
SL> leaders :),
I never thought about that. I was only talking about Quality Control,
and that of very basic. How to keep quality of Samba.
# Who care about project leaders, as long as it's being kept in the
# robust way. At least, I don't, and rather, keep me away from such
# headache. I'm having them enough at business world.
SL> it seems to me that pulling the mktemp code entirely into Samba
SL> would be a good idea. There's enough variation in the mktemp(), mkstemp(),
SL> tmpnam(), etc. functions available on different Unices, and it's simple enough
SL> to reimplement correctly, that it might just reduce the Samba code size to do
SL> it all internally.
Sorry to say, I don't have mktemp() of my own. But I do have
mkstemp() that will give filename&file being opened and that will
meet requirement of samba.
I don't really know if this code works on any unix or not. I never
tried it on unix.
Here comes the code:
/*
* mkstemp() by K. Okuyama.
* look at man page of some unix for how to use :p
*
* This code uses only 0-9 and A-Z for temporary part.
* This is good enough even against CD-ROM.
*
* Warning: This code uses rand(), but there are several implementation
* of rand(), and old type stdc rand() have to be very careful about
* low-bit quality. Meanwhile, ther are also some rand() that have
* problem with high-bit quality( like when you simply use current time ).
* Please select good enough rand() function.
*
* Warning: Note that this function does not add anything like
* "/tmp/" or "/usr/tmp/" or anything. That's what outside this
* function should do.
*
*/
#define TRYTIMES 255
#define X_MARKER 0xfe
#define X_EMPTY 0x01
#define NUMLETTERS 36
static const char letters[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
int mkstemp( char *template )
{
char *filename;
char *marker;
char *replace_start;
ssize_t templatelen;
ssize_t i;
ssize_t count_X;
if ( template == NULL ) {
errno = EINVAL;
return -1;
}
templatelen = strlen( template ) + 1;
if ( templatelen < 6 ) {
errno = EINVAL;
return -1;
}
filename = malloc( templatelen );
if ( filename == NULL ) {
errno = EINVAL;
return -1;
}
replace_start = NULL;
count_X = 0;
for ( i = templatelen - 2; i >= 0; i-- ) {
if ( template[i] != 'X' ) {
if ( count_X < 6 ) {
errno = EINVAL;
return -1;
} else {
replace_start = &(template[i+1]);
break;
}
}
count_X++;
}
for ( i = 0; i < TRYTIMES; i++ ) {
char *p;
unsigned long v;
int fd;
v = 0;
for ( p = replace_start; *p; p++ ) {
if ( v < NUMLETTERS ) {
/* seems like we ran out of bits */
/* unfair? maybe. */
v = rand();
}
*p = letters[v % NUMLETTERS];
v = v / NUMLETTERS;
}
fd = open( filename, O_CREAT | O_EXCL );
if ( fd == -1 ) {
continue;
}
memmove( template, filename, templatelen );
return fd;
}
errno = EEXIST;
return -1;
}
----
Kenichi Okuyama@Tokyo Research Lab. IBM-Japan, Co.
okuy...@dd.iij4u.or.jp
>> we do now, but word 'GOD' stands for "infinite resource", so I wont'
>> stop you. But I can give you an oracle that you will fail.
I was sure that you are (^o^).
JA> What I meant is that we grep for *every* use of open/fopen
JA> and check that it cannot be used for mktemp races. It's
JA> not so difficult.
Ah... Jeremy. "It's not so difficult" is quite different from "It'll
never be difficult". I'm sure that you can do this work in very good
quality. But still, we have chance of sneeking the problem into the
samba code. And there is chance that you might miss the change.
Kenichi Okuyama
Can you explain more deeply about this? I don't seems to understand
what you're saying.
>>>>> "AT" == Andrew Tridgell <tri...@linuxcare.com> writes:
AT> We continue to use mktemp() because the alternatives are worse.
AT> If we switched to using something based on mkstemp() then we would
AT> actually open up a security hole! That is because some platforms open
AT> the file in mkstemp() with permissions of 0666, which allows an
AT> attacker to modify the file contents. That's why recent Linux man
AT> pages recommend NOT using mkstemp() and instead using tmpfile().
There seems to be another selection, make our own mkstemp/tmpfile or
whatever. I don't think it's good idea to use stdlib's mkstemp()
too, but I don't think it's good idea to use mktemp() as well, and
also, not having mkstemp() type of function that hides every messy
thing that we have to deal with for temporary files, seems bad idea
for me as well.
As far as I can understand, mkstemp() of stdlib does not meet our
request because of two reason ( not just one ).
1) mkstemp() sometimes create file with name not appropreate for us.
We have no way to control which character will be used for
creating temporary filename.
2) mkstemp() create temporary file with permission of 0666.
Doesn't these simply means that we need mkstemp() of our own? We
need both filename generator that only uses characters that we
require, and with 'mode' parameter, aren't we?
# And according to what I understand so far, creating mkstemp()
# is QUITE easy, if we have good enough random generator.
# And I believe we already do, aren't we?
Why do you stick to using mktemp()? I mean, mktemp() doesn't meet
reason 1) anyway, why try to use it?
Robert Dahlem
wrote:
>Sorry to say, I don't have mktemp() of my own. But I do have
>mkstemp() that will give filename&file being opened and that will
>meet requirement of samba.
>Here comes the code:
[...]
> filename = malloc( templatelen );
Doesn't this produce a memory leak?
Regards,
Robert
--
---------------------------------------------------------------
Robert...@gmx.net Fax +49-69-432647
---------------------------------------------------------------
Sent using PMMail (http://www.pmmail2000.com) - fast, decent, email
software; far better than Outlook. Try it sometime.