GNU Mailman

Code review comment for lp:~ralfjung-e/mailman/csrf-injective

csrf-injective
Merge into 2.1

Revision history for this message

Ralf Jung (ralfjung-e) wrote on 2018-06-04:

> 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.

> 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.

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.

« Back to merge proposal