Problem/Motivation

The contact form allows users to change the name and mail fields. This can be considered a security issue in that recipients of the message may be tricked into communicating with an impostor.

Proposed resolution

Remove the ability for logged in users to change name and email address on the contact form.

Remaining tasks

  • Minor cleanups.
  • Manually test the email sent for both the sitewide and personal contact forms for both anonymous and authenticated users.

User interface changes

Only anonymous users will be allowed to enter a name and email address on the contact form. Users with an account will be forced to use the settings from their user data. Text will alert recipients of the sent emails that the anonymous users are not verified.

Before patch

After patch

String addition

The string t('Unverified') is added to contact form emails sent by anonymous users.

API changes

The structures of contact_site_form() and contact_personal_formm() change for authenticated users (replacing text fields with read-only values).

Original report by Dave Reid

Currently, both the site-wide and personal contact forms allow registered users to change the values of the 'name' and 'mail' fields. So I could submit a user's personal contact form with the name 'Dries' and 'dries-not-valid-mail@drupal.org' and it would look to the user like this e-mail actually came from Dries, which is a bad thing.

These forms should both remove the fields when a non-anonymous user is using them, and instead use theme_username() to display that the e-mail will be 'sent' from the current user.

CommentFileSizeAuthor
#155 601776-contact-core-134.patch5.3 KBstefan.r
#135 601776-contact-core-134.patch5.3 KBdcam
#131 before.PNG3.74 KBdcam
#131 after.PNG4.33 KBdcam
#130 601776-contact-core-130.patch1.32 KBDavid_Rothstein
#127 before.PNG4.92 KBdcam
#127 after.PNG4.09 KBdcam
#125 601776-contact-core-125.patch5.25 KBACF
#121 601776-contact-core-121.patch3.41 KBDavid_Rothstein
#118 601776-contact-core-118.patch5.11 KBACF
#110 601776-contact-core-110.patch4.78 KBACF
#108 601776-106-drupal7-tests.patch1.89 KBZenDoodles
#108 601776-106-drupal7-combined.patch5.2 KBZenDoodles
#106 601776-D7-use_profile_contact-105-tests.patch744 bytesZenDoodles
#106 601776-D7-use_profile_contact-105.patch4.03 KBZenDoodles
#98 drupal-601776-98-tests.patch2.34 KBtim.plunkett
#98 drupal-601776-98-combined.patch5.67 KBtim.plunkett
#97 contact_forms_security-601776-97.patch5.67 KBjfhovinne
#97 interdiff.txt688 bytesjfhovinne
#96 drupal-contact_forms_security-601776-96.patch5.35 KBdisasm
#96 interdiff.txt3.33 KBdisasm
#91 contact_forms_security_601776-90.patch5.36 KBkwattro
#88 contact_forms_security_601776-88.patch36.67 KBkwattro
#86 contact_forms_security_601776-86.patch5.39 KBkwattro
#82 contact_forms_security_601776-82.patch5.39 KBkwattro
#82 interdiff-77-82.txt2.38 KBkwattro
#77 interdiff.txt3.06 KBkbasarab
#77 contact_forms_security_601776-77-FAIL.patch1.89 KBkbasarab
#77 contact_forms_security_601776-77.patch4.9 KBkbasarab
#75 contact_forms_security_601776-75.patch4.91 KBkbasarab
#71 contact_forms_security_601776-71.patch4.42 KBZgear
#70 contact_forms_security_601776-63.patch4.42 KBZgear
#64 contact_forms_security_601776-63.patch4.41 KBZgear
#64 interdiff.txt9.62 KBZgear
#62 5-anonymous-user-contact-form-pre-patch.jpg17.04 KBlucascaro
#62 6-authenticated-user-contact-form-pre-patch.jpg23.56 KBlucascaro
#62 7-anonymous-user-contact-form-post-patch.jpg20.08 KBlucascaro
#62 8-authenticated-user-contact-form-post-patch.jpg20.16 KBlucascaro
#61 1-anonymous-pre-patch.png10.02 KBlucascaro
#61 2-authenticated-pre-patch.png15.18 KBlucascaro
#61 3-anonymous-post-patch.png10.17 KBlucascaro
#61 4-authenticated-post-patch.png14.5 KBlucascaro
#57 contact_forms_security_601776-57.patch4.21 KBBerdir
#55 contact_forms_security_601776-55.fix+test.patch3.65 KBJon Pugh
#52 contact_forms_security_601776-51.patch1.51 KBJon Pugh
#50 contact_forms_security-601776-50.test.patch1.51 KBJon Pugh
#50 contact_forms_security-601776-50.fix+test.patch2.3 KBJon Pugh
#48 contact_form_textfields.tests_.patch1.5 KBJon Pugh
#48 contact_form_textfields.fix_.patch2.3 KBJon Pugh
#46 contact_form_textfields.tests_.patch1.5 KBJon Pugh
#41 contact_forms_security-601776-41.patch2.84 KBpillarsdotnet
#36 contact_form_security-601776-36.patch2.84 KBpillarsdotnet
#26 contact_impersonation_security-601776-26.patch1.69 KBpillarsdotnet
#32 contact_impersonation_security-601776-32.patch1.32 KBpillarsdotnet
#25 contact_impersonation_security-601776-25.patch1.66 KBpillarsdotnet
#22 contact_impersonation_security-601776-22.patch1.39 KBpillarsdotnet
#21 contact_impersonation_security-601776-21.patch993 bytespillarsdotnet
#15 contact-name-mail-fields-601776-15.patch4.04 KBdinarcon
#1 601776-contact-name-mail-fields-D7.patch4.47 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs review
FileSize
4.47 KB

Basic patch to display theme_username() instead of textfields for name and e-mail for registered users on both site-wide and personal contact forms. My only concern is that maybe admin users ('administer contact forms' for the site-wide and 'administer users' for personal contact forms) should maybe be able to enter a different e-mail address, so say if I contacted a user on drupal.org about a security issue and wanted to set 'my' email address as security@drupal.org.

xmacinfo’s picture

This is not a good idea to remove entirely this feature. Yes, this is a feature.

For some web sites, we need to use generic user names. For example for a presentation, or a download area. So instead of requiring each user to create an account, we often create a generic username.

To help out these user, we also enable the contact form to send us a message, where they can go and type their name and their email address by replacing the values set for the generic user name.

So please make this a setting or a permission.

However, the more and more I am using the contact module, the more and more I think that for our clients we will enable the webform module.

So if we are using the 80% rule I see often in the issue queue, I would agree with you and ask the 20% use cases administrators to install webform.

So in addition to your concern about admin users, I have concerns for generic user names as well. :-)

What do you think?

Status: Needs review » Needs work

The last submitted patch failed testing.

antelrope’s picture

Allowing a user to enter a different name and email address is not necessarily a bad thing. It is a feature that I and others, it seems, want on their site. Some may not want this functionality. That is fine. This should be a configuration option.
I do advocate always including what account the email was sent from as part of the email.
What does everyone else think?

Dave Reid’s picture

Great thing is you can always un-lock down these fields via a quick form_alter, but I think it's a good idea to lock them down.

xmacinfo’s picture

Well make it an admin contact setting then, defaulted to locked. :-)

antelrope’s picture

Even better would be a permissions setting, that way it could be restricted or allowed based on roles. The only pitfall I see is that it doesn't make sense to dissallow an anonymous user from changing their name and email. Is there a way to make a permission box sticky and always checked for a particular role? If not, the only workaround I can imagine for this is to, in the code, check first for permission and if that fails, check to see if the user is anonymous, if so, grant permission anyhow.

xmacinfo’s picture

Version: 7.x-dev » 8.x-dev

Let's make this 8.x-dev.

This way we will have more time on how to properly handle generic users versus specific ones.

The current solutions does not offer any real fall back. And for end users, it's not always possible to modify the theme.

greggles’s picture

I think the features available by default in core should not create a situation that allows for false e-mails to be sent,which is what this bug does.

If someone wants to configure webform to let webform do that then they have to actively make that decision, but core shouldn't, by default, allow impersonation. As Dave says, if someone really wants to use the contact module and not enforce this then they could do a form_alter.

I don't think it should be exposed as an option. If we expose everything the system becomes too complex. I don't think the use of this feature is very common (I've never seen it used).

Dave Reid’s picture

Priority: Normal » Major

Yeah this just makes no sense to allow users to change information on a contact form that needs to be validated when they change their account info.

sun’s picture

Category: bug » feature
Priority: Major » Normal

Sorry, but: http://api.drupal.org/api/drupal/modules--contact--contact.pages.inc/fun...

May be confusing or annoying, but not a bug.

pillarsdotnet’s picture

sun’s picture

Category: feature » task
Priority: Normal » Critical
Issue tags: +Needs backport to D6, +Security improvements, +Needs backport to D7

Alrighty, @Dave Reid made sure to "highlight" the current situation in a very subtle way ;)

In turn, this can be considered as a publicly disclosed security vulnerability. Thus, let's make sure to fix this soon and backport to D7 and D6.

greggles’s picture

Just so the reputations of the innocent are not damaged, I made that email ;)

dinarcon’s picture

Status: Needs work » Needs review
FileSize
4.04 KB

the attached patch is basically the same as #1 with the following modifications:
- removed $form['#token'] according to recommendation at http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
- use of format_username($user) instead of theme('username', array('account' => $user))

Although this method prevents authenticated users from entering a name and e-mail, it does not prevent anonymous users to do so. Maybe we could add a validation that prevents anonymous users to enter an e-mail address that belongs to a registered user. Could adding a similar validation for the name field be too much? Probably anonymous user handling might go into its own issue.

edit: the file affected by this patch haven't changed between D7 and D8 so the same patch could be applied on both branches

webchick’s picture

Issue tags: +Needs tests

Tests, plz!

Status: Needs review » Needs work

The last submitted patch, contact-name-mail-fields-601776-15.patch, failed testing.

xmacinfo’s picture

@dinarcon: That would mean that a user would need to log in to post a message whereas anonymous users would be able, at anytime, to send a message. I don't think we need to go that far.

What we need is a way to know if the message comes from a registered user or from an anonymous user.

pillarsdotnet’s picture

What we need is a way to know if the message comes from a registered user or from an anonymous user.

like the (verified) or (unverified) attached to comments, right?

xmacinfo’s picture

@pillarsdotnet: Indeed. The security issue here is impersonation. Adding (unverified) would solve this. Or a better wording.

pillarsdotnet’s picture

Title: Don't allow registered users to change their name or e-mail in contact forms » Show the real (as well as the impersonated) name with contact forms.
Status: Needs work » Needs review
FileSize
993 bytes

Very minimal patch to satisfy the security issue.

pillarsdotnet’s picture

Slightly better.

greggles’s picture

I'm not aware of a problem that calls for strip_tags. Can you expand on the need for that?

pillarsdotnet’s picture

Title: Show the real (as well as the impersonated) name with contact forms email "From" name. » Show the real (as well as the impersonated) name with contact forms.

The output of format_username() may include a link to the user profile page, or other tags. The "From" line of an email message will display as plain text, not html. The api page recommends calling check_plain(), but I'd prefer

From: Bob Vincent (pillarsdotnet)

to

From: Bob Vincent (<a href="/drupal/user/36148">pillarsdotnet</a>)

in my emails. How about you?

pillarsdotnet’s picture

Title: Show the real (as well as the impersonated) name with contact forms. » Show the real (as well as the impersonated) name with contact forms email "From" name.
FileSize
1.66 KB

Now includes both username and email info, if they differ.

pillarsdotnet’s picture

Title: Remove the ability of registered users to change the sender name or email address in contact forms. » Show the real (as well as the impersonated) name with contact forms email "From" name.
Priority: Major » Critical
Status: Needs work » Needs review
FileSize
1.69 KB

Adds '(Verified)' if the user info matches the submitted form values.

So a contact email from me would look like one of the following, depending on my submitted form values:

No change of contact info: From: pillarsdotnet (Verified)
Changed name only: From: Bob Vincent (pillarsdotnet)
Changed email address: From: pillarsdotnet (<bobvin@pillars.net>)
Changed both: From: Bob Vincent (pillarsdotnet <bobvin@pillars.net>)
Not logged in: From: Bob Vincent (Anonymous)

EDIT: Removed irrelevant ramblings.

pillarsdotnet’s picture

Title: Show the real (as well as the impersonated) name with contact forms. » Show the real (as well as the impersonated) name with contact forms email "From" name.
Priority: Critical » Major

Tasks aren't critical, and I'm not even convinced that this one qualifies as "major", but splitting the difference.

xmacinfo’s picture

Priority: Major » Critical

I like very much the way you handle this issue.

Personally I would use the example above where we need to only translate on string: (verified). However, is it best to highlight the verified account or the (unverified) ones? In comments, we highlight only the unverified ones.

xmacinfo’s picture

Priority: Critical » Major

Cross-posting.

pillarsdotnet’s picture

Status: Needs work » Needs review

However, is it best to highlight the verified account or the (unverified) ones? In comments, we highlight only the unverified ones.

Added the "Not logged in" case to the tables in #26.

I don't think the comment form allows logged-in users to provide a name and email other than their own. But the contact form does, and some people don't want to remove that "feature".

Dave Reid’s picture

Status: Needs review » Needs work

The output of format_username() may include a link to the user profile page, or other tags.

No, that's theme_username(), not format_username(), which always returns plain-text username only. Running strip_tags is unnecessary.

pillarsdotnet’s picture

Okay, but the API page calls it "unsanitized text". Is it safe to stuff into an email header?

sun’s picture

Status: Needs review » Needs work

I'd highly recommend to go back to the patch in #1.

The original approach is mostly identical to what happens in comment_form(). It doesn't make sense to expose a username and e-mail field for registered users. Sites with use-cases like @xmacinfo mentioned can happily use Webform or write a 4 lines hook_form_alter() to unlock the fields.

The only remark I have about #1 is that it should not use a conditional form structure for the 'name' field; i.e., the form element should always be contained, but #access => FALSE for authenticated users.

And lastly, we're still missing tests, of course.

pillarsdotnet’s picture

Title: Show the real (as well as the impersonated) name with contact forms email "From" name. » Remove the ability of registered users to change the sender name or email address in contact forms.

Re-titling then.

xmacinfo’s picture

Title: Show the real (as well as the impersonated) name with contact forms email "From" name. » Remove the ability of registered users to change the sender name or email address in contact forms.
Priority: Critical » Major
Status: Needs review » Needs work

@Sun: Does patch in #1 let an anomymous user impersonate a user? When opening a message from a mailbox, how can we spot the difference?

pillarsdotnet’s picture

Assigned: Dave Reid » Unassigned
Status: Needs work » Needs review
FileSize
2.84 KB

Attached patch:

  • Forbids logged-in users from changing the sender name or email (sets #access to !$user->uid).
  • Adds inline comments explaining why this restriction was put in place.
  • Appends ' (Unverified)' to the sender name for anonymous usage.

If this is a satisfactory approach, I will write tests.

Status: Needs review » Needs work

The last submitted patch, contact_form_security-601776-36.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review

Account tests fail because they require what this patch forbids. Again, if someone agrees this is the correct approach, I will (re-)write tests.

EDIT: bump.

Dave Reid’s picture

I'd rather use '#type' => 'item' for both name and e-mail for registered users, so we can at least remind people what the mail will look like it comes from, rather than just not displaying that information.

pillarsdotnet’s picture

@Dave Reid:

I'd rather use '#type' => 'item' for both name and e-mail for registered users

Are you voting for the patch in #32 or for something different?

EDIT: Now I see what you mean. Will roll another patch.

pillarsdotnet’s picture

Attached patch implements the suggestion of Dave Reid In #39.

Again, I expect tests will fail, because they require what we now want to forbid. If we can agree that this is the correct approach, I will re-write tests to match.

Status: Needs review » Needs work

The last submitted patch, contact_forms_security-601776-41.patch, failed testing.

xmacinfo’s picture

I like the way you are approaching this issue. I thank you for the addition of the (Unverified) string.

From my point of view you can start building the tests.

Anonymous’s picture

Good to see someone on this. So, say I wanted to disable the ability for registered users to change the e-mail or name on the contact form in D7, which one of these patches will allow me to do this? I realise that it's nice to have the option, especially for anonymous users, to change these things, but I want a 'disable custom user name/e-mail' check box in the contact form admin panel.

xmacinfo’s picture

@Borden Rhodes: The last patch should do the trick. But you’d need to reroll it for Drupal 7.

Jon Pugh’s picture

Tests!!

only tests for logged in users so far.

Jon Pugh’s picture

Status: Needs work » Needs review
Jon Pugh’s picture

Heres the fix and the tests. I have to leave, so sorry if I forgot additional lines! Its not an exact re-roll, but it works

Status: Needs review » Needs work

The last submitted patch, contact_form_textfields.fix_.patch, failed testing.

Jon Pugh’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
1.51 KB

new patches...

Status: Needs review » Needs work

The last submitted patch, contact_forms_security-601776-50.fix+test.patch, failed testing.

Jon Pugh’s picture

The fix doesn't quite work yet, still have to pull the values on submit.

Here's a patch just for the test, hopefully it fails properly :D

Jon Pugh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, contact_forms_security_601776-51.patch, failed testing.

Jon Pugh’s picture

Status: Needs work » Needs review
FileSize
3.65 KB

Test failed successfully!

Attached is what is hopefully a final patch. Emails are sending, personal form is also now patched.

Status: Needs review » Needs work

The last submitted patch, contact_forms_security_601776-55.fix+test.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

Fixed the comment coding style and fixed the failing tests.

xjm’s picture

Issue tags: -Needs tests +Novice

Thanks @careernerd and @Berdir. The test looks good.

I found a few more minor stylistic issues:

  1. +++ b/core/modules/contact/contact.testundefined
    @@ -175,6 +175,10 @@ class ContactSitewideTestCase extends DrupalWebTestCase {
    +    // Logout user to to be able to set a name and mail address.
    
    • The word "to" is repeated twice.
    • "Log out" should be two words when it's a verb.
    • It should be "email" rather than "mail address," no? The latter sounds like a postal address.
    • The comment is confusing.

    I'd suggest:
    Log the current user out in order to test the name and email fields.

  2. +++ b/core/modules/contact/contact.testundefined
    @@ -432,3 +436,36 @@ class ContactPersonalTestCase extends DrupalWebTestCase {
    +/**
    + * Checks that textfields for email address and name only appear for anonymous users.
    + */
    +class ContactTextfieldTestCase extends DrupalWebTestCase {
    

    The docblock summary should be a single line of less than 80 characters. Also, I'd suggest changing the text here (and in the getInfo() and test method docblock) because the test doesn't actually have anything to do with anonymous users.

    I'd suggest:

    • Renaming the test case to ContactAuthenticatedTestCase
    • Changing the docblock to:
      Tests the contact form for authenticated users.
    • Updating the getInfo() accordingly.
  3. +++ b/core/modules/contact/contact.testundefined
    @@ -432,3 +436,36 @@ class ContactPersonalTestCase extends DrupalWebTestCase {
    +   * Checks that for a logged in user, textfields do not appear on /contacts
    

    This comment needs a period. It could also be a little clearer. Maybe:
    Tests that name and email fields are not present for authenticated users.

  4. +++ b/core/modules/contact/contact.testundefined
    @@ -432,3 +436,36 @@ class ContactPersonalTestCase extends DrupalWebTestCase {
    +    $this->assertFalse($this->xpath('//input[@name=:name]', array(':name' => 'name')), t('"Name" textfield exists for logged in user.'));
    ...
    +    $this->assertFalse($this->xpath('//input[@name=:name]', array(':name' => 'mail')), t('"Email" textfield exists for logged in user.'));
    

    The assertion messages here need not be translated. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

These are straightforward cleanups, so tagging novice for the task of making those changes. The correct failures of the added test are already exposed in #53.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots

Wait, what happened to the "Unverified" string? It seems to have disappeared in the latest patches but I don't see a justification for that change.

Also, couple more minor points for cleanup:

+++ b/core/modules/contact/contact.pages.incundefined
@@ -76,6 +76,24 @@ function contact_site_form($form, &$form_state) {
+    //Change form elements to values

This comment needs a space after // and a period at the end. Reference: http://drupal.org/node/1354#inline

+++ b/core/modules/contact/contact.pages.incundefined
@@ -76,6 +76,24 @@ function contact_site_form($form, &$form_state) {
+    // Display readonly name and mail address to the user.

"Read-only" should be hyphenated.

Once the patch is fixed for the points above, let's also get some screenshots of the way the form looks for anonymous vs. authenticated users for reviewers, and an updated summary while we're at it. Note: please make screenshots in a small browser window and crop only to the relevant portion of the screen.

lliss’s picture

Added an issue summary.

lucascaro’s picture

Here are some screenshots for anonymous and authenticated users both before and after applying the patch.

Anonymous user, before the patch:

1-anonymous-pre-patch.png

Authenticated user before the patch:

2-authenticated-pre-patch.png

Anonymous user, after the patch:

3-anonymous-post-patch.png

Authenticated user after the patch:

4-authenticated-post-patch.png

lucascaro’s picture

here are some screenshots for the user contact form (the ones in #61 are for the site-wide contact form). These are in the same order as before.

5-anonymous-user-contact-form-pre-patch.jpg

6-authenticated-user-contact-form-pre-patch.jpg

7-anonymous-user-contact-form-post-patch.jpg

8-authenticated-user-contact-form-post-patch.jpg

Zgear’s picture

Assigned: Unassigned » Zgear

about to reroll patch with changes, gimme a sec.

Zgear’s picture

Here is the patch with the changes suggested and an interdiff :)

Zgear’s picture

Status: Needs work » Needs review

The last submitted patch, contact_forms_security_601776-63.patch, failed testing.

Zgear’s picture

Status: Needs work » Needs review

The last submitted patch, contact_forms_security_601776-63.patch, failed testing.

Zgear’s picture

:( ok let me take a look at it

Zgear’s picture

sorry about that guys, this should do the job

Zgear’s picture

ah my bad, here it is with the correct name

Zgear’s picture

Status: Needs work » Needs review
Kevin Morse’s picture

Verified the behaviour of the forms illustrated above is working in all four test cases.

I do not have the ability to send mail from my local test server so I cannot comment on the appearance of the emails themselves.

xjm’s picture

Few more minor cleanups:

  1. +++ b/core/modules/contact/contact.pages.incundefined
    @@ -76,6 +76,24 @@ function contact_site_form($form, &$form_state) {
    +  // We do not allow authenticated users to alter their name and email
    +  // here because they could impersonate someone else.
    
    @@ -212,6 +229,24 @@ function contact_personal_form($form, &$form_state, $recipient) {
    +  // We do not allow authenticated users to alter their name and email
    +  // here because they could impersonate someone else.
    

    This comment looks like it's wrapping too early. Each line should go as close to 80 chars as possible without going over. Also, let's remove the word "we" in accordance with our text guidelines (http://drupal.org/node/604342). I'd rephrase the comment as:
    Do not allow authenticated users to alter the name or email values to prevent the impersonation of other users.

  2. +++ b/core/modules/contact/contact.pages.incundefined
    @@ -129,7 +147,6 @@ function contact_site_form_validate($form, &$form_state) {
       global $user, $language_interface;
    -
       $values = $form_state['values'];
    

    Unrelated change; let's remove this to reduce patch conflicts.

  3. +++ b/core/modules/contact/contact.pages.incundefined
    @@ -212,6 +229,24 @@ function contact_personal_form($form, &$form_state, $recipient) {
    +    // Display readonly name and mail address to the user.
    

    "Read-only" still needs a hyphen. :)

Additionally, when uploading the final patch, let's also upload a test-only version to expose the expected failures without the bugfix.

Once the patch is updated, it would be good if someone could test the sent emails manually, and double-check that the "unverified" string is there.

Finally, I'm updating the summary with the screenshots from #61. Thanks @lliss, @lucascaro, and @Zgear!

xjm’s picture

Issue summary: View changes

Added issue summary according to https://drupal.org/node/1155816

kbasarab’s picture

Uploading a revised patch taking xjm's suggestions into patch.

Trying to create the failure test though isn't working so well because DrupalWebTestCase is no longer in the recent head of core. I'm gonna reroll with changing the test case but wanted to get this one up with just the style changes.

Status: Needs review » Needs work

The last submitted patch, contact_forms_security_601776-75.patch, failed testing.

kbasarab’s picture

Alrighty lets try these. One that passes and one that will fail out testing just the test case.

Also an Interdiff between this patch and #71

kbasarab’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, contact_forms_security_601776-77-FAIL.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing

That looks good to me. Thanks @kbasarab.

Now let's just have someone test sending an email as an anonymous user. If that works correctly I think this is RTBC.

damiankloip’s picture

Assigned: Zgear » damiankloip

I will look at this and re-roll; contact.test doesn't exist anymore. Tests are now in ContactPersonalTest.php and ContactSitewideTest.php.

kwattro’s picture

Hi,

I've re-rerolled the patch, applied the #77 with --reject option and made manually the changes to tests.

I've also created a new test class ContactAuthenticatedUser.php, actually tests are passing on my local 8.x .

Manually tested :

  • Site-wided contact form as auth. user: can't modifiy my email and name
  • Site-wided contact form as unauth. user: e-mail is well sent
damiankloip’s picture

Assigned: damiankloip » Unassigned

I'll unassign myself then. Saves me some time :)

kwattro’s picture

kwattro’s picture

Sent for Retest after the recent merge of the wscci/kernel initative

kwattro’s picture

I've rerolled the patch against the latest update of 8.x

xmacinfo’s picture

Status: Needs review » Needs work
+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactAuthenticatedUserTest.php
@@ -0,0 +1,44 @@
+/**
+ * @file ¶
+ * Definition of Drupal\contact\ContactAuthenticatedUserTest
+ *
+ */

White space after @file.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactAuthenticatedUserTest.php
@@ -0,0 +1,44 @@
+    // Ensure that there is no textfield for email.
+    $this->assertFalse($this->xpath('//input[@name=:name]', array(':name' => 'mail')));
+  }
+}
\ No newline at end of file

Needs a newline at end of file.

kwattro’s picture

Fixed and rebased on the latest 8.x

kwattro’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, contact_forms_security_601776-88.patch, failed testing.

kwattro’s picture

Status: Needs work » Needs review
FileSize
5.36 KB

Missed something in the rebase :-(

Retry

SuperHeron’s picture

Tested on a fresh d8. Looks OK. It could become RTBC.

jfhovinne’s picture

Tested site-wide and user contact forms on a fresh D8, as anonymous user and authenticated user.
Ability for logged in users to change name and email address on the contact form is removed.
The string t('Unverified') is added to the user contact form emails sent by anonymous users, but NOT to the site-wide contact form.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/contact/contact.pages.incundefined
@@ -76,6 +76,25 @@ function contact_site_form($form, &$form_state) {
+    // Display read-only name and mail address to the user.

e-mail, not mail

+++ b/core/modules/contact/contact.pages.incundefined
@@ -212,6 +231,24 @@ function contact_personal_form($form, &$form_state, $recipient) {
+    // Display read-only name and mail address to the user.

e-mail, not mail

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactAuthenticatedUserTest.phpundefined
@@ -0,0 +1,44 @@
+ * Definition of Drupal\contact\ContactAuthenticatedUserTest
+ *
+ */

There's an extra blank line here, and the Definition line is missing a trailing full stop.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactAuthenticatedUserTest.phpundefined
@@ -0,0 +1,44 @@
+      'name' => 'Contact form textfields.',
+      'description' => 'Tests contact form textfields are present if authenticated',

The name shouldn't have a full stop, but the description should

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactAuthenticatedUserTest.phpundefined
@@ -0,0 +1,44 @@
+    parent::setUp('contact');

Eventually all calls to setUp will expect an array, might as well switch this while rerolling.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactAuthenticatedUserTest.phpundefined
@@ -0,0 +1,44 @@
+    $user = $this->drupalCreateUser(array('access site-wide contact form'));
+    $this->drupalLogin($user);

Might as well combine this line, that will also avoid calling it $user. $this->drupalLogin($this->drupalCreateUser(array('access site-wide contact form'));

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -180,6 +180,10 @@ class ContactSitewideTest extends WebTestBase {
+    // Log the current user out in order to test the name and email fields.

e-mail, not email

kwattro’s picture

Thanks @jfhovinne & @tim.plunkett for your review, will update it during the week.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.33 KB
5.35 KB

Attached is the changes requested with an interdiff of the last patch.

jfhovinne’s picture

Tested latest patch on d8.

User interface is OK (4 cases: anon/auth - user/site-wide contact forms).

The 'Unverified' string (anonymous case) is added to the message sent while using the user contact form, but not to the message sent using the site-wide form.

I don't think this is the expected behaviour, so I attach the modified patch - now the string 'Unverified' is also added while using the site-wide contact form.

tim.plunkett’s picture

Just a reroll and to see if the tests actually fail on their own.

dags’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Tested and can confirm that it works as intended, including jfhovinne's addition of Unverified to the site-wide contact form.

Dan Z’s picture

Good stuff, and I hope to see it back-ported to D7 soon. Two important considerations:

First, it is still easily possible for authenticated users to spoof e-mail addresses via this form, even with this improvement. The user merely has to change his own e-mail address in his profile and then use the form. A bot could easily change it repeatedly, and use the "Send yourself a copy" checkbox to distribute spam.

A solution is available at http://drupal.org/project/email_required. It requires that you verify your e-mail address (respond to a message sent there) before using specified pages, and the contact page can specified. It appears to take effect on new (unverified) accounts, or after the e-mail address is changed.

Second, if hook_form_alter() or other means is used to modify the "Your e-mail address" value, for whatever reason, the "Send yourself a copy" feature should be disabled in most cases, as it could be used for spam. Instead of merely checking if it is an anonymous user, there should be a check to see if the e-mail address used for the contact function matches the e-mail address in the user profile.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

At a glance this looks good to me. Committed and pushed to 8.x, thanks!

Marking to 7.x for backport.

tim.plunkett’s picture

Unfortunate commit message is unfortunate.
http://drupalcode.org/project/drupal.git/commit/20d4ceb

Dan Z’s picture

I "back-ported" this fix to D7 via a new module. This makes it available immediately, without waiting for the next core release. See http://drupal.org/sandbox/Z-TwistBooks/1704772 for the module.

For those not familiar with the git pages, the module can be downloaded by clicking on an appropriate "snapshot" link at http://drupalcode.org/sandbox/Z-TwistBooks/1704772.git.

Given that this is scheduled for backporting into D7 core anyway, is it worth making a full-blown project out of this?

greggles’s picture

Given that this is scheduled for backporting into D7 core anyway, is it worth making a full-blown project out of this?

I don't think so.

ZenDoodles’s picture

Backported to D7.

ZenDoodles’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.03 KB
744 bytes

Actually attach 'em this time.

Status: Needs review » Needs work

The last submitted patch, 601776-D7-use_profile_contact-105.patch, failed testing.

ZenDoodles’s picture

Status: Needs work » Needs review
FileSize
5.2 KB
1.89 KB

Sorry for the spam. I missed a file earlier in the tests.

Status: Needs review » Needs work

The last submitted patch, 601776-106-drupal7-combined.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
4.78 KB

Fix for D7

ryan.gibson’s picture

Tested the patch from #110. Seems to work great. Can't alter the name and email after applied.

jaffaralia’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and Tested the patch #110. It works perfectly. Authenticated user can't able to alter the name and email after this patch applied.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

First, it is still easily possible for authenticated users to spoof e-mail addresses via this form, even with this improvement. The user merely has to change his own e-mail address in his profile and then use the form.

Is this correct? Offhand, it sounds to me like it is (and for users with permission to change their own username the same thing goes for the name field too)...

If so, we need to discuss the security considerations here more. If there isn't much (or any) security benefit in the end, then we shouldn't backport this, because it makes changes to the form structure on two very public-facing forms, so per http://drupal.org/node/1527558 we shouldn't do it unless it fixes an important issue.

There might be some benefits of this patch either way (ones I can think of include: can't exactly spoof the name/e-mail of another registered user anymore, can't spoof the name anymore if you don't have "change own username" permission, can't spoof anything anymore if you stumble upon someone's already-logged-in account on a public computer) but those seem significantly less than the advertised benefits of this patch and also might be achievable with other, less intrusive changes... So yeah, let's please discuss this more.

****

A couple other things looking at the patch and while this is back on Drupal 8 anyway:

+    // Change form elements to values.
+    $form['name']['#type'] = $form['mail']['#type'] = 'value';

Why is this changing #type rather than setting #access FALSE? This is especially important for possible D7 backport, since it's a lot friendlier to any other code that's changing this via hook_form_alter().

+      '#markup' => user_format_name($user),

Seems wasteful to call this function twice (especially since calling it invokes a hook), rather than storing the previous call from earlier in the same function in a variable and then reusing it here.

+  if ($user->uid){
...
+  if ($user->uid){

Minor code style (missing space before the brackets).

   // Save the anonymous user information to a cookie for reuse.
   if (!$user->uid) {
+    $values['sender']->name .= ' (' . t('Unverified') . ')';
     user_cookie_save(array_intersect_key($values, array_flip(array('name', 'mail'))));
   }

The newly added line of code doesn't seem to have anything to do with the code comment above it. (Because of that I almost thought there was a bug here where "Unverified" was being saved to the cookie, but there isn't.)

Also, this part of the patch looks like it might be a good to backport to Drupal 7 regardless of the rest, but why use "Unverified" here rather than "not verified"? (For consistency with the rest of Drupal, and, for Drupal 7 backport, for the sake of not adding a new string for no reason...)

Dan Z’s picture

First, it is still easily possible for authenticated users to spoof e-mail addresses via this form, even with this improvement. The user merely has to change his own e-mail address in his profile and then use the form.

Is this correct?

...

If so, we need to discuss the security considerations here more. If there isn't much (or any) security benefit in the end, then we shouldn't backport this,

There is is absolutely a good security benefit here and this fix should be backported.

There are two holes that let the user spoof e-mail. Hole #1 is changing the e-mail address via this form. Hole #2 is changing the e-mail address via the profile. Both holes need to be plugged.

This fix plugs up hole #1. That's a good thing. We still need to plug up hole #2, that's all. However, that's a different issue that should be addressed separately.

One way to plug hole #2 is to add some sort of verification when someone changes his e-mail address. For example, the module at http://drupal.org/project/email_required does this.

David_Rothstein’s picture

For Drupal 8, addressing them separately definitely makes sense. But for Drupal 7, we're talking about changing the behavior in a stable release for all sites that use the Contact module (and changing it in a way that not only changes the form structure but also removes a feature that according to several of the early comments in this issue, some people's sites are actually relying on)....

So the question is, if you need to install a contrib module to get a real security benefit from this anyway, then in Drupal 7, why not leave it up to that contrib module to change this behavior also, so that only sites which will need/want/benefit from the change actually get it?

Maybe we will decide that this patch actually does provide enough of a security benefit on its own that it's worth doing in Drupal 7. I just want to make sure we have that conversation.

Dan Z’s picture

Oh, it's worth backporting, and the sooner the better. I'm confused as to why e-mail spoofing for authenticated users was ever allowed in the first place through this form. I'm even more confused as to why it hasn't been fixed after nearly three years!

Very few administrators are going to want to allow spoofing. It's not even worth adding an "Allow authenticated users to use fake e-mail addresses" option to the admin/structure/contact form, because almost nobody would check it. Those few who do want spoofing can enable it via a hook_form_alter() similar to the module in #103.

Dan Z’s picture

Version: 8.x-dev » 7.x-dev

The patch for 8.x has already been committed and pushed. Moving this back to 7.x. Please create a new issue for any issues with code committed to 8.x.

Regarding #113:

I don't see any difference with making the '#type' a 'value' and setting '#access' to false. Any module using hook_form_alter() can easily replace the whole field either way.

Your notes about the style and re-use issues seem valid. Please submit a new patch to fix them.

ACF’s picture

Status: Needs work » Needs review
FileSize
5.11 KB

Reattached patch with small fixes.

jhood’s picture

I applied and tested patch #118 and everything seems to be working as it should.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Changes still need to go into Drupal 8 first before being backported.

Some of the minor ones could definitely be split off to a separate issue, but not any of the ones that fundamentally affect the Drupal 7 backport (such as #type='value' vs. #access). And as long as we're moving it back for those, it's probably just a lot simpler to fix the minor issues at the same time.

I don't see any difference with making the '#type' a 'value' and setting '#access' to false. Any module using hook_form_alter() can easily replace the whole field either way.

Yes, but it complicates things considerably because normally a module will not do that. The standard way is to use #access, and if a module writes code to set a form element's access to TRUE or FALSE based on certain conditions, that code really should "just work" (rather than having to remember to do something different for this particular form element).

That's especially true for Drupal 7, where such code might already exist and we don't want to break it. The backport policy (see http://drupal.org/node/1527558 which links to http://groups.drupal.org/node/210973#comment-701613) mentions that changing #type is considered to be a pretty intrusive change.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
3.41 KB

Here's a followup patch for Drupal 8 addressing the full list of feedback from #113.

jaffaralia’s picture

Status: Needs review » Reviewed & tested by the community

Here i applied and tested the patch #121. Its working fine..

Dan Z’s picture

Version: 7.x-dev » 8.x-dev

The matter of being able to change your e-mail address on your user profile page without verification is a known issue and being addressed at #85494: Use email verification when changing user email addresses. It could use more attention....

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Those all look like sensible clean-ups.

Committed and pushed to 8.x. Back to 7.x now.

And I'm really sorry, ACF, but I pressed the button too fast and accidentally didn't add your name to the commit message. :( Your next core patch, I will add you twice to make up for it. :D

ACF’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.25 KB

Have ported the patch to d7 using David_Rothstein's changes.

jhood’s picture

I applied patch #125 and it is working fine in D7.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.09 KB
4.92 KB

I tested #125 on D7 and it seems to work well. Before and after images are attached.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » David_Rothstein

Leaving this one for David, unless he gives the go-ahead for me to take it. Sounds like he had some concerns.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Critical
Status: Reviewed & tested by the community » Needs work
-      '#markup' => user_format_name($user),
+      '#markup' => $form['name']['#default_value'],

Yikes, I can't believe I missed this before, but both of these lines (the one from the original patch and the one my followup patch changed it to) are a security hole.

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
Status: Needs work » Needs review
FileSize
1.32 KB

This should fix it.

Hopefully after that, we can actually put this issue back on Drupal 7 and be able to leave it there for a real discussion :)

dcam’s picture

FileSize
4.33 KB
3.74 KB

I tested #130 and it fixes the vulnerability. Before and after images are attached.

+1 for RTBC.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks good :)

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Ok, ONE last time, I hope. :D

Committed and pushed to 8.x. Thanks!

Back to you, David!

webchick’s picture

Priority: Critical » Major

Oh, and reducing back down to just "major" now that the security vulnerability is dealt with.

dcam’s picture

Priority: Major » Critical
Status: Patch (to be ported) » Needs review
FileSize
5.3 KB

This is the same patch from #125 for D7 with the security improvements from #130 added.

tim.plunkett’s picture

Priority: Critical » Major

Crosspost.

dcam’s picture

Oops, I didn't mean to change the status back to critical. The opened the page to post the patch before webchick downgraded it to major.

YesCT’s picture

#135: 601776-contact-core-134.patch queued for re-testing.

YesCT’s picture

this

looks like the issue summary is pretty up-to-date.

  • Minor cleanups.
  • Manually test the email sent for both the sitewide and personal contact forms for both anonymous and authenticated users.

for the Minor cleanups... what detail can we write down?
so that a novice has more direction to create a patch (contributor task doc if it's helpful for someone: http://drupal.org/node/1424598)

and a help doc for the manually test in case someone wants to jump into this: http://drupal.org/node/1489010

... :) and in case the previous patch does not apply, reroll doc: http://drupal.org/patch/reroll

YesCT’s picture

YesCT’s picture

#135: 601776-contact-core-134.patch queued for re-testing.

lovelykaushik86’s picture

lovelykaushik86’s picture

Issue summary: View changes

Updated issue summary.

greggles’s picture

I think at least this set of tags was removed accidentally. Not sure about the novice tag.

royal121’s picture

Patch in comment #135 works fine.

mgifford’s picture

135: 601776-contact-core-134.patch queued for re-testing.

blasthaus’s picture

Patch in #135 works. but would it not be simpler to just make the textfields 'readonly' as this formatting will cause headaches for a lot of themes?

<?php
  if ($user->uid) {
    // Make original name and e-mail address fields read-only
    $form['name']['#attributes']['readonly'] = '';
    $form['mail']['#attributes']['readonly'] = '';
  }
?>
jukka792’s picture

Is this implemented in the core already?
I am still able to change the email and username (in the contact form) when sending message as authenticated user to another. Drupal 7.39.

xmacinfo’s picture

The best way to have this issue implemented in core is to review the code in comment #135, test it and report back.

  • webchick committed 6de13da on 8.3.x
    Issue #601776 follow-up by David_Rothstein: Improve security hardening...
  • webchick committed ce9a30b on 8.3.x
    Issue #601776 follow-up by David_Rothstein: Further security hardening...

  • webchick committed 6de13da on 8.3.x
    Issue #601776 follow-up by David_Rothstein: Improve security hardening...
  • webchick committed ce9a30b on 8.3.x
    Issue #601776 follow-up by David_Rothstein: Further security hardening...

  • webchick committed 6de13da on 8.4.x
    Issue #601776 follow-up by David_Rothstein: Improve security hardening...
  • webchick committed ce9a30b on 8.4.x
    Issue #601776 follow-up by David_Rothstein: Further security hardening...

  • webchick committed 6de13da on 8.4.x
    Issue #601776 follow-up by David_Rothstein: Improve security hardening...
  • webchick committed ce9a30b on 8.4.x
    Issue #601776 follow-up by David_Rothstein: Further security hardening...
dxvargas’s picture

Status: Needs review » Reviewed & tested by the community

I tested and #135 works.
Nice code, tests passed!

@blasthaus I agree that a readonly field could be better for theming. But this issue has some security concerns associated, it is too old and we should just move on with what we have, it is a big improvement. Marking it as RTBC!

blasthaus’s picture

We abandoned using the contact module and just rolled our own after seeing some malicious attempts at sending POSTs to this path. It's amazing that this issue is 8 years old and still no love for D7. +1 for a commit before we break a record here :-)

stefan.r’s picture

Reuploading to trigger tests

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 155: 601776-contact-core-134.patch, failed testing.

  • webchick committed 6de13da on 9.1.x
    Issue #601776 follow-up by David_Rothstein: Improve security hardening...
  • webchick committed ce9a30b on 9.1.x
    Issue #601776 follow-up by David_Rothstein: Further security hardening...
xmacinfo’s picture

What's happening with this issue and multiple commits? Last patch was failing.

greggles’s picture

Those are a new branch, 9.1.x. Whenever a new branch is opened up it happens like it did for 8.3.x and 8.4.x.