Comments

alexey.r created an issue. See original summary.

alexey.r’s picture

Assigned: alexey.r » Unassigned
tombsage’s picture

Hi alexey.r

I was having the same problem. Please try clearing all Drupal cache and also your browser cache and try again. This seems to have worked for me. Let me know if this also fixes the problem for you.

This seems to be happening when validating the token created for the CAPTCHA, line 229 of clientside_validation.module. The token is generated on line 1336 of clientside_validation.module.

Tom

alexey.r’s picture

Unfortunately after both cache cleaning the error still the same.

tombsage’s picture

I have just done the same thing on a different set of servers and I am still getting this problem.

line 1338 of clientside_validation.module - add the following:
watchdog('CAPTCHA-KEYS-1', json_encode(array(session_id(), drupal_get_private_key(), drupal_get_hash_salt())));

line 228 of clientside_validation.module - add the following:
watchdog('CAPTCHA-KEYS-2', json_encode(array(session_id(), drupal_get_private_key(), drupal_get_hash_salt())));

Clear cache and run through the captcha again. Then go to /admin/reports/dblog - if the session IDs are different then that is causing the problem.

Are you doing the captcha as an anonymous user?

joel_osc’s picture

Seeing this problem too... seems to work after second try in FF using private browsing window. But does not work in FF or Chrome in standard browsing window. All testing done as an anonymous drupal user.

tombsage’s picture

Hi joel_osc,

Have you tried the method above to check if the session id's are different?

The drupal_valid_token() function does take a third parameter ($skip_anonymous: Set to true to skip token validation for anonymous users). This will make the captcha work but skips the token validation for anonymous users so I do not consider this a fix.

line 229 of clientside_validation.module:

- if (!drupal_valid_token($token, $captcha_validate)) {
+ if (!drupal_valid_token($token, $captcha_validate, TRUE)) {

What are your server setups? (e.g. load-balanced, single server?)

joel_osc’s picture

I put watchdog's on the lines above, but found that the second watchdog (CAPTCHA-KEYS-2) was only hit when the captcha was successful so I didn't have much to compare. I have it happening on a production site (simple single server) and in a lab environment server (even simpler) as well.

tombsage’s picture

The second watchdog should be in the following position:

$token = $_POST['param'][2];
watchdog('CAPTCHA-KEYS-2', json_encode(array(session_id(), drupal_get_private_key(), drupal_get_hash_salt())));
if (!drupal_valid_token($token, $captcha_validate)) {
    return drupal_access_denied();
}

If you have it there, it should get hit even if the captcha is unsuccessful. (it's the "return drupal_access_denied();" line which returns the 403)

joel_osc’s picture

Hi,

Thank-you for the outstanding help on this! Indeed you are correct the session ids are different in the two sections of code. The private key and hash salt are the same. I am not sure how the session ids could be different when all I am doing is loading the page (CAPTCHA-KEYS-1) and then answering the captcha (CAPTCHA-KEYS-2).

php 2017-09-13 Warning: Cannot modify header information - headers...Anonymous (not verified)
access denied 2017-09-13 clientside_validation/captchaAnonymous (not verified)
CAPTCHA-KEYS-2 2017-09-13 ["95jd4GcwQQbYm30eGCXvaKF36hny_yk9YKzmGC2i39Y",...Anonymous (not verified)
CAPTCHA-KEYS-1 2017-09-13 ["XQs2z_4k2J7ATwgFJ9aljLxtZ-ynqCqVF6a8ygMeWaE",...Anonymous (not verified)
tombsage’s picture

Hi,

I'm also not sure why the session is being re-generated. Do you have any info on the "Warning: Cannot modify header information - headers...Anonymous (not verified)" error? Perhaps it is related.

drupalevangelist’s picture

I encountered the similar issue today. I used the instructions from #5 to compare the session IDs and they are different. How should I go about resolving this issue? Any suggestions?

tombsage’s picture

Hi emdadnrn,

You could turn off token validation for anon users (see #7) - again I'm not considering this an actual fix but it can get you up-and-running for now.

drupalevangelist’s picture

Hi tombsage,

Thank you.

joel_osc’s picture

In looking at this further I have added a bunch of watchdogs in the session.inc code and found that a session is never really created for anonymous users. And then looking at the API docs I found the following comment regarding drupal_get_token (https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ge...)

The generated token is based on the session ID of the current user. Normally, anonymous users do not have a session, so the generated token will be different on every page request. To generate a token for users without a session, manually start a session prior to calling this function.

I not an expert in the area, so I would like to throw this out there for some thoughts...

nehasinghaniya21’s picture

I am also having similar issue, where forms using clientside validation module showing error of incorrect captcha answer even though answer was correct.

I made changes as suggested by #7 (tombsage) and it works fine.

Thanks.

aaron.ferris’s picture

Will skipping the token validation for anonymous users reintroduce https://www.drupal.org/node/2907118?

alexey.r’s picture

#7 receipt solved the problem.

Thank you.

tombsage’s picture

Hi Aaron,

Skipping token validation for anon users does reintroduce this issue.

http://cgit.drupalcode.org/clientside_validation/commit/?id=b5b3ea2
https://www.drupal.org/node/2907118

#7 - "This will make the captcha work but skips the token validation for anonymous users so I do not consider this a fix."

joel_osc, you seem to on the right lines with manually starting a session. Perhaps Jelle_S could help with this?

aaron.ferris’s picture

Hi @tombsage,

Thought as much.

I think the fix for this is to follow the guideline from drupal_get_token() and create a persistent session at the point of this token being created in _clientside_validation_set_captcha();

I'm working on a patch, ill attach it here when i've completed some testing.

Thanks
Aaron

tombsage’s picture

Hi Aaron,

I've tried adding drupal_session_start(); to the start of the _clientside_validation_set_captcha() function. I am now getting matching session ids but the token validation is still failing.

Example:

_clientside_validation_set_captcha example vars ($js_rules[$name]['captcha']):

validate = 'captcha_validate_case_insensitive_ignore_spaces'
token = 'BkOwzzihjgS1mTkjoVPtZyznFztXSkjyc-zc8cameZg'

_clientside_validation_ajax_captcha() vars ($captcha_validate, $token)

$captcha_validate = 'captcha_validate_case_insensitive_ignore_spaces'
$token = 'BkOwzzihjgS1mTkjoVPtZyznFztXSkjyc-zc8cameZg'

Am I missing something? How is your testing going?

aaron.ferris’s picture

Hi @tombsage

Testing on my patch seems to mitigate the 403 in my implementation, I'm just completing some further testing and ill attach it, if you could please apply it and see how you get on with it, that would be appreciated.

I'm just a little concerned over any potential performance impact of creating a session for anonymous users.

Thanks
Aaron

aaron.ferris’s picture

Here is the patch I'm working on.... as above, definitely concerns over any performance impact this could have.

TODO: If using this approach, we need to consider that the session would be destroyed as part of the ajax callback, but what if the user doesn't complete the form? We might need to look at similar destructive code elsewhere, perhaps hook_init() when the page request isn't 'clientside_validation/captcha'... needs some thought.

Edit: Bah, incorrect patch name :(

Thanks
Aaron

tombsage’s picture

Hi Aaron,

I've applied your patch and I'm still getting a 403 from /clientside_validation/captcha. Does the session need to be started before we call drupal_get_token() in _clientside_validation_set_captcha()?

This would also mean if an anonymous user hits a page with a captcha but does not attempt it, their session will not be destroyed. This may impact caching back-ends such as Varnish for some setups.

EDIT: sorry didn't see the TODO

aaron.ferris’s picture

Hi @tombsage

Seems to be fine for me in its current position, you could try moving it above to see if that helps?

My testing steps are:
1. Clear Drupal caches
2. Clear browser caches
3. Attempt to submit my Captcha enabled form

If I comment out drupal_session_start(); I start seeing the 403 again, so at least for me the fix seems to resolve my issue, quite why it's not working for you is strange though.

Out of interest, which version of the Captcha module are you using? I wonder if that could have an impact....

Thanks
Aaron

joel_osc’s picture

Just as an FYI, module like flag will use this module https://www.drupal.org/project/session_api to deal with anonymous users.

tombsage’s picture

Hi Aaron,

I did have differing results when I first updated the module - worked fine (no patches) on dev server... 403 on live server. I have tried multiple browsers & cache clears and still get a 403 (using the patch).

I'm using captcha module 7.x-1.5.

Also _clientside_validation_ajax_captcha() could be called multiple times from AJAX requests per captcha. Do you actually want to destroy the session within this function?

aaron.ferris’s picture

Hi @tombsage

Possibly not, the session destroy could come from elsewhere as noted in my 'TODO'. In fact if we're going to do that anyway, we could remove the destroy from _clientside_validation_ajax_captcha(). My patch was more to start a discussion on a possible fix, it's definitely not ready for use.

I'm not sure why we're seeing such differing results, perhaps I have something else in my codebase supporting this, hard to see what it could be mind. If the patch doesn't resolve your issue then it's clearly not fit for purpose for all use cases, hard for me to expand on this if I can't recreate it though :(.

Edit - Looking back at this thread, it seems your token is now matching during _clientside_validation_ajax_captcha()? Feels very strange why drupal_valid_token() would return false if the data matches.... is it possible the 403 is being caused elsewhere? My issue was that during _clientside_validation_ajax_captcha() the $token values were different which of course would cause a 403. The session_start mitigates this in my implementation.

@joel_osc could you please apply the patch attached and test? Or anyone else from this thread.

Perhaps a different approach to mitigating https://www.drupal.org/node/2907118 is needed, to move away from drupal_get_token if possible.

Edit - i'm also using Captcha 7.x-1.5

Thanks
Aaron

tombsage’s picture

Hi Aaron,

I'm using image_captcha & image_captcha_refresh on webforms. Are you using a different challenge type?

After a bit more testing, it looks like the session being destroyed in _clientside_validation_ajax_captcha() is causing the 403. It seems to be calling _clientside_validation_ajax_captcha() multiple times while entering the captcha, destroying the session in _clientside_validation_ajax_captcha() is then making the sid change invalidating the token.

If we use hook_init to destroy the session, we will need to not destroy the session if the page being loaded contains a form with a captcha or is /clientside_validation/captcha - not sure how we would achieve this. Any ideas?

aaron.ferris’s picture

Hi @tombsage

Indeed if this is firing multiple times then the subsequent calls to _clientside_validation_ajax_captcha() will have a fresh session ID reintroducing the 403, that makes sense. For reference i'm using a hidden captcha field from hidden_captcha on this particular form.

I've been playing with a method of destroying the session outside of this callback, and not getting very far. hook_init() isn't called on cached pages so I don't think we can go down this route. I've been attempting to do this in hook_boot() but I can't seem to tie down the correct code to ensure it isn't fired as part of 'clientside_validation/captcha'.....

My theory was to have something like:

/**
 * Implements hook_boot().
 */
function clientside_validation_boot() {
  if (!empty($_SESSION) && !empty($_SESSION['clientside_validation_captcha_set'])) {
    if (count($_SESSION) === 1 && strpos($_SERVER['REQUEST_URI'], 'clientside_validation/captcha') !== TRUE) {
      // We create a session to ensure drupal_valid_token() can validate against
      // the session_id as part of _clientside_validation_ajax_captcha(), so
      // destroy it here.
      $sid = session_id();
      _drupal_session_destroy($sid);
    }
  }
}

But this seems to fire on every page call, including the AJAX callback, destroying the session and giving us the 403 again. I think we're on the right track with this, it's just a case of destroying the session in a reliable and correct manner.

I might look into destroying the session as part of a pre_process. Any ideas welcome on where else we could do this!

Thanks
Aaron

joel_osc’s picture

Hi Aaron,

The patch fixes my site.

Cheers!

jelle_s’s picture

Thank you all for looking in to this. I currently don't have much time to spend on this. But I've been looking at the code in the captcha module. Captcha creates a sort of 'session' as well, and stores it in the database (the captcha_sessions table), where it also stores a token. If we could somehow create a token of the callback function and the captcha session id or token, in stead of using drupal_get_token, we could use that as well for preventing the arbitrary code execution mentioned in https://www.drupal.org/node/2907118

For those looking for a fix in the meantime, you can disable captcha validation (on the client side) in the config form of clientside validation. The captcha will still be validated on form submit by the captcha module, so no worries there.

aaron.ferris’s picture

Thanks @joel_osc

Thanks @Jelle_s that makes sense and is definitely something I will try and look into to avoid drupal_get_token, also thanks for the heads up around disabling clientside_validation for Captcha as that's an option for the moment.

Aaron

aaron.ferris’s picture

Hi @Jelle_s, @joel_osc, @tombsage,

Would something like the attached work (please ignore this patch and see the below comment)? It's essentially using the same approach as drupal_get_token() but substituting the session_id() for the captcha_token. This approach would pass the captcha_token through as a POST parameter as well, so I guess could also be open to manipulation but my thinking is for someone to manipulate this they'd need to know how we were generating the token in the first place? Feedback welcome.

I have tested this in my implementation and the 403 doesn't exist anymore, naturally my concern is around how robust this solution is to also avoid https://www.drupal.org/node/2907118.

I'll continue testing various scenarios, it would be good for others to please test this out if possible.

Edit: As a side note, it appears the documentation on https://www.drupal.org/node/2907118 is incorrect?

Versions affected
Clientside Validation 7.x-1.x versions prior to 7.x-1.44.

This seems to suggest the issue doesn't exist on 1.44, although the recommended solution is to install 1.45 this could still confuse some people, ill fire the security team a contact form.

Thanks
Aaron

aaron.ferris’s picture

Ignore that last patch, seemed I had a missing variable...

tombsage’s picture

Hi Aaron,

Updated patch, would not apply as paths contained your full dev path. I will test this shortly.

aaron.ferris’s picture

Thanks @tombsage, I had just realised this myself, so was coming on to attach a new patch!

Thanks
Aaron

aaron.ferris’s picture

Coding standards fix.

tombsage’s picture

Hi Aaron

I forgot to mention the previous patch had the same problem. I have tested the latest patch and it seems to be working for me.

I believe an attacker would need to know the private key and salt to perform a successful attack, correct me if i'm wrong.

Hopefully a few more people can test the patch to cover all use cases.

Cheers
Tom

aaron.ferris’s picture

Hi Tom,

That's my understanding as well, they'd need to know our method of creating the token as part of clientside_validation_generate_token() (including private key and salt) which is highly unlikely (perhaps even impossible?). Probably best to await @Jelle_s' input to be sure.

Thanks for the feedback.

Thanks
Aaron

jelle_s’s picture

Overall the patch looks good to me. I'm just wondering if passing the captcha token to the frontend has any security implications (I would imagine not)... But if it has, we could pass the captcha session id and then get the captcha token from the database using that session id. Ideally we could have a maintainer of the captcha module chime in here, but I'm willing to commit the patch as-is as I don't really see a security issue here (I am not a security expert, but I don't think anyone can establish any sort of valuable information from that token).

I'll leave this issue open for a couple of days before I commit the patch, so that people have the chance to test this fix or to chime in.

tombsage’s picture

Hi @Jelle_S

I tested using the captcha session id earlier and i'm not sure it would work as you could end up with multiple tokens per session id.

You could base it on the latest captcha token based on the session id but you could potentially have someone open multiple browser tabs with captcha's.

jelle_s’s picture

Hm that makes sense. As said, I'll leave it open for a couple of days to make sure, and to give people the chance to test. Feel free to remind me if I haven't committed this by monday.

tombsage’s picture

Status: Active » Needs review
aaron.ferris’s picture

I'm not sure exposing the Captcha token in the front end is a concern, if it is this is already happening by way of a hidden input field with the token value.

$element['captcha_token'] = array(
    '#type' => 'hidden',
    '#value' => $captcha_token,
  );

  • Jelle_S committed 952b9b7 on 7.x-1.x authored by aaron.ferris
    Issue #2907951 by aaron.ferris, tombsage: Captcha field validating error
    
jelle_s’s picture

Version: 7.x-1.45 » 7.x-2.x-dev

Fixed in 7.x-1.46

drupalevangelist’s picture

Thank you. Installed the latest version 7.x-1.46 and the issue is permanently resolved.

aaron.ferris’s picture

Thanks for the feedback @emdadnrn and @Jelle_S for the fast commit and new version release.

Guess this should be listed as fixed.

Thanks
Aaron

aaron.ferris’s picture

Status: Needs review » Fixed
jelle_s’s picture

Status: Fixed » Patch (to be ported)

@aaron.ferris It's not yet fixed in 2.x :)

aaron.ferris’s picture

Sure, thanks @Jelle_S.

I'll look at porting this when I get 5 minutes.

aaron.ferris’s picture

Hi all,

Here is an initial patch for the 7.x-2.x branch, a disclaimer though I couldn't test this with a functioning form as i'm seeing errors related to jQuery.validate in my development environment, meaning I can't reproduce this issue (naturally because the JS is falling over). I'd appreciate it if someone who has a working version of the 2.x module, and can reproduce the issue to please test this.

The minimal testing i've done is via using the clientside_validation/captcha URL.

Thanks
Aaron

aaron.ferris’s picture

StatusFileSize
new3.74 KB

Ignore the patch above, it had some unwanted changes related to me trying to fix my local issues....

aaron.ferris’s picture

Status: Patch (to be ported) » Needs review