6

A recent security notice (http://osdir.com/ml/bugtraq.security/2015-04/msg00102.html) stated that this line of code:

fprintf(stderr, (isprint(adata->contents[i])) ? "%c " : "%02x", adata->contents[i]);

was subject to a "format string attack" which I understand as using something like:

fprintf(stderr, varWithUserSuppliedData);

instead of:

fprintf(stderr, "%s", varWithUserSuppliedData);

but I'm not seeing that in that first fprintf call - what am I missing?

Improve this question
8
  • 9
    I don't see anything wrong with that fprintf call but I suspect that automatic program analysis tools could generate a false positive. But I'm not going to risk an answer yet :) Maybe someone will come up with a compelling argument as to why it is dangerous. Commented Jul 10, 2015 at 15:23
  • 1
    I've tried to see something that would be incorrect, however it all looks ok. I'm with you rici, I'll await a possible answer. Commented Jul 10, 2015 at 15:27
  • this has got to be a lame tool that say the word after stderr not starting with the character " Commented Jul 10, 2015 at 15:42
  • can you try char *a = "%s"; printf(a, " hi"); will this make false positive? Commented Jul 10, 2015 at 15:46
  • 1
    I think the key words in the link are "prone to"... I think the claim is not that this is actually exploitable, but rather, that it's generally an insecure practice to allow untrusted data to influence a format string. Commented Jul 10, 2015 at 15:50

3 Answers 3

Reset to default
4

Thanks to @cremno for providing a link to the GIT repository for the file in question: kssl.c (Note that this is not the repository head.)

It's clear that this report is spurious. First, there is no real problem with the fprintf call, although you can argue that code in a security-related product like OpenSSL needs to go beyond being secure to the point of being visibly secure even to a casual glance. (I'm not sure I would make that argument, but it has been made.)

But more importantly, the code in question is disabled (note the preprocessor directives surrounding it):

# if 0
{
    int i;
    fprintf(stderr, "%s[at%d:%d] ", label, adata->ad_type, adata->length);
    for (i = 0; i < adata->length; i++) {
        fprintf(stderr, (isprint(adata->contents[i])) ? "%c " : "%02x",
                        adata->contents[i]);
    }
    fprintf(stderr, "\n");
}
# endif
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Good to know my understanding of format string attacks remains viable! :)
2

This looks like an automatically generated error message. Obviously the tool doesn't know which format string will be used, so it cannot analyse the format string and the arguments and declare that the usage is safe.

A more clever tool might figure out that there are just two possibilities for the format string, and that each possibility is safe, and not give an error message.

The code itself is safe. Of course you get rid of the message by using an if/else statement. And in safety critical code, you wouldn't just fix things that are wrong, but also things that look wrong. And you never know, after turning the fprintf into an if/else, the tool might detect a real problem that we all missed.

Improve this answer

Comments

0

Credit must go to rici, but everything looks ok with the code.

Even there is a problem, everything can be printed safe as char or as hex.

Probably the problem could be with the for or while loop that operates over the string.

Could you post the loop as well?

Improve this answer

2 Comments

github.com/openssl/openssl/blob/OpenSSL_1_0_2a/ssl/… (note the purpose of the function is debugging and also the #if 0)
And I think the debug wouldn't be formatted that nicely! :-)

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.