#2846: Security vulnerability in APOP authentication
On Sunday, 18 March 2007 at 17:36, Rocco Rutte wrote:
> I was looking at some mutt code for this issue and other issues that
> report broken threading upon invalid message-ids. It seems that mutt
> happily accepts the following syntax: '<.*>' which is just plain wrong.
>
> I looked at rfc822.c to try to reuse address parsing for parsing
> message-ids but failed since I didn't have much time and the quote is
> quite complex.
>
> Even though adopting your code for mutt would be quite easy, I'm not
> yet sure what to do in case of validation errors.
>
> Say we get '<foobar>' during APOP authentication; should be really
> reject that and report failed authentication? If a message has
> '<foobar>' as message-id and others have it in their References:
> header, should we really ignore it and break threading?
Here's a patch that does a really minimal check that the message ID is
of the form <x@y> where x and y are between ASCII 0 and 127. I hope
that it's enough to thwart the MD5 collision attack, but liberal
enough to tolerate the range of broken POP servers out there. The @y
test could be easily removed if necessary.
Comments?
# HG changeset patch
# User Brendan Cully <brendan@xxxxxxxxxx>
# Date 1175552458 25200
# Node ID aad620cbe1bfc79a933a5f3dcf466b141f890ed6
# Parent eabef30c9344f388dffcf4e4d9d6637229f6957c
Validate msgid in APOP authentication. Closes #2846
diff -r eabef30c9344 -r aad620cbe1bf pop_auth.c
--- a/pop_auth.c Mon Apr 02 14:33:29 2007 -0700
+++ b/pop_auth.c Mon Apr 02 15:20:58 2007 -0700
@@ -183,6 +183,13 @@ static pop_auth_res_t pop_auth_apop (POP
if (!pop_data->timestamp)
return POP_A_UNAVAIL;
+ if (rfc822_valid_msgid (pop_data->timestamp) < 0)
+ {
+ mutt_error _("POP timestamp is invalid!");
+ mutt_sleep (2);
+ return POP_A_UNAVAIL;
+ }
+
mutt_message _("Authenticating (APOP)...");
/* Compute the authentication hash to send to the server */
diff -r eabef30c9344 -r aad620cbe1bf rfc822.c
--- a/rfc822.c Mon Apr 02 14:33:29 2007 -0700
+++ b/rfc822.c Mon Apr 02 15:20:58 2007 -0700
@@ -792,6 +792,52 @@ ADDRESS *rfc822_append (ADDRESS **a, ADD
return tmp;
}
+/* incomplete. Only used to thwart the APOP MD5 attack (#2846). */
+int rfc822_valid_msgid (const char *msgid)
+{
+ /* msg-id = "<" addr-spec ">"
+ * addr-spec = local-part "@" domain
+ * local-part = word *("." word)
+ * word = atom / quoted-string
+ * atom = 1*<any CHAR except specials, SPACE and CTLs>
+ * CHAR = ( 0.-127. )
+ * specials = "(" / ")" / "<" / ">" / "@"
+ / "," / ";" / ":" / "\" / <">
+ / "." / "[" / "]"
+ * SPACE = ( 32. )
+ * CTLS = ( 0.-31., 127.)
+ * quoted-string = <"> *(qtext/quoted-pair) <">
+ * qtext = <any CHAR except <">, "\" and CR>
+ * CR = ( 13. )
+ * quoted-pair = "\" CHAR
+ * domain = sub-domain *("." sub-domain)
+ * sub-domain = domain-ref / domain-literal
+ * domain-ref = atom
+ * domain-literal = "[" *(dtext / quoted-pair) "]"
+ */
+
+ char* dom;
+ unsigned int l, i;
+
+ if (!msgid || !*msgid)
+ return -1;
+
+ l = mutt_strlen (msgid);
+ if (l < 5) /* <atom@atom> */
+ return -1;
+ if (msgid[0] != '<' || msgid[l-1] != '>')
+ return -1;
+ if (!(dom = strrchr (msgid, '@')))
+ return -1;
+
+ /* TODO: complete parser */
+ for (i = 0; i < l; i++)
+ if (msgid[i] > 127)
+ return -1;
+
+ return 0;
+}
+
#ifdef TESTING
int safe_free (void **p) /* __SAFE_FREE_CHECKED__ */
{
diff -r eabef30c9344 -r aad620cbe1bf rfc822.h
--- a/rfc822.h Mon Apr 02 14:33:29 2007 -0700
+++ b/rfc822.h Mon Apr 02 15:20:58 2007 -0700
@@ -52,6 +52,7 @@ void rfc822_write_address_single (char *
void rfc822_write_address_single (char *, size_t, ADDRESS *, int);
void rfc822_free_address (ADDRESS **addr);
void rfc822_cat (char *, size_t, const char *, const char *);
+int rfc822_valid_msgid (const char *msgid);
extern int RFC822Error;
extern const char *RFC822Errors[];