The following line in captcha.module:

      variable_set('captcha_wrong_response_counter', variable_get('captcha_wrong_response_counter', 0) + 1);

- is very expensive for site performance (call to variable_set() clears variable cache) and it opens doors to simple DOS attacks (bad guys can just keep anonymously submitting forms with wrong captchas).

Files: 
CommentFileSizeAuthor
#10 captcha-disable-captcha_wrong_response_counter-1002246-10.patch2.66 KBpbull
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]
#7 captcha-disable-captcha_wrong_response_counter-1002246-7.patch2.72 KBpbull
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]

Comments

soxofaan’s picture

Title:Cache clear and performance of variable_set('captcha_wrong_response_counter',...)» variable_set('captcha_wrong_response_counter',...) is expensive performance wise (clears cache)

Good point.

But what is your feature request:
remove the counting of wrong responses or making it optional?

iva2k’s picture

I would mark it as a bug, but nothing is broken, so I chose "feature request".
I think the module will benefit from a different accounting method, so it really is an "improvement request"

iva2k’s picture

(duplicate reply removed)

soxofaan’s picture

Maybe, we can combine this with feature request #998326: Separate log of wrong and empty responses (especially see http://drupal.org/node/998326#comment-3827828)

iva2k’s picture

Would be great if it is solved there. Unfortunately the patch in #998326: Separate log of wrong and empty responses does not change variable_set().

It is actually a design challenge to do it right. Drupal variable_*() API is not intended for often-changed values such as the counter in question. Own table is probably a way to go. I also thought of a separate module that would provide counter API to modules like CAPTCHA. "fastvars.module" anyone?

soxofaan’s picture

Yep, the patch even doesn't work yet (it breaks tests). But the counter could be moved to a separate "CAPTCHA logging and counting" module as suggested in http://drupal.org/node/998326#comment-3827828 :

  • CAPTCHA core module: exposes counting/logging with a hook, but doesn't do it itself.
  • "CAPTCHA logging and counting module": implements the counting with one of the following (for example)
    • variable_set: still not optimal, but at least the module is not required for CAPTCHA to work and can be disabled
    • a custom variation of variable_set that doesn't flush cache: a bit ugly/hackish, but it doesn't require to create/manage/maintain an extra table.
    • with its own table: seems a bit overkill for a single counter variable
    • by depending on another module (fastvars or something): also seems a bit overkill to depend on another module for storing a simple variable, unless the module is widely used and popular (genre CCK/views)

just thinking out loud here

pbull’s picture

Version:6.x-2.x-dev» 7.x-1.x-dev
StatusFileSize
new2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]

It's a serious concern that incorrect CAPTCHAs could be used as an attack vector. Attaching a patch against the 7.x-1.x branch that does the following:

* disables the updating of captcha_wrong_response_counter by default
* provides an admin option to enable the counter
* hides the counter from the Status report page if it's disabled, as it's not a meaningful data point if it's not being incremented

pbull’s picture

Status:Active» Needs review
soxofaan’s picture

Status:Needs review» Needs work

great, thanks

maybe use a more general variable 'captcha_enable_stats' instead of 'captcha_count_wrong_responses'. That way we can use it to guard more counters in the future that would require variable_set (e.g. total number of generated CAPTCHA's, number of correctly solved ones, ...)

pbull’s picture

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]

@soxofaan: Good idea, the 'captcha_count_wrong_responses' variable seemed a bit clunky to me. Attaching an updated patch to use 'captcha_enable_stats' and 'Enable statistics' on the admin side.

soxofaan’s picture

Status:Needs review» Fixed

tweaked the admin description a bit and committed

http://drupalcode.org/project/captcha.git/commit/ad165d1

Status:Fixed» Closed (fixed)
Issue tags:-cache, -variable_set

Automatically closed -- issue fixed for 2 weeks with no activity.