What is the potential problem?

[happy]

I read an unanswered interview question somewhere :

#include <stdlib.h>
#include <stdio.h>
void Error(char* s)
{
printf(s);
return;
}

int main()
{
int *p;
p = (int*)malloc(sizeof(int));
if(!p)
{
Error("memory error");
Error("Quitting....\n");
exit(1);
}
else
{
/*some stuff to use p*/
}
free(p);
return 0;
}

Find potential problem with the error function.. a security concern..

Please tell me what can be the potential problem. Is this the printf
(p) because p can contain conversion specifiers?

 

[Stefan Ram]

Please tell me what can be the potential problem. Is this the printf
(p) because p can contain conversion specifiers?

Yes, but if the function would be »static«, it would be more secure,
because then it would not be possible to call it from other linking
units, and in /this/ compilation unit, it is only called in:

Error("memory error");
Error("Quitting....\n");

where there are no conversion specifiers visible.

 

[bartc]

Yes, but if the function would be »static«, it would be more secure,
because then it would not be possible to call it from other linking
units, and in /this/ compilation unit, it is only called in:


where there are no conversion specifiers visible.

An external routine may not know that "%" characters are special in the
message strings passed to Error().

But that they would just make it go wrong, I can't see a security issue. And
if the caller of Error() was malicious, he could just call his own printf()
function with whatever he likes.

 

[Eric Sosman]

I read an unanswered interview question somewhere :

#include<stdlib.h>
#include<stdio.h>
void Error(char* s)
{
printf(s);
return;
}

int main()
{
int *p;
p = (int*)malloc(sizeof(int));
if(!p)
{
Error("memory error");
Error("Quitting....\n");
exit(1);
}
else
{
/*some stuff to use p*/
}
free(p);
return 0;
}

Find potential problem with the error function.. a security concern..

Please tell me what can be the potential problem. Is this the printf
(p) because p can contain conversion specifiers?

That would be my guess at what the question's author was
trying to get at. The question is poorly framed, though,
since we don't have enough context. There is no "security
concern," for example, if all the Error strings are provided
by a trusted source, someone who knows not to put percent
signs in them.

There are other potential problems, depending on what you
consider a "problem." For example, the output might not appear
at all if the caller of Error forgets to end with a '\n' (in
the example, the output would be "memory errorQuitting....\n"
because Error does not supply its own line break). Also, Error
sends its messages to stdout, whereas they probably ought to go
to stderr.

There's also a useless `return' statement, and the caller
has a useless cast and a non-portable exit status. All in all,
I'd say the question is poorly composed.

 

[Stefan Ram]

An external routine may not know that "%" characters are special in the
message strings passed to Error().

This could also be attributted to the lack of proper
documentation of this function. Thus, documentation also has
security benefits/implications. So, one answer to the
original question might be: The lack of documentation makes
it impossible to say whether the program has the intended
behavior, because we do not know what it is intended to do.

And not being able to say what a program is supposed to do
might be deemed a security problem, because

- failures in the implementation might go unnoticed and

- the program might be used/called under circumstances
or for reasons for which it has not been designed.

But that they would just make it go wrong, I can't see a security issue.

If the string was read in, it could have been created
in order to support an attack.

 

[Kalle Olavi Niemitalo]

But that they would just make it go wrong, I can't see a security
issue. And if the caller of Error() was malicious, he could just call
his own printf() function with whatever he likes.

The security issue is if the caller is not malicious but is
passing a string from a malicious source, e.g. if an authorized
user is running the program to view some file on a floppy disk
that he found lying on the street next to the office.

Most conversion specifications in this printf call would just
cause printf to read a va_arg that was not provided by the
caller, and then either display it (which as such may already
write some secret to a place that unauthorized people can see) or
dereference it as a pointer and possibly crash (for "%s"), but
"%n" is unusual in that it actually writes to memory. The
attacker can control the value written (i.e. the number of
characters output so far), and by suitably corrupting the stack
or other data structures, he may be able to lure the program
into executing some code of his choosing.

 

[Paul N]

     That would be my guess at what the question's author was
trying to get at.  The question is poorly framed, though,
since we don't have enough context.  There is no "security
concern," for example, if all the Error strings are provided
by a trusted source, someone who knows not to put percent
signs in them.

     There are other potential problems, depending on what you
consider a "problem."  For example, the output might not appear
at all if the caller of Error forgets to end with a '\n' (in
the example, the output would be "memory errorQuitting....\n"
because Error does not supply its own line break).  Also, Error
sends its messages to stdout, whereas they probably ought to go
to stderr.

     There's also a useless `return' statement, and the caller
has a useless cast and a non-portable exit status.  All in all,
I'd say the question is poorly composed.

In addition to what the others have said - the function "Error"
doesn't actually stop the program, which one might expect it to do.