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

CommentFileSizeAuthor
#10 captcha-disable-captcha_wrong_response_counter-1002246-10.patch2.66 KBpbull
#7 captcha-disable-captcha_wrong_response_counter-1002246-7.patch2.72 KBpbull

Comments

soxofaan’s picture

soxofaan commented
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

iva2k commented

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

iva2k commented

(duplicate reply removed)

soxofaan’s picture

soxofaan commented

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

iva2k commented

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

soxofaan commented

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

pbull commented
Version: 6.x-2.x-dev » 7.x-1.x-dev
StatusFileSize
new captcha-disable-captcha_wrong_response_counter-1002246-7.patch2.72 KB

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

pbull commented
Status: Active » Needs review
soxofaan’s picture

soxofaan commented
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

pbull commented
Status: Needs work » Needs review
StatusFileSize
new captcha-disable-captcha_wrong_response_counter-1002246-10.patch2.66 KB

@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

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

Issue tags: +cache, +variable_set

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.