Any forms with the machine names wrapping the 'system_', 'search_', 'views_exposed_form_' wording will not be protected because the code considers them as system forms. But if we have form machine names like 'subsystem_', 'research_', 'xxxviews_exposed_form_' which are not really system forms, the code will not alter them according to:

if (strpos($form_id, 'system_') === FALSE && strpos($form_id, 'search_') === FALSE && strpos($form_id, 'views_exposed_form_') === FALSE) {}

in line 81 honeypot.module. This is causing security issue for us as we have 'research_' in our form machine name unfortunately. We need to rule out this case.

CommentFileSizeAuthor
#8 form_not_protected_if_machinename_wrapping_specific_word-2912575-8.patch872 bytesgeerlingguy
#3 form_not_protected_if_machinename_wrapping_specific_word-2912575-3.patch886 bytesrli
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rli created an issue. See original summary.

rli’s picture

rli
Primary language English
Location Canberra
CreditAttribution: rli commented
Issue summary: View changes
rli’s picture

rli
Primary language English
Location Canberra
CreditAttribution: rli as a volunteer commented
Status: Active » Needs review
FileSize
form_not_protected_if_machinename_wrapping_specific_word-2912575-3.patch886 bytes

Changed strpos to preg_match to include [a-zA-Z]search_ for example.

tobybellwood’s picture

tobybellwood CreditAttribution: tobybellwood at govCMS (Australian Government Department of Finance) for govCMS (Australian Government Department of Finance) commented
Status: Needs review » Reviewed & tested by the community

We've reviewed, tested and deployed this at https://github.com/govCMS/govCMS/pull/581

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Priority: Major » Normal

This would also apply to forms with names like `solarsystem_` :D

Thanks for the patch, looks good! I'll also port it to 8.x.

  • geerlingguy committed d38be2d on 7.x-1.x authored by rli 
    Issue #2912575 by rli: From with 'research_' in machine name is not...
geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Status: Patch (to be ported) » Needs review
FileSize
form_not_protected_if_machinename_wrapping_specific_word-2912575-8.patch872 bytes

  • geerlingguy committed 336bafa on 8.x-1.x 
    Issue #2912575 by rli, geerlingguy: From with 'research_' in machine...
geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

ciss’s picture

ciss CreditAttribution: ciss at yousign GmbH commented 
+++ b/honeypot.module
@@ -64,7 +64,7 @@ function honeypot_form_alter(&$form, FormStateInterface $form_state, $form_id) {
+    if (preg_match('/[^a-zA-Z]system_/', $form_id) === 0 && preg_match('/[^a-zA-Z]search_/', $form_id) === 0 && preg_match('/[^a-zA-Z]views_exposed_form_/', $form_id) === 0) {

Late to the party, but wouldn't it be more efficient to group the terms inside a single pattern:

if (preg_match('/[^a-zA-Z](?:system_|search_|views_exposed_form_)/', $form_id) === 0) {

Also, which cases are covered by "[^a-zA-Z]"? Shouldn't we explicitely match either the beginning of the string or an underscore?

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented

@ciss - Since this issue is closed, an optimization to the regex would be good to add to a follow-up feature request issue.

ciss’s picture

ciss CreditAttribution: ciss at yousign GmbH commented

@geerlingguy I created the follow-up #3085291: Lock down $form_id matching. I'd be grateful if you could take a quick look and confirm the overall premise before I start working on a patch.