User talk:Ilmari Karonen/NoCSRF.js

Good idea. I don't have the time right now to try to use this or to do a thorough review (I'm about to leave for vacation), so here's just a few quick comments after having taken a quick glance at this:

Thanks for the comments. I've interleaved some point-by-point responses below and a longer reply at the end:

• Make it all one object with members: less global namespace pollution.
  • I probably should, yes.
• I don't like the use of strings as status indicators in nocsrf_check_url. If that is called, the caller clearly expects a hash to be present, so not having a "&nocsrf" parameter in the URL is equivalent to having one with the wrong hash. A boolean as return value would suffice (true for success, false otherwise). If you really want to distinguish the case of there not being a "&nocsrf" parameter at all, use the boolean as a three-valued item (null for not present, false for present but wrong hash, true for success).
  • I was mainly concerned with the possibility that callers may want to distinguish expired timestamps from hash failures. That requires returning more than just a boolean. I don't like the idea of null/false/true: yes, it allows for two different "false" return values, but it's not really logical and it won't scale to more than two. (Well, OK, you could have "" and 0 too, and I think "0" as well, but that's just... eww.) In a lower-level language like C or Java, this would call for an enumerated return type. In JavaScript, strings will do just as well.
• Add an expiration to the cookie.
  • I probably should, yes. I originally made it deliberately into a session cookie, but thinking about a bit more the reasons I had for that aren't as good as I thought after all.
• It again has the whole document.cookie in the entropy sources, so it'll have the same problem as the current mechanism used in QuickModify/QuickMod.
  • It won't, since I'm only using it as a source of randomness. It wouldn't matter if it looked completely different on each request — in fact, that would be a good thing.
• In nocsrf_sign_url, variable timestamp is undefined? Why is nocsrf_timestamp global anyway?
  • The unprefixed "timestamp" is a bug — thanks. I made it global to ensure that all the links on the page would have the same timestamp, but it probably won't make much difference either way.
• The noscrf parameter should not be forced to be the last one. Stop the regexp not at $, but at & or # or $.
  • In any URL that nocsrf_sign_url() returns, it will be the last one. I suppose the caller could move it to some other location, but if they modified it in any other way, the MAC check would fail anyway.
• nocsrf_get_key expects only 16 characters from the cookie, but writes 32?
  • Another bug, thanks. I originally had it truncate the hash to only 8 characters, then went to 16 and finally decided not to truncate it at all, but forgot to update the regexp. The original idea behind using a truncated hash was to make sure that the entropy sources (some of which could be privacy-sensitive) would not be recoverable from the hash even by brute force. However, using a truncated hash also makes brute force MAC forgery easier, and I'm not too convinced there's any hash length that would accomplish the former while avoiding the latter. It's all likely to be pretty academic anyway, unless we have a very determined attacker and very little available entropy.
• Is hex_hmac_md5 guaranteed to always return 32 characters? Otherwise, add ".substring(0, 32)".
  • It should be. Certainly it should never return more — MD5 is a 128-bit hash.
• Won't nocsrf_check_url always fail because it uses a different nocsrf_timestamp that was used to sign the url?
  • Ah, yes, another bug. It was of course meant to use the timestamp extracted from the URL, not the current one.

That's all for now. But it's really a good idea. We should polish this. Lupo 06:35, 9 April 2010 (UTC)

Thanks for the review and comments. This does indeed still need some polishing yet. In particular, after thinking about it overnight, I decided that I probably should provide a intermediate-level interface, something like nocsrf_mac(string) which basically just returns hex_hmac_md5( nocsrf_get_key(), string ). That way, users who want to do something other than the one-size-fits-all approach of nocsrf_sign_url() / nocsrf_check_url() can still take advantage of the key management and the HMAC wrapper.

Also, one advantage to storing the cookie for a longer period would be that we could spend more effort on entropy gathering. My first idea was to collect mouse events, but there's no real guarantee that we'd manage to collect any entropy that way in the (possibly very short) time before we're called to sign something (and stopping everything and asking the user to wave the mouse around is not very user-friendly, even if it is an excellent way to collect entropy). On the other hand, we could just make a MediaWiki API request to the server and ask it to give us some entropy — in particular, we could request an edit token and use that as one entropy source, making our tokens at least as random as those MediaWiki uses internally. —Ilmari Karonen (talk) 22:45, 9 April 2010 (UTC)

OK, I've made some of the changes discussed above. I also ran a few tests, and it seems to be basically working. I've still got some things on my "to do" list, but these are basically all internal tweaks and optimizations:

• Better entropy sources (API request / mouse events / something else?)
• Set explicit expiration time for the cookie.
• Decide if I really want to cache the base timestamp or not... I'm thinking maybe not.
• Call the lower-level MD5.js functions directly instead of going through hex_hmac_md5(); this could save a few cycles, since we know that e.g. the key is always 7-bit ASCII.

I think the API feels more or less OK now, though. I think I'll move this to MediaWiki:NoCSRF.js soon and call it ready for use. —Ilmari Karonen (talk) 16:24, 10 April 2010 (UTC)