IRC correctness checks

[Ethan Blanton]

All,

Daniel noticed a problem with IRC whois parsing, where the server
message arguments are not checked for existence before they are used.
This is a remote crasher in the event of a malicious server or MitM.
At a quick glance through msgs.c in the IRC prpl, there may be some
other places where this happens.

In discussion with Daniel, we both agreed that the parser could be
doing more for the prpl here.  In particular, while it is the case
that there are optional arguments in IRC and servers with differing
numbers of arguments, in many cases there are certain arguments that
we just Have to Have in order to parse a message correctly.  In these
cases, the parser could be enforcing the minimum number of required
arguments, and perhaps logging a message if they are not met.  For
example, at the top of irc_msg_features we have:

if (!args || !args[0] || !args[1])
    return;

In this case, we could simply not call irc_msg_features if less than
two parsed arguments are provided by the server.

I propose adding an int required field to struct _irc_msg, and
enumerating the number of required arguments for every IRC message in
parse.c.  Many of them require all provided arguments; for example,
005 would become:

    { "005", "n*", 2, irc_msg_features }

The if (!args...) check in irc_msg_features could then be removed
entirely.  Determining how many arguments are (in practicality)
required for each numeric would have to be determined as follows:

1) If there is an explicit check as above, with no action taken on
   check failure, there's your number of required arguments (modulo
   #2).

2) If an argument is used without checking (e.g., args[5] for 311 and
   314 in irc_msg_whois), then the number of required arguments must
   cover that argument (6, in this case).

3) If there is an explicit check for an argument, and the command
   still does *something* if that argument doesn't exist, but the
   argument is the smallest such argument used, then the number of
   required arguments is the largest argument used of lesser position.
   (This may take a read or two to make sense, I'll clarify if there
   is confusion.)

In addition, after such a pass is done, msgs.c needs to be audited for
bogus argument usages.  (E.g., an explicit check for args[2] that
allows the command to proceed if it fails, but then args[3] is used
without a check.)  What I've been seeing doesn't make me confident
that never happens.  ;-)

I'll own this problem.  It's my mistake.  There are two things I did
wrong when writing the IRC prpl: not having a required arguments
count, and not passing an argc to each msgs function.  Making the
latter change is probably also not a bad idea at this point, but I
don't think it's as important as the former, and an audit of msgs.c.

I'm writing this email because neither Daniel nor I feels we have the
cycles to spend on this at this point.  I'm hoping someone else here
does.  I don't know if this can be worked in before 2.10.8 or not; if
it can, it needs a CVE.

Tomasz, I'm sure your official security time is burned up at this
point, but is there any chance you might be able to look at this?
Anyone else?

Ethan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: Digital signature
URL: <https://pidgin.im/cgi-bin/mailman/private/security/attachments/20140114/ff889c75/attachment.sig>