Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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).
Comment | File | Size | Author |
---|---|---|---|
#10 | captcha-disable-captcha_wrong_response_counter-1002246-10.patch | 2.66 KB | pbull |
#7 | captcha-disable-captcha_wrong_response_counter-1002246-7.patch | 2.72 KB | pbull |
Comments
Comment #1
soxofaan CreditAttribution: soxofaan commentedGood point.
But what is your feature request:
remove the counting of wrong responses or making it optional?
Comment #2
iva2k CreditAttribution: iva2k commentedI 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"
Comment #3
iva2k CreditAttribution: iva2k commented(duplicate reply removed)
Comment #4
soxofaan CreditAttribution: soxofaan commentedMaybe, we can combine this with feature request #998326: Separate log of wrong and empty responses (especially see http://drupal.org/node/998326#comment-3827828)
Comment #5
iva2k CreditAttribution: iva2k commentedWould 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?
Comment #6
soxofaan CreditAttribution: soxofaan commentedYep, 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 :
just thinking out loud here
Comment #7
pbull CreditAttribution: pbull commentedIt'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
Comment #8
pbull CreditAttribution: pbull commentedComment #9
soxofaan CreditAttribution: soxofaan commentedgreat, 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, ...)
Comment #10
pbull CreditAttribution: pbull commented@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.
Comment #11
soxofaan CreditAttribution: soxofaan commentedtweaked the admin description a bit and committed
http://drupalcode.org/project/captcha.git/commit/ad165d1