521679 - Fix NSS build warnings: "format not a string literal and no format arguments"

Status: RESOLVED FIXED

(NSS :: Tools, defect, P4)

Description

[Daniel Holbert [:dholbert]]

Attachment: fix: s/printf(foo)/printf("%s", foo)/

When building mozilla-central, I get 6 warnings of this type in NSS code:

> secutil.c:1066: warning: format not a string literal and no format arguments

This is GCC detecting a potential format string attack (
http://en.wikipedia.org/wiki/Format_string_attack ).  This is mostly an
issue when the printed string is user-controlled, but it's trivial to fix, so I
don't see why we shouldn't just fix it.

The attached patch fixes 6 instances of this problem in NSS.  (Combined with the patch on bug 521677, this fixes all warnings of this type that I hit when building current mozilla-central.)

Comment 1

[Daniel Holbert [:dholbert]]

dbaron pointed out in bug 521677 that |fprintf(stream, "%s", str)| calls can be shortened to |fputs(str, stream)|.

I can do that for the affected fprintf calls in this bug's patch, too, if desired.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405792 [details] [diff] [review]
fix: s/printf(foo)/printf("%s", foo)/


>-    fprintf(stdout, str);
>+    fprintf(stdout, "%s", str);
>     fprintf(stdout, " > ");

If the first of those fprintf calls is wrong, then why isn't the second?
I claim neither is wrong.

Comment 3

[Daniel Holbert [:dholbert]]

The first is "wrong" because we don't know what "str" contains -- if it contains e.g. "%s", then fprintf(stdout, str) could start printing out memory from the stack and/or crash.

The second is not wrong because you can tell from inspection (and your compiler can tell from static checking) that it doesn't contain any percent-strings, so it won't do anything nasty.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

It's a static function with a small finite number of callers in the same
source file.  One can tell by inspection of the source file whether it is 
correct or not.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> It's a static function with a small finite number of callers in the same
> source file.

Well, it's called 19 different times (and it often passes in long multi-line strings).  That's on the large side of "small".

> One can tell by inspection of the source file whether it is
> correct or not.

But when the fix is so trivial (and commonly understood to be The Right/Secure Thing To Do), why not use the safer style, so that there's no reason to worry about inspecting 19 different callers in the first place?

And most importantly from my point of view, why not fix this and eliminate 12 lines of useless warning-spam from every NSS & mozilla build? :)  (12 lines = 6 warnings * 2 lines per warning)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405792 [details] [diff] [review]
fix: s/printf(foo)/printf("%s", foo)/

The answer to the "why not" question is that there are a whole bunch of serious security bugs that the NSS team should be working on, and working on this cosmetic flaw in some QA test tools takes people away from the more important work.

Comment 7

[Daniel Holbert [:dholbert]]

Fair enough -- clearly this isn't a blocker bug. :)

This bug is just to scratch an itch -- to try and cut down on the number of build warnings generated by mozilla-central -- but I'm not arguing that this needs to be addressed ASAP or anything.

Comment 8

[Elio Maldonado]

Comment on attachment 405792 [details] [diff] [review]
fix: s/printf(foo)/printf("%s", foo)/

+1, using fputs would be nice but not required

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix: now using fputs

Fixed up the previous patch to prefer fputs in all cases, except for with the "FPS" macro in nss/cmd/certutil/keystuff.c  (where FPS is #defined as "fprintf(stderr,")

Comment 10

[Daniel Holbert [:dholbert]]

diff from old patch to new patch is available here:
 http://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/609d2aaa96ac

Comment 11

[timeless]

dholbert: bugzilla can do that for us:

https://bugzilla.mozilla.org/attachment.cgi?oldid=405792&action=interdiff&newid=409449&headers=1

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Bug 521679: Fix warnings: "format not a string literal and no format arguments" 
Patch contributed by Daniel Holbert <[e-mail]>, r=nelson

Checking in certutil/certext.c;  new revision: 1.11; previous revision: 1.10
Checking in certutil/keystuff.c; new revision: 1.22; previous revision: 1.21
Checking in lib/secutil.c;       new revision: 1.99; previous revision: 1.98
Checking in pk12util/pk12util.c; new revision: 1.43; previous revision: 1.42