Disabled cookies == no session data, impossible to submit a protected form

Subscribing.

The $_SESSION stuff was introduced in #562374: Mollom incompatible with Pressflow's (and Drupal 7's) session handling -- causes CAPTCHAs to fail.

I'm not sure I understand why it's necessary. The comments in that issue and in the code mention that this is a protection against replay attacks, but AFAICT the cache entry is read/written/deleted (using cache_get()/cache_set()/cache_clear_all() at the same time that the $_SESSION entry is read/written/deleted. Doesn't the cache already offer the same protection, i.e. can't all the $_SESSION related code simply be removed?

Like this.

I talked to http://drupal.org/user/350460 at DrupalVolConBE about this. He's the maintainer of the Wordpress Mollom plugin. He is not using sessions. He simply allows endless retries. I'm not sure why we shouldn't do the same.

As far as I understand from a quick code skim, Mollom's servers also maintain a session. This probably yields better results. If we want to support Mollom's servers' own sessions, then we *must* use Drupal/PHP sessions as well.
(I started rerolling the patch to make the tests pass but stopped when I realized this.)

If we want to support Mollom's servers' own sessions, then we *must* use Drupal/PHP sessions as well.

I'm not sure what you mean. Mollom only uses $_SESSION to see if the session id exists in the current user session - and as mentioned it looks like this check is dublicated using the cache. The value in $_SESSION is never sent to the Mollom servers.

However, that still leaves us with the security issue of being able to re-use/replay a form containing a Mollom session id (until it is successfully submitted).

Whenever a comment is submitted to the site, the form has been successfully submitted, and the cache entry has been deleted. So you can only submit one comment per successfully submitted CAPTCHA, right?

As mentioned in #2, $_SESSION is updated at the same time as the cache, so the use of $_SESSION does not add any protection against replay attacks that are not already provided by the cache—right?

I agree that we should delete the cache entry when a new session id is returned by Mollom.

This flushes the old session from the cache when a new session ID is received from Mollom.

This looks like a nice clean-up to me, but it would be good to have sun review it too. :)

Good point, sun.

The advantage of the current approach is that it detects mis-configured proxy/caching servers -- it detects when a session from one user is re-used by another user. At the same time, it also freaks out when cookies are disabled.

Why should we prevent re-use?

If a session ID is used to submit ham, it will have a good reputation in the Mollom backend. If that session is reused to submit spam (e.g. because it was incorrectly cached), then the spam is likely to get through. By binding the Mollom session ID to the user session ID, we avoid re-use. Long story short, a session should only be used by one user and should never be reused.

With the new approach, we delete the session from the mollom_cache after we submitted the form, so that should prevent some re-use but probably not all. I think it could be OK, but it is somewhat difficult to judge. I lean towards being OK with it -- especially if we'd be able to detect mis-configured caching proxies in another fashion.

subscribe

If a session ID is used to submit ham, it will have a good reputation in the Mollom backend. If that session is reused to submit spam (e.g. because it was incorrectly cached),

I'm not sure how this can happen. Can you elaborate on how such attack could happen?

The session id only appears in the hidden HTML field after the user has submitted the form, i.e. in response to a POST request, right? POST requests are unlikely to be cached, so I cannot see how the spammer can sniff the other user's session id. If there is an XSS hole on the site, the spammer can sniff the user's Drupal session cookie as well as the Mollom session id, so in that case the $_SESSION approach doesn't offer additional security.

Even if a with a session id in the hidden field form was cached incorrectly, the call to cache_get($mollom_session_id, 'cache_mollom') in mollom_process_mollom_session_id() checks that we cannot re-use a session id that has already been used in a successful form submission.

I looked at this some more and I think c960657 is right. I committed this patch to DRUPAL-6--1.

Can we get a port to Drupal 7, please? We try to keep both versions in sync. Given that this means deleting code, it should be fun to port. ;)

Thanks c960657.

We're no longer using cache_mollom in D7, so only the $_SESSION removal hunks applied.

As c960657 knows, the session handling in Drupal 7 and Pressflow is different from the one in Drupal 6. Page with a CAPTCHA can't be cached as the session ID changes. The $_SESSION stuff was introduced because of the changes in session handling, and because Mollom stopped working with Varnish. By setting session data, we create a cookie, which will prevent the page from being cached.

The session id only appears in the hidden HTML field after the user has submitted the form, i.e. in response to a POST request, right? POST requests are unlikely to be cached, so I cannot see how the spammer can sniff the other user's session id. If there is an XSS hole on the site, the spammer can sniff the user's Drupal session cookie as well as the Mollom session id, so in that case the $_SESSION approach doesn't offer additional security.

Not for CAPTCHAs in CAPTCHA only mode. For example, on the registration form, CAPTCHAs will appear on the page before the form is submitted. In other words, we can't cache pages with CAPTCHAs -- either in Drupal's page cache or using a reverse proxy like Varnish.

Thanks for the clarification about CAPTCHA-only mode.

Are you saying that there is a problem with the D6 or D7 patch? The following is executed when the CAPTCHA is inserted. AFAICT this prevents caching of pages with CAPTCHAs without starting a session, at least in D6 and D7 (don't know about Pressflow).

    // Prevent the page cache from storing a form containing a CAPTCHA element.
    $GLOBALS['conf']['cache'] = 0;

The D7 version should probably use drupal_page_is_cacheable(FALSE) instead, though.

Looking at the code, it looks like $GLOBALS['conf']['cache'] = 0; might not actually work. It doesn't avoid the page from being stored in the page cache, as far as I can tell. drupal_page_is_cacheable() does not consult $GLOBALS['conf']['cache'] = 0;.

(Update: it looks like it might work. We do: if (variable_get('cache', 0) && ($cache = drupal_page_set_cache())) {. Any idea of why we don't put that check in drupal_page_is_cacheable() instead?)

Furthermore, preventing the page from being stored in the page cache is good, but we should also prevent Varnish from caching the page. It looks like we just call ob_flush() instead of omitting the right headers. Need to look more at the surrounding code though.

  if (variable_get('cache', 0) && ($cache = drupal_page_set_cache())) {
    drupal_serve_page_from_cache($cache);
  }
  else {
    ob_flush();
  }

Any idea of why we don't put that check in drupal_page_is_cacheable() instead?

Not really. In _drupal_bootstrap_page_cache() we may call page_is_cacheable() before variables have been initialized (if $conf['page_cache_without_database'] has been set), but that is a recent addition that wasn't there when page_is_cacheable() was introduced in #201122: Drupal should support disabling anonymous sessions. I guess we could/should move the check into page_is_cacheable(), though we'd have to be careful about $conf['page_cache_without_database'].

It looks like we just call ob_flush() instead of omitting the right headers.

By default Drupal sends anti-caching headers in all requests in drupal_page_header(). These are overwritten by headers that allow caching in drupal_serve_page_from_cache(). If $conf['cache'] is 0, we don't hit that function, so the anti-cache headers are still sent, so Varnish doesn't cache (I think - I haven't tested it).

I reviewed the code once more and decided to commit this to CVS HEAD. Let's see how it works -- thanks c960657 et al.

Committed #25 to D6. Needs fixing in D7 too?

Grabbed the -dev version trying to solve a problem with Mollom on PressFlow and it seems like this is the patch that fixed it. It'd be great to get a release out that incorporates it.

Agreed that we should try to roll another -dev release. sun and myself can coordinate that next week. Thanks!

Tracking the new release at #863486: New releases.