Re: Unchecked sprintf/strcat?
* Mon May 29 2006 Rocco Rutte <pdmef@xxxxxxx>
> * TAKAHASHI Tamotsu [06-05-29 23:13:05 +0900] wrote:
> >1) Simple miscalculation about sizeof(helpstr), while they are CHECKED.
> >| mutt_make_help (buf, sizeof (buf), _("Exit "), menu_to_use, OP_EXIT);
> >| strcat (helpstr, buf); /* __STRCAT_CHECKED__ */
>
> While sizeof(buf)<=sizeof(helpstr) I see no real problem. If this
> doesn't hold, maybe someone forgot to update/re-do the checks... which
> we should do anyway now.
As we don't re-check all the insecure functions regularly,
I prefer safe_strcat.
In fact, mutt_ssl.c and mutt_ssl_gnutls.c use safe_strcat.
And, smime.c uses helpstr[HUGE_STRING*3]. This is long enough,
but HUGE*3 is quite a waste of memory.
http://www10.plala.or.jp/sanrinsha/tamo/safestrcat.diff
> >2) Silent truncation like bug #2205.
>
> This bad, I agree. But we already have BUFFER and it could be used to
> grow as needed regarding the input (like it's used for parsing config
> files). But that would have drastic impact on the speed if used
> everywhere (since that would be malloc() while now we don't need it).
A long patch is here:
http://www10.plala.or.jp/sanrinsha/tamo/patch-1.5.11cvs.tamo.dynbuf.1
- This patch uses dynamic BUFFER for mailcap commands.
- This patch replaces most of IMAP static arrays with dynamic
BUFFER (in order to eliminate truncation and overflow - grep
"XXX" and "FIXME"). I see no drastic performance risk so far
although I don't use IMAP everyday.
- Fix browse_get_namespace() which could overflow ns[LONG_STRING].
(Possible remote vuln?)
Fix msg_parse_fetch() tmp[SHORT_STRING], too.
- And this patch checks the length of arguments for "set",
"source", "mailboxes" commands.
I hope some of these are useful. (But I'm sure there are many
more useful patches waiting to be applied.)
--
tamo