Merge lp:~ralfjung-e/mailman/csrf-injective into lp:mailman/2.1

[Ralf Jung]

Separate data in CSRF token by colon to avoid collisions.

This makes the data-to-token function injective. Previously, for example, the
list called "list1" and the IP "10.0.0.0" would have the same hash as the list
called "list" and the IP "110.0.0.0", as the strings were just concatenated.

[Jim Popovitch (jimpop)]

A couple thoughts:

1) The original code in listinfo.py uses 'now' twice in the token. I wonder if that's necessary.

2) Should use something besides a colon as colons are part of IPv6 addresses?

[Mark Sapiro (msapiro)]

Thank you for your contribution. It is appreciated and I will merge it.

Regarding @jimpop thoughts, 'now' is used twice because it is part of the data that's hashed in order to make the hash more random, but it also has to be in plain text in the token (which maybe means putting it in the hash doesn't really add anything) because it is required both for the FORM_LIFETIME and SUBSCRIBE_FORM_MIN_TIME checks and to compute the hash to check against. Granted, the time doesn't need to be in the hash, but the original implementation did it like that, and I see no need to change it.

And, the colon is OK as a separator because even though an IPv6 address can begin with a colon, the list name can't end with one, and it doesn't matter anyway. We are only throwing in an extra character to avoid the list name's ending in a digit creating the same hash as a list name without the digit and an IP address with the added digit.

review: Approve

[Ralf Jung (ralfjung-e)]

> 1) The original code in listinfo.py uses 'now' twice in the token. I wonder if that's necessary.

It's not really twice in the (hashed) token, it's one in clear-text and then once more in the hashed token. It needs to be in clear-text as otherwise subscribe.py can't re-compute the hash, and it's in the token to make tokens expire. You don't want an attacker to grab a CSRF token once and then use that same token forever, the timestamp (together with the FORM_LIFETIME check) prevents that.

> it is required both for the FORM_LIFETIME and SUBSCRIBE_FORM_MIN_TIME checks and to compute the hash to check against. Granted, the time doesn't need to be in the hash, but the original implementation did it like that, and I see no need to change it.

If it wasn't in the hash, the MIN_LIFETIME and FORM_LIFETIME would not actually be enforced: An attacker could just always pick a timestamp from 1min ago. Hashing the timestamp prevents the attacker from forging it.

> 2) Should use something besides a colon as colons are part of IPv6 addresses?

The same thought occurred to me this morning. However, assuming the FORM_SECRET does not contain a colon, one can still take the first three colons as separators and everything after the third colon as the IP address. The important property is that it is possible, given the string that's put into the hash, to parse out the field values -- IOW, if any of the field values differ, the hash must be different. (This is assuming list names cannot contain colons, is that correct? Essentially, if only the last field can contain our separator, we are good.)

One easy fix to avoid even having to think about this would be to use ":::" as the separator, which is not valid even in IPv6 addresses.

The "right" way to do this would be to use some kind of serialization (i.e., to pickle the data that goes into the hashed token). Since serialization is guaranteed to be invertible by deserialization, that works without any assumptions about which fields can contain which characters. I can change the patch to do that, if you want.

[Ralf Jung (ralfjung-e)]

> I can change the patch to do that, if you want.

Never mind, I only now saw it is already merged. Thanks!

=== modified file 'Mailman/Cgi/listinfo.py'
--- Mailman/Cgi/listinfo.py	2018-05-26 16:22:35 +0000
+++ Mailman/Cgi/listinfo.py	2018-06-03 20:42:57 +0000
@@ -218,9 +218,9 @@
             remote = remote.rsplit(':', 1)[0]
         replacements['<mm-subscribe-form-start>'] += (
                 '<input type="hidden" name="sub_form_token" value="%s:%s">\n'
-                % (now, Utils.sha_new(mm_cfg.SUBSCRIBE_FORM_SECRET +
-                          now +
-                          mlist.internal_name() +
+                % (now, Utils.sha_new(mm_cfg.SUBSCRIBE_FORM_SECRET + ":" +
+                          now + ":" +
+                          mlist.internal_name() + ":" +
                           remote
                           ).hexdigest()
                     )

=== modified file 'Mailman/Cgi/subscribe.py'
--- Mailman/Cgi/subscribe.py	2018-04-11 09:36:40 +0000
+++ Mailman/Cgi/subscribe.py	2018-06-03 20:42:57 +0000
@@ -173,9 +173,9 @@
         except ValueError:
             ftime = fhash = ''
             then = 0
-        token = Utils.sha_new(mm_cfg.SUBSCRIBE_FORM_SECRET +
-                              ftime +
-                              mlist.internal_name() +
+        token = Utils.sha_new(mm_cfg.SUBSCRIBE_FORM_SECRET + ":" +
+                              ftime + ":" +
+                              mlist.internal_name() + ":" +
                               remote1).hexdigest()
         if ftime and now - then > mm_cfg.FORM_LIFETIME:
             results.append(_('The form is too old.  Please GET it again.'))