148272 - flawfinder warnings in directory

Status: RESOLVED WORKSFORME

(Directory :: LDAP C SDK, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I run flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 branch.

flawfinder found 212 warnings in directory/c-sdk code (861-1072). Go through
that list and for each warning:

* If it is false positive, comment here why it is not an issue
* If it is a real issue, make patch for it here and let's get them checked in

In addition the checking the branch, also check the trunk.

I will attach an excerpt of the log since the full log is so big and inside NS
firewall.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: flawfinder warnings

Comment 2

[Stephen P. Morse]

Attaching another 121 flawfinder warnings in direct

Comment 3

[Stephen P. Morse]

Attachment: more flawfinder warnings

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The files have changed so line numbers don't always match, use find to find the
actual place...

All the warnings up to 880 are gone on trunk since the files no longer exist.

880 appears to be a command line utility, so it is perhaps not of interest.
However, it does have an exploitable buffer overflow because it uses strcpy to
copy into a static buffer of size 128 and the data comes from command line.

881-933 again no longer apply for the trunk.

934 is debug only code which is disabled by default, so probably not a problem
currently. However, there is a fixed static buffer of size 256 to which we write
debug messages and the users of this debug logging macro are probably unaware of
the size limitations.

935 is just function forward declarations, flawfinder will notice the usage of
this function as well.

936 file no longer on trunk

937-938 debug code so didn't look

939 debug

940 uses sprintf to write to a static buffer of size 80 in error cases; might be
safe but I wouldn't want to go through all the callers and possible arguments to
this function, I'd just make this safe which is less work

941 debug

942 see 940 above...

943 - 950 debug

951 is safe, writing int to buffer of 50

952 - 953 is safe, allocates enough dynamic memory for copy

954 debug

955 - 959 allocates big static buffer (1024), then copies some error messages
into that. Might be safe, but I would simply make this safe here by dynamically
allocating enough. Less work than going through all the call sites...

960 might be safe but it copies stuff into fixed size buffer (256) and I don't
know how big this "stuff" could be

961 is safe, uses static buffer of size 4 and copies 3+NULL

That is about half way through the trunk. No obvious holes, but there might be
some in the cases that I indicated. We should make them bulletproof. Module
owners please finish this task...

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Could we please get some progress on this bug?

Comment 6

[Mark Smith [:mcs]]

Raising priority and accepting bug. The initial focus will be on problems that
are inside the core LDAP library code that is widely used (e.g., by the Mozilla
client).

Comment 7

[Frankie]

Spam for bug 129472

Comment 8

[Mark Smith [:mcs]]

There are a lot of flawfinder warnings to review. Most of them are benign. I
installed flawfinder myself so I can regenerate the warning list easily. One
question: do we want to add the special /* flawfinder... */ comments for code
that has been reviewed and deemed OK?

I will attach a patch for the most critical problems under
mozilla/directory/c-sdk/ldap/libraries (use of unchecked buffers with snprintf).

Comment 9

[Rich Megginson]

Seems like a good idea to include /* flawfinder ... */ comments near fixes made
to fix problems found by flawfinder.

Comment 10

[Mark Smith [:mcs]]

940 and 942 are OK because the sprintf() parameter is a char (not a char *).

The patch I am about to post addresses 955 - 960 (by using snprintf() on
platforms that have it).

962-968 and 975 are OK because the right amount of space is malloc'd.

969 and 970 are OK because the destination buffers are large enough.

971-974 and 976-995 are in example code which is not built.

996 and 997 are in 16-bit Windows-only code (!)

998-1007 are in debug code.

1008-1010 are OK because the correct amount of space is malloc'd.

1011-1026 are in test code that is not built.

1027-1058 (tmplout.c) are in code that is built but not used in the mozilla
client. But the problems will need to be fixed in case someone starts to use
this code (or we will have to remove the code).

1059 is OK because the copied string is always shorter than the destination.

1060 and 1061 are OK because the correct amount of space is malloc'd.

1062 is OK because the destination is big enough to accomodate the source
(static) string.

1063 is OK; the format strings are statically defined in the nearby code.

1064 is OK because the correct amount of space is malloc'd.

1065, 1066, and 1069 refer to forward declarations.

1067 and 1068 are in dead code that is not compiled.

1070 is OK because the correct amount of space is malloc'd.

1071 and 1072 are OK because the destination buffers are large enough.

The warnings in Attachment 102694 [details] are repeats of those in Attachment 85960 [details] or
are in command line utilities that are not used in the mozilla client (but I
will go through those warnings in detail).

Comment 11

[Mark Smith [:mcs]]

Oops. Make that Attachment 85690 [details].

Comment 12

[Mark Smith [:mcs]]

Attachment: proposed fix for most critical warnings

Comment 13

[Rich Megginson]

Fix looks good.

Comment 14

[Mark Smith [:mcs]]

Partial fix committed to the trunk:

mozilla/directory/c-sdk/ldap/include/portable.h
  new revision: 5.7; previous revision: 5.6
mozilla/directory/c-sdk/ldap/libraries/libldap/error.c
  new revision: 5.2; previous revision: 5.1
mozilla/directory/c-sdk/ldap/libraries/libldap/getfilter.c
  new revision: 5.2; previous revision: 5.1
    Partial fix for 148272 - flawfinder warnings in directory.
      Fix most critical warnings in the core LDAP library code:
        AIX has snprintf() so we now #define HAVE_SNPRINTF there.
        Use snprintf() instead of sprintf() in ldap_perror().
        Use snprintf() instead of sprintf() in ldap_init_getfilter_buf()
         and improve error reporting for bad regular expressions.

These changes should also be made on the branch used by the Mozilla client
(ldapcsdk_50_client_branch).

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I haven't required adding those Flawfinder comments to code, and so far nobody
has done that. We intend to use other tools besides Flawfinder as well, so
Flawfinder warnings would not help with them. Some tools may be able to
recognize those comments from several tools (for example Flawfinder understand
RATS and ITS4 (?) comments in addition to its own). I think I am slightly
against adding those comments.

The hope is that we will have Flawfinder warnings reported on Tbox, possibly
with diff from last pull so that it would be easy to spot & fix any potential
new issues.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 119110 [details] [diff] [review]
proposed fix for most critical warnings

sr=[e-mail]

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Closing all still open flawfinder bugs as WORKSFORME because there are now much better tools that are being used (Coverity, Klocwork).

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Closing all still open flawfinder bugs as WORKSFORME because there are now much better tools that are being used (Coverity, Klocwork).

Comment 19

[Simon Paquet [:sipaq]]

Comment on attachment 119110 [details] [diff] [review]
proposed fix for most critical warnings

Since this bug has been resolved as WFM by the reporter, I'm removing the review request.