Veracode static analysis results

[Elliott Sales de Andrade]

On Thu, Dec 6, 2012 at 1:38 AM, Eion Robb <eion at robbmob.com> wrote:
>
> On Thursday, 6 December 2012, Elliott Sales de Andrade wrote:
>>
>> On Wed, Dec 5, 2012 at 4:22 PM, Ethan Blanton <elb at pidgin.im> wrote:
>> > Chris Wysopal spake unto us the following wisdom:
>> >> A customer asked us to analyze Pidgin using our static analyzer.  Our
>> >> responsible disclosure policy is to inform you of any findings so that
>> >> you may have the chance to review, comment, and/or fix the issues.
>> >>
>>
>> It would probably be better if they checked the prpls too. Those're
>> more likely to have problems.
>>
>> >> I think the software performed very well on our analysis but there are
>> >> a few issues we have found.  Attached is our full report. You can find
>> >> the description of the issues found on pages 10-15. We found 1 Very
>> >> High criticality. 5 Medium, and 47 low. Here is a summary.
>> >
>> > OK, here's my fifteen minute analysis of the bugs.  There's only one I
>> > think I'd really worry about.  I've not Cc'd veracode, we can send
>> > them our final conclusions.
>> >
>> > Very High:
>> >
>> > * gtkpounce error is a false positive.  Yes, we execute a user path
>> >   without verifying it, but that's the whole *point* of that feature.
>> >   It's not particularly safe, but only in an "enough rope to hang
>> >   yourself with" kind of way.
>> >
>> > Medium:
>> >
>> > * NTLM session key -- I don't know enough about NTLM to say if this is a
>> >   real problem or not.  Using a real RNG certainly wouldn't hurt.
>> >
>> > * purple_core_migrate user-specified path is a false positive.  Pidgin
>> >   can already be coerced to read any file the user can read, and in the
>> >   general sense *should* be able to do so.  This class of bug simply
>> >   doesn't apply to Pidgin.
>> >
>> > * PurpleDesktopItem creation from file -- I don't even know what this
>> >   is.
>> >
>>
>> I think this is only used in Pidgin for reading .desktop files that
>> are dropped on the conversation. If it's a link, then that link gets
>> added to the entry. Anything else seems to be ignored.
>>
>> > * write_data_to_file path problem -- see purple_core_migrate
>> >
>> > * write_data_to_file race -- this is real.  We should be using open()
>> >   and fdopen() (or the g_ equivalents thereof?).
>> >
>> > Low:
>> >
>> > I'm not even going through these right now.  Some of them probably merit
>> > checks; the majority of the 47 are in imported code from glib or
>> > something, though.
>>
>> Only 6 of them are in our code. I'm not sure what version of the code
>> they verified, but they don't match hg (3.0.0) and I didn't bother
>> looking at 2.10.7.
>
> The analysis was on 2.10.6 on "Linux"
>

OK, I had a look at the 6 low issues.

4 - pidgin home/.../pidgin/gtkprefs.c 729 9085
4 - pidgin home/.../pidgin/gtkprefs.c 732 9056
4 - pidgin home/.../pidgin/gtkprefs.c 773 9055
4 - pidgin home/.../pidgin/gtkprefs.c 780 9058

These are all `g_rename` and `g_remove` for themes that are dragged
onto the prefs window. `g_rename` the temp file/directory into the
`~/.purple` theme directory and `g_remove` the temporary one. Maybe
the `g_rename` should be checked.

5 - libpurple.so home/.../libpurple/log.c 838 8992

This is an `unlink` used to remove a logged image if it's not saved
correctly, so the return value isn't too important. However, maybe
this one should be g_unlink since we use g_fopen?

7 - libpurple.so home/.../libpurple/util.c 1537

This is a `strchr`, but the return value is guaranteed to exist due to
the `if()`  around it.

-- 
Elliott aka QuLogic
Pidgin developer