Created attachment 84127 [details] [review]
patch against 2.0.3

This is a patch against 2.0.3; I haven't had time to try and port it to HEAD, but did review the HEAD code which appeared to still be vulnerable.

I'll attach a similar patch against 2.0.2 - in addition to fixing the issues it also adds G_GNUC_PRINTF annotations to the relevant prototypes. I believe my patch catches all of these issues.

These annotations together with -Wformat-nonliteral (which is not included in -Wall) will make gcc warn about similar issues. Probably worth doing.

Created attachment 84130 [details] [review]
Patch against 2.0.6

Actually, the patch is against 2.0.6 (which is what is in Fedora 6), not 2.0.2.

bah, against 2.0.5

Thanks! I haven't time to apply them just now, but I'll try to have a look later today (probably this evening).

If any other committer can beat me to it, do it!

CVE-2007-0999 for these new format string flaws

Hi Kees!

Thanks very much for this patchset. I corrected the first format string issue some weeks ago. I didn't correct these things (-> patch) mainly because of
a) lack of time
b) it's not *that* critical because these strings are not configurable/external

So regarding a) I'm very happy you did it. I reviewed the 2.0.5 patch, and it shouldn't be a problem to apply it 1:1 to HEAD, as these code sections didn't change much in the past. I will do it this evening (also a commit to gnome-2-14 branch [upcoming 2.0.6 release] will be done).

@Snark: I'll apply them today, if you want.

Regards and thanks again,
Jan

Jan: will the 2.0.6 release be done for GNOME 2.18.0?

You should ask Damien for that, but I think it's planned for the Gnome release or around the release.

J.

I'll do it this week-end.

Does it make sens to have calls of the form foo(..., "%s", msg) all over the place? Shouldn't the status bar directly take the message as a const gchar *?

This would also avoid playing with G_GNUC_PRINTF annotations which make the code less clear -- especially since the patch probably makes ekiga uncompilable with another compiler : it doesn't use #ifdef #endif to check for it before using them.

If G_GNUC_PRINTF isn't defined, it will substitute to nothing, hence it should be portable IMHO.

J.

The preprocessor will substitute things it knows about, but won't touch it if it doesn't... hence the compiler _will_ complain about a syntax error.

jpuydt@hilbert:/tmp$ cat test.c
#include <stdio.h>

int
main (int argc,
      char *argv []) MY_COMPILER_ANNOTATION
{
  printf ("Hello\n");
  return 0;
}
jpuydt@hilbert:/tmp$ gcc test.c -o test
test.c: In function ‘main’:
test.c:5: error: expected declaration specifiers before ‘MY_COMPILER_ANNOTATION’
test.c:9: error: expected ‘{’ at end of input

Modifying the api shouldn't require that much work :
jpuydt@hilbert:~/Ekiga/ekiga.svn$ grep -r . -e gm_statusbar_push_info_message | grep -v ".svn" | wc -l
4
jpuydt@hilbert:~/Ekiga/ekiga.svn$ grep -r . -e gm_statusbar_push_message | grep -v ".svn" | wc -l
3
jpuydt@hilbert:~/Ekiga/ekiga.svn$ grep -r . -e gm_statusbar_flash_message | grep -v ".svn" | wc -l
5

Should I proceed?

If you want to do it, yes. But tell me soon, I just came home and was about to start.

For the macro: GLib always defines that macro.

J.

(In reply to comment #7)
> b) it's not *that* critical because these strings are not configurable/external

Sure, I was just being complete for some of the calls.  However, I just want to make sure it's understood that 2.0.5 is still vulnerable to external string attacks.  (It's just that the statusbar is no longer being overflowed, now the history window is vulnerable.)

I'm happy to email a proof-of-concept attack/crash, but I'd rather not post it publicly to the bug.  :)

Snark, the G_GNUC_PRINTF macros are always defined. The glib header file in question contains the appropriate #ifdef. If it isn't being compiled by gcc it will expand to nothing.

Yes, Jan corrected me on this on irc : I had missed the G_... and only saw the GNU... brain parser mistake ;-)

Committed to GNOME branch, including a fix for a practical remote exploit. Now porting to HEAD.

Committed to HEAD, including the same fix mentioned above.
Damien: Leaving open until you tell what to do with H-Release, though it should be easy to do.
Kees&Mark: Thank you very much.
Selfnote: Hope that didn't break anything I have overseen so far :)

J.

Please commit to H_RELEASE too.

Scheduled for this evening.

Committed. Still leaving open, because I forgot one of the chat_window_*() calls in manager.cpp in HEAD and GNOME, I recognized it when adapting H_Release.

Okay, also fixed in HEAD and GNOME
Closing... Thanks!

Don't forget to update the NEWS file.

And thanks for your work !