Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-57531

Address the vulnerabilities in recent PHPMailer 5.2.x

    Details

    • Testing Instructions:
      Hide

      On versions 3.1 and below enable emailonlyfromnoreplyaddress.

      in admin > server > email > outgoing mail configuration
      setting an invalid email address in the noreply address field - ensure that form validation works correctly and an invalid email address cannot be set.

      With an invalid noreply set (via config.php or directly in db before this patch)
      trigger an e-mail to be sent and check that the noreply address has been set to:
      noreply@(SITEURL)

      with a valid noreply set, trigger an e-mail to be sent and check that the noreply address has been set to the noreply address configured.

      in admin > server > email > outgoing mail configuration
      add a new allowed email domain: eg: *.moodle.org
      Trigger an e-mail from a user that has a valid *.moodle.org address and check to see if that email is set in the from component of the sent email.
      Trigger an e-mail from a user that has a *.moodle.org address that is not valid (changed at db level to something like "moodle@moodle.org>\r\nRCPT TO:<victim@example.com"
      check to make sure a debugging message appears when this e-mail is attempted and no e-mail is sent.

      Show
      On versions 3.1 and below enable emailonlyfromnoreplyaddress. in admin > server > email > outgoing mail configuration setting an invalid email address in the noreply address field - ensure that form validation works correctly and an invalid email address cannot be set. With an invalid noreply set (via config.php or directly in db before this patch) trigger an e-mail to be sent and check that the noreply address has been set to: noreply@(SITEURL) with a valid noreply set, trigger an e-mail to be sent and check that the noreply address has been set to the noreply address configured. in admin > server > email > outgoing mail configuration add a new allowed email domain: eg: *.moodle.org Trigger an e-mail from a user that has a valid *.moodle.org address and check to see if that email is set in the from component of the sent email. Trigger an e-mail from a user that has a *.moodle.org address that is not valid (changed at db level to something like "moodle@moodle.org>\r\nRCPT TO:<victim@example.com" check to make sure a debugging message appears when this e-mail is attempted and no e-mail is sent.
    • Affected Branches:
      MOODLE_27_STABLE, MOODLE_30_STABLE, MOODLE_31_STABLE, MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE, MOODLE_30_STABLE, MOODLE_31_STABLE, MOODLE_32_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
      MDL-57531-27-phpmailer
    • Pull 3.1 Branch:
      MDL-57531-31-phpmailer
    • Pull 3.2 Branch:
      MDL-57531-32-phpmailer
    • Pull Master Branch:
      MDL-57531-master-phpmailer

      Description

      PHPMailer should be updated to 5.2.21+ - was .18 when opening this issue BUT a 0-day vulnerability was found in it: CVE-2016-10045. See more details in the comments too - in all the security supported branches, https://legalhackers.com/advisories/PHPMailer-Exploit-Remote-Code-Exec-CVE-2016-10033-Vuln.html:

      A successful exploitation could let remote attackers to gain access to
      the target server in the context of the web server account which could
      lead to a full compromise of the web application.

      At the time of .18 there was already an exploit but not publicly available but then when .18 was released a public exploit was incorrectly published (then becoming a 0-day vulnerability!): PHPMailer has already patched the code for both the two CVEs.

      Please keep care of new properties/features to avoid kind of MDL-52637 and MDL-57474 issues:

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks Matteo for reporting this. I just learned about the phpmailer issue from twitter and found your report here.

              Firstly, I am wondering if we should remove the Security label from this issue. It is a security issue indeed. But it is an issue in the upstream code and the details about it have been partially published already. By having this issue marked as security, the moodle community is not aware about our work on this. So my +1 to open this issue to public to prevent more duplicates. Ping Eloy Lafuente (stronk7), Dan Poltawski, Marina Glancy for opinions.

              For the record, Moodle comes with the following PHPMailer versions:

              • 3.2 ships with phpmailer 5.2.16 (potentially affected by CVE-2016-10033)
              • 3.1 ships with phpmailer 5.2.14 (potentially affected by CVE-2016-10033)
              • 3.0 ships with phpmailer 5.2.13 (potentially affected by CVE-2016-10033, CVE-2015-8476)
              • 2.7 ships with phpmailer 5.2.7 (potentially affected by CVE-2016-10033, CVE-2015-8476, CVE-2008-5619)

              Source: https://github.com/PHPMailer/PHPMailer/blob/master/SECURITY.md

              By looking at the actual fix for the latest security issue https://github.com/PHPMailer/PHPMailer/compare/v5.2.17...PHPMailer:v5.2.18 it seems to me that vulnerable are sites that allow the Sender field be set by the user (attacker). It is not the moodle's case. We set that field explicitly to either the noreply address, or an email processing address (for email bouncing).

              So my current conclusion is that Moodle sites are not affected by the Sender vulnerability discovered in phpmailer < 5.2.18.

              Show
              mudrd8mz David Mudrák added a comment - Thanks Matteo for reporting this. I just learned about the phpmailer issue from twitter and found your report here. Firstly, I am wondering if we should remove the Security label from this issue. It is a security issue indeed. But it is an issue in the upstream code and the details about it have been partially published already. By having this issue marked as security, the moodle community is not aware about our work on this. So my +1 to open this issue to public to prevent more duplicates. Ping Eloy Lafuente (stronk7) , Dan Poltawski , Marina Glancy for opinions. For the record, Moodle comes with the following PHPMailer versions: 3.2 ships with phpmailer 5.2.16 (potentially affected by CVE-2016-10033) 3.1 ships with phpmailer 5.2.14 (potentially affected by CVE-2016-10033) 3.0 ships with phpmailer 5.2.13 (potentially affected by CVE-2016-10033, CVE-2015-8476) 2.7 ships with phpmailer 5.2.7 (potentially affected by CVE-2016-10033, CVE-2015-8476, CVE-2008-5619) Source: https://github.com/PHPMailer/PHPMailer/blob/master/SECURITY.md By looking at the actual fix for the latest security issue https://github.com/PHPMailer/PHPMailer/compare/v5.2.17...PHPMailer:v5.2.18 it seems to me that vulnerable are sites that allow the Sender field be set by the user (attacker). It is not the moodle's case. We set that field explicitly to either the noreply address, or an email processing address (for email bouncing). So my current conclusion is that Moodle sites are not affected by the Sender vulnerability discovered in phpmailer < 5.2.18.
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi David;
              I was in doubt to mark it as a security issue too: especially for your conclusion.
              So +1 to remove the label but I'm not from HQ so mine is just an opinion and... read more below.

              AFAIK people will ask for the last PHPMailer vulnerability status in Moodle even if not exploitable with the current code from the main stream: indeed moodle_phpmailer doesn't set it by default so it's up to the consumer to do it properly, see get_mailer() and email_to_user() i.e. some plug-ins could be affected: my +1 to update PHPMailer to the latest version (keeping care in 2.7 to backport at least MDL-52637 too).

              HTH,
              Matteo

              Show
              matteo Matteo Scaramuccia added a comment - Hi David; I was in doubt to mark it as a security issue too: especially for your conclusion. So +1 to remove the label but I'm not from HQ so mine is just an opinion and... read more below. AFAIK people will ask for the last PHPMailer vulnerability status in Moodle even if not exploitable with the current code from the main stream: indeed moodle_phpmailer doesn't set it by default so it's up to the consumer to do it properly, see get_mailer() and email_to_user() i.e. some plug-ins could be affected: my +1 to update PHPMailer to the latest version (keeping care in 2.7 to backport at least MDL-52637 too). HTH, Matteo
              Hide
              mudrd8mz David Mudrák added a comment -

              I am not saying we should not update it. But I would update it in Moodle 3.2 only. Otherwise we risk all the regressions (such as your issue or another MDL-57474) to happen on LTS releases, for no real reason.

              Show
              mudrd8mz David Mudrák added a comment - I am not saying we should not update it. But I would update it in Moodle 3.2 only. Otherwise we risk all the regressions (such as your issue or another MDL-57474 ) to happen on LTS releases, for no real reason.
              Hide
              matteo Matteo Scaramuccia added a comment -

              OK, point taken but please let (dev only?) people understand that third party plug-ins could instantiate PHPMailer by their own without passing through the Moodle Mail way (API we could say... indeed the higher API exists: https://docs.moodle.org/dev/Message_API).

              Time for a new custom check while validating a plug-in? Or, a "quick grep" to exclude issues in those ones already published in the Directory? I know I'm asking for a great effort here, apologies if this is far behind the scope of this issue (devs could ignore Moodle practices... sometime ).

              TIA,
              Matteo

              Show
              matteo Matteo Scaramuccia added a comment - OK, point taken but please let (dev only?) people understand that third party plug-ins could instantiate PHPMailer by their own without passing through the Moodle Mail way (API we could say... indeed the higher API exists: https://docs.moodle.org/dev/Message_API ). Time for a new custom check while validating a plug-in? Or, a "quick grep" to exclude issues in those ones already published in the Directory? I know I'm asking for a great effort here, apologies if this is far behind the scope of this issue (devs could ignore Moodle practices... sometime ). TIA, Matteo
              Hide
              mudrd8mz David Mudrák added a comment -

              We have another duplicate report for this. I believe it will be better to disclose this issue to the public.

              Show
              mudrd8mz David Mudrák added a comment - We have another duplicate report for this. I believe it will be better to disclose this issue to the public.
              Hide
              mudrd8mz David Mudrák added a comment -

              The exploit has been published at https://github.com/opsxcq/exploit-CVE-2016-10033

              As commented above, the attacker would have to have access to modifying the Sender property. For emails sent via the email_to_user() function, Moodle sets the property explicitly. So the default fallback to using the From field should not apply (for injecting the shell commands via malicious firstname / lastname).

              Additionally, the exploit seems to require that PHP mail() function is used for sending the email. For Moodle it means that only sites having the $CFG->smtphosts empty could be potentially affected.

              Still, I believe that Moodle is not affected. But it should not hurt if we patched all versions with something like

              diff --git a/lib/phpmailer/class.phpmailer.php b/lib/phpmailer/class.phpmailer.php
              index f9013ebb17..09e4dd17a0 100644
              --- a/lib/phpmailer/class.phpmailer.php
              +++ b/lib/phpmailer/class.phpmailer.php
              @@ -1430,7 +1430,7 @@ class PHPMailer
                       $params = null;
                       //This sets the SMTP envelope sender which gets turned into a return-path header by the receiver
                       if (!empty($this->Sender)) {
              -            $params = sprintf('-f%s', $this->Sender);
              +            $params = sprintf('-f%s', escapeshellarg($this->Sender));
                       }
                       if ($this->Sender != '' and !ini_get('safe_mode')) {
                           $old_from = ini_get('sendmail_from');
              

              and upgrade to latest 5.2.x on master and 3.2 branches only (to avoid eventual regressions on stable branches).

              Show
              mudrd8mz David Mudrák added a comment - The exploit has been published at https://github.com/opsxcq/exploit-CVE-2016-10033 As commented above, the attacker would have to have access to modifying the Sender property. For emails sent via the email_to_user() function, Moodle sets the property explicitly. So the default fallback to using the From field should not apply (for injecting the shell commands via malicious firstname / lastname). Additionally, the exploit seems to require that PHP mail() function is used for sending the email. For Moodle it means that only sites having the $CFG->smtphosts empty could be potentially affected. Still, I believe that Moodle is not affected. But it should not hurt if we patched all versions with something like diff --git a/lib/phpmailer/class.phpmailer.php b/lib/phpmailer/class.phpmailer.php index f9013ebb17..09e4dd17a0 100644 --- a/lib/phpmailer/class.phpmailer.php +++ b/lib/phpmailer/class.phpmailer.php @@ -1430,7 +1430,7 @@ class PHPMailer $params = null; //This sets the SMTP envelope sender which gets turned into a return-path header by the receiver if (!empty($this->Sender)) { - $params = sprintf('-f%s', $this->Sender); + $params = sprintf('-f%s', escapeshellarg($this->Sender)); } if ($this->Sender != '' and !ini_get('safe_mode')) { $old_from = ini_get('sendmail_from'); and upgrade to latest 5.2.x on master and 3.2 branches only (to avoid eventual regressions on stable branches).
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi David,
              that could be a good compromise, eventually agreed by most people!
              The main advantages with that code change is the "external" validation done in Moodle and the shell escaping to keep things safe in the library - note that the validation done inside the library via validateAddress() could be weak compared to the PHP and the PCRE versions used.

              Just a note: the master is now 3.3, if you don't want to update the stables .

              TIA,
              Matteo

              Show
              matteo Matteo Scaramuccia added a comment - Hi David, that could be a good compromise, eventually agreed by most people! The main advantages with that code change is the "external" validation done in Moodle and the shell escaping to keep things safe in the library - note that the validation done inside the library via validateAddress() could be weak compared to the PHP and the PCRE versions used. Just a note: the master is now 3.3 , if you don't want to update the stables . TIA, Matteo
              Hide
              james.mclean James McLean added a comment -

              Of note, the fixed version of PHPMailer, 5.2.18, is vulnerable to a further RCE attack. PHPMailer 5.2.20 resolves this.

              Full details at https://legalhackers.com/advisories/PHPMailer-Exploit-Remote-Code-Exec-CVE-2016-10045-Vuln-Patch-Bypass.html

              Show
              james.mclean James McLean added a comment - Of note, the fixed version of PHPMailer, 5.2.18, is vulnerable to a further RCE attack. PHPMailer 5.2.20 resolves this. Full details at https://legalhackers.com/advisories/PHPMailer-Exploit-Remote-Code-Exec-CVE-2016-10045-Vuln-Patch-Bypass.html
              Hide
              poltawski Dan Poltawski added a comment -

              (Haven't looked into the issue at all, but +1 to not hiding the details here if its already been publicly disclosed and note another duplicate with patch MDL-57539 )

              Show
              poltawski Dan Poltawski added a comment - (Haven't looked into the issue at all, but +1 to not hiding the details here if its already been publicly disclosed and note another duplicate with patch MDL-57539 )
              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks James for the update. I checked the report and the new one still relies on the same weak point of the Sender field sanitization. Moodle should be still ok IMHO.

              Show
              mudrd8mz David Mudrák added a comment - Thanks James for the update. I checked the report and the new one still relies on the same weak point of the Sender field sanitization. Moodle should be still ok IMHO.
              Hide
              danmarsden Dan Marsden added a comment -

              $CFG->noreplyaddress looks vulnerable to me (PARAM_NOTAGS)

              I think it might also be a good idea to add a validate_email() call on the sender email address within email_to_user()

              Show
              danmarsden Dan Marsden added a comment - $CFG->noreplyaddress looks vulnerable to me (PARAM_NOTAGS) I think it might also be a good idea to add a validate_email() call on the sender email address within email_to_user()
              Hide
              matteo Matteo Scaramuccia added a comment -

              Dan Marsden great investigation work!
              The validation happens in admin/process_email.php but not efficiently in https://github.com/moodle/moodle/blob/7eb34671c13b07ae62c8391ade4777361f4384b7/admin/settings/server.php#L240 and for other email fields too e.g. supportemail.

              IMHO shell escaping will eventually fix those missings, even if it would be nice to cleanup them too.

              Show
              matteo Matteo Scaramuccia added a comment - Dan Marsden great investigation work! The validation happens in admin/process_email.php but not efficiently in https://github.com/moodle/moodle/blob/7eb34671c13b07ae62c8391ade4777361f4384b7/admin/settings/server.php#L240 and for other email fields too e.g. supportemail . IMHO shell escaping will eventually fix those missings, even if it would be nice to cleanup them too.
              Hide
              mudrd8mz David Mudrák added a comment -

              Let's put it clear. What Dan says is that Moodle is vulnerable via a setting configurable by the admin only. So yes, validation for these fields will be good to have (to minimise the attacker's chances to execute code remotely on the server if they find a way to inject a value into this CFG). But I believe the main message is still "don't panic".

              I am officially on holiday until the next week and I won't be able to work on this until then. To repeat and summarise what was analysed so far is that the phpmailer's $Sender field in Moodle is explicitly set by the Moodle core based on values defined by the admin.

              Show
              mudrd8mz David Mudrák added a comment - Let's put it clear. What Dan says is that Moodle is vulnerable via a setting configurable by the admin only. So yes, validation for these fields will be good to have (to minimise the attacker's chances to execute code remotely on the server if they find a way to inject a value into this CFG). But I believe the main message is still "don't panic". I am officially on holiday until the next week and I won't be able to work on this until then. To repeat and summarise what was analysed so far is that the phpmailer's $Sender field in Moodle is explicitly set by the Moodle core based on values defined by the admin.
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi David,
              your effort is actually appreciated and I second your Don't panic statement .

              BTW PHPMailer is still evolving since 5.2.18 has introduced another vulnerability, as said above, and they're working on releasing 5.2.20 which improves the way they were shell escaping in 5.2.18.

              Let's wait,
              Matteo

              Show
              matteo Matteo Scaramuccia added a comment - Hi David, your effort is actually appreciated and I second your Don't panic statement . BTW PHPMailer is still evolving since 5.2.18 has introduced another vulnerability, as said above, and they're working on releasing 5.2.20 which improves the way they were shell escaping in 5.2.18. Let's wait, Matteo
              Hide
              danmarsden Dan Marsden added a comment -

              yes - this is more of a problem for organisations that host multiple sites on a single host - (eg partners) -in these cases admin users should not be able to execute code and potnetially affect/gain access to other sites on the same server.

              until this is fixed you can do the following things to mitigate:
              force the use of smtp server
              force the noreply address by setting it in config.php

              I haven't tried to exploit this using noreply - it just looks possible after reading through the code.

              I would vote to upgrade phpmailer and add better validation to the sender within our email to user function - we already validate the recipient e-mail but for added safety it would be good to explicitly validate the sender as well.

              Show
              danmarsden Dan Marsden added a comment - yes - this is more of a problem for organisations that host multiple sites on a single host - (eg partners) -in these cases admin users should not be able to execute code and potnetially affect/gain access to other sites on the same server. until this is fixed you can do the following things to mitigate: force the use of smtp server force the noreply address by setting it in config.php I haven't tried to exploit this using noreply - it just looks possible after reading through the code. I would vote to upgrade phpmailer and add better validation to the sender within our email to user function - we already validate the recipient e-mail but for added safety it would be good to explicitly validate the sender as well.
              Hide
              matteo Matteo Scaramuccia added a comment - - edited

              FYI
              Same issue in Mahara: https://bugs.launchpad.net/mahara/+bug/1652995, they released the supported branches with the updated PHPMailer (now: 5.2.21).

              PHPMailer has disclosed the circumstances of the vulnerability (CVE-2016-10033) and the reasons why 5.2.18 added a new 0-day vulnerability (CVE-2016-10045): https://github.com/PHPMailer/PHPMailer/wiki/About-the-CVE-2016-10033-and-CVE-2016-10045-vulnerabilities.

              Side effect of adopting 5.2.21+:

              Possible side effect - complex sender addresses (such as those used in VERP addressing) may no longer work. We advise switching to the SMTP transport if you need that functionality, and it offers higher performance anyway.

              HTH,
              Matteo

              Show
              matteo Matteo Scaramuccia added a comment - - edited FYI Same issue in Mahara: https://bugs.launchpad.net/mahara/+bug/1652995 , they released the supported branches with the updated PHPMailer (now: 5.2.21). PHPMailer has disclosed the circumstances of the vulnerability (CVE-2016-10033) and the reasons why 5.2.18 added a new 0-day vulnerability (CVE-2016-10045): https://github.com/PHPMailer/PHPMailer/wiki/About-the-CVE-2016-10033-and-CVE-2016-10045-vulnerabilities . Side effect of adopting 5.2.21+: Possible side effect - complex sender addresses (such as those used in VERP addressing) may no longer work. We advise switching to the SMTP transport if you need that functionality, and it offers higher performance anyway. HTH, Matteo
              Hide
              peterbulmer Peter Bulmer added a comment -

              Yes, this is a known security issue, but this bug discusses the exact way in which Moodle is vulnerable, and where.

              +1 for security issue.

              Show
              peterbulmer Peter Bulmer added a comment - Yes, this is a known security issue, but this bug discusses the exact way in which Moodle is vulnerable, and where. +1 for security issue.
              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks guys for the comments. And especially thanks Dan for spotting the missing validation of the sender address. I have looked at the latest development of the upstream issue.

              There will be consequences of upgrading PHPMailer to latest 5.2.21 and things should be done carefully and after thinking all consequences.

              Firstly, as discussed above, there were regressions of PHPMailer upgrades. If we, for example, wanted to upgrade the PHPMailer on Moodle 3.0 branch from 5.2.13, we would need to also backport all the related regression fixes that were done in 3.1 (PHPMailer 5.2.14) and 3.2 (5.2.16). The most risky (in terms of stability) would be the change in the 2.7 LTS where PHPMailer 5.2.7 is used. We could re-introduce some regressions again.

              Secondly, also mentioned above. The way how PHPMailer fixed the RCE vulnerability in 5.2.20, has significant impact on the functionality that Moodle may make use of. Latest PHPMailer, when operating via sendmail, no longer supports extended characters in the sender's email address - see the isShellSafe() method introduced in 5.2.20. This includes equal and plus
              signs used typically for VERP. As per https://github.com/PHPMailer/PHPMailer/issues/944 this behaviour is considered as a feature. The only allowed non-alphanum chars in emails are _-. (and even those may be dangerous in some shells).

              In other words, sites having $CFG->handlebounces enabled and $CFG->smtphosts empty are no longer compatible with the latest PHPMailer 5.2.21. Before the upgrade, they should be reconfigured to use direct SMTP
              connection.

              It must be said that handlebounces is an advanced feature that also requires specific setup of the SMTP server - https://docs.moodle.org/dev/Email_processing

              With regards to noreplyaddress (and generally sender's address) validation. I believe we should change the current PARAM_NOTAGS to the proper PARAM_EMAIL:

              diff --git a/admin/settings/server.php b/admin/settings/server.php
              index 3c56ff19e1..f8b12e546c 100644
              --- a/admin/settings/server.php
              +++ b/admin/settings/server.php
              @@ -237,7 +237,7 @@ $temp->add(new admin_setting_configtext('smtpmaxbulk', new lang_string('smtpmaxb
               $temp->add(new admin_setting_heading('noreplydomainheading', new lang_string('noreplydomain', 'admin'),
                        new lang_string('noreplydomaindetail', 'admin')));
                $temp->add(new admin_setting_configtext('noreplyaddress', new lang_string('noreplyaddress', 'admin'),
                -          new lang_string('confignoreplyaddress', 'admin'), 'noreply@' . get_host_from_url($CFG->wwwroot), PARAM_NOTAGS));
                +          new lang_string('confignoreplyaddress', 'admin'), 'noreply@' . get_host_from_url($CFG->wwwroot), PARAM_EMAIL));
               $temp->add(new admin_setting_configtextarea('allowedemaildomains',
                        new lang_string('allowedemaildomains', 'admin'),
                                 new lang_string('configallowedemaildomains', 'admin'),
              

              This is likely to affect some (if not most) developer and test machines as
              values like noreply@localhost would be no longer considered as valid ones.

              Additionally, the validate_email() should be used in email_to_user() to
              make sure that the Sender is set to a valid value.

              Good news is that our email validation function seems to protect us from some malicious attempts to abuse escaping issues in unpatched PHPMailer issues, at least for addresses published as examples illustrating the vulnaribility:

              validate_email('"attacker\\" -oQ/tmp/ -X/var/www/vhost/moodle/backdoor.php  some"@email.com')
              

              returns false. But there might be other ways to bypass it.

              Just note that emails like

              our.school+noreply@gmail.com
              

              would be considered still valid by Moodle, but the new PHPMailer would not send them via sendmail.

              To summarise, here are the individual actions we can do:

              1. Change the type of noreplyaddress to PARAM_EMAIL. It should not affect production sites that are supposed to have reasonable values there.
              2. Validate sender's email in email_to_user().
              3. Upgrade PHPMailer in Moodle 3.3dev and 3.2
              4. Upgrade PHPMailer in 3.1, 3.0 and 2.7

              I am going to have testable patches for 1. and 2. tonight. Maybe we could even get them included in the next minor releases.

              The patches for 3. should not be hard to prepare as there seem to be no other relevant changes in PHPMailer recent versions.

              The patches for 4. would deserve a bit of time to make sure the risk of regression is minimised.

              Show
              mudrd8mz David Mudrák added a comment - Thanks guys for the comments. And especially thanks Dan for spotting the missing validation of the sender address. I have looked at the latest development of the upstream issue. There will be consequences of upgrading PHPMailer to latest 5.2.21 and things should be done carefully and after thinking all consequences. Firstly, as discussed above, there were regressions of PHPMailer upgrades. If we, for example, wanted to upgrade the PHPMailer on Moodle 3.0 branch from 5.2.13, we would need to also backport all the related regression fixes that were done in 3.1 (PHPMailer 5.2.14) and 3.2 (5.2.16). The most risky (in terms of stability) would be the change in the 2.7 LTS where PHPMailer 5.2.7 is used. We could re-introduce some regressions again. Secondly, also mentioned above. The way how PHPMailer fixed the RCE vulnerability in 5.2.20, has significant impact on the functionality that Moodle may make use of. Latest PHPMailer, when operating via sendmail, no longer supports extended characters in the sender's email address - see the isShellSafe() method introduced in 5.2.20. This includes equal and plus signs used typically for VERP. As per https://github.com/PHPMailer/PHPMailer/issues/944 this behaviour is considered as a feature. The only allowed non-alphanum chars in emails are _-. (and even those may be dangerous in some shells). In other words, sites having $CFG->handlebounces enabled and $CFG->smtphosts empty are no longer compatible with the latest PHPMailer 5.2.21. Before the upgrade, they should be reconfigured to use direct SMTP connection. It must be said that handlebounces is an advanced feature that also requires specific setup of the SMTP server - https://docs.moodle.org/dev/Email_processing With regards to noreplyaddress (and generally sender's address) validation. I believe we should change the current PARAM_NOTAGS to the proper PARAM_EMAIL: diff --git a/admin/settings/server.php b/admin/settings/server.php index 3c56ff19e1..f8b12e546c 100644 --- a/admin/settings/server.php +++ b/admin/settings/server.php @@ -237,7 +237,7 @@ $temp->add(new admin_setting_configtext('smtpmaxbulk', new lang_string('smtpmaxb $temp->add(new admin_setting_heading('noreplydomainheading', new lang_string('noreplydomain', 'admin'), new lang_string('noreplydomaindetail', 'admin'))); $temp->add(new admin_setting_configtext('noreplyaddress', new lang_string('noreplyaddress', 'admin'), - new lang_string('confignoreplyaddress', 'admin'), 'noreply@' . get_host_from_url($CFG->wwwroot), PARAM_NOTAGS)); + new lang_string('confignoreplyaddress', 'admin'), 'noreply@' . get_host_from_url($CFG->wwwroot), PARAM_EMAIL)); $temp->add(new admin_setting_configtextarea('allowedemaildomains', new lang_string('allowedemaildomains', 'admin'), new lang_string('configallowedemaildomains', 'admin'), This is likely to affect some (if not most) developer and test machines as values like noreply@localhost would be no longer considered as valid ones. Additionally, the validate_email() should be used in email_to_user() to make sure that the Sender is set to a valid value. Good news is that our email validation function seems to protect us from some malicious attempts to abuse escaping issues in unpatched PHPMailer issues, at least for addresses published as examples illustrating the vulnaribility: validate_email('"attacker\\" -oQ/tmp/ -X/var/www/vhost/moodle/backdoor.php some"@email.com') returns false. But there might be other ways to bypass it. Just note that emails like our.school+noreply@gmail.com would be considered still valid by Moodle, but the new PHPMailer would not send them via sendmail. To summarise, here are the individual actions we can do: 1. Change the type of noreplyaddress to PARAM_EMAIL. It should not affect production sites that are supposed to have reasonable values there. 2. Validate sender's email in email_to_user(). 3. Upgrade PHPMailer in Moodle 3.3dev and 3.2 4. Upgrade PHPMailer in 3.1, 3.0 and 2.7 I am going to have testable patches for 1. and 2. tonight. Maybe we could even get them included in the next minor releases. The patches for 3. should not be hard to prepare as there seem to be no other relevant changes in PHPMailer recent versions. The patches for 4. would deserve a bit of time to make sure the risk of regression is minimised.
              Hide
              mudrd8mz David Mudrák added a comment -

              I forgot to mention that the phpmailer limitation introduced in 5.2.20 should not affect the inbound messaging (known as "reply to forum via email") as the new isShellSafe() method is used for checking the sender (From) field only. So even if that mechanism uses VERP too, there should be no impact on sites using it.

              Show
              mudrd8mz David Mudrák added a comment - I forgot to mention that the phpmailer limitation introduced in 5.2.20 should not affect the inbound messaging (known as "reply to forum via email") as the new isShellSafe() method is used for checking the sender (From) field only. So even if that mechanism uses VERP too, there should be no impact on sites using it.
              Hide
              mudrd8mz David Mudrák added a comment -

              I was wondering if noreplyaddress had been defined as PARAM_NOTAGS for some reason. But it turned out that was done back in 2006 (1ea1bcb3), whereas PARAM_EMAIL was first introduced in 2010 (79f1d9538a8). So I think it is a valid fix.

              I submitted a branch for peer-review containing the validation of the sender's email.

              Show
              mudrd8mz David Mudrák added a comment - I was wondering if noreplyaddress had been defined as PARAM_NOTAGS for some reason. But it turned out that was done back in 2006 (1ea1bcb3), whereas PARAM_EMAIL was first introduced in 2010 (79f1d9538a8). So I think it is a valid fix. I submitted a branch for peer-review containing the validation of the sender's email.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi David - shouldn't it fall-back to a reasonable no-reply address like:
              https://github.com/mudrd8mz/moodle/blob/3cb1abdbbd196814cc1463e7c0b36f5ae68f2ef8/lib/moodlelib.php#L5769

              rather than return false? - causing all email to fail if an invalid no-reply is set?

              makes sense to tackle 1/2 for the release planned next week (delaying the release if necessary) - and then tacking the upgrade of phpmailer as a separate issue. Particularly for 3.1 if possible as it's support life-cycle is a lot longer and we're likely to have this crop up in external security audits. with the patch for 1/2 in place we may be able to defer the patch for 2.7 to avoid regressions as the patch for 1/2 mitigates the problem and it's only supported for another 6 months.

              Show
              danmarsden Dan Marsden added a comment - Hi David - shouldn't it fall-back to a reasonable no-reply address like: https://github.com/mudrd8mz/moodle/blob/3cb1abdbbd196814cc1463e7c0b36f5ae68f2ef8/lib/moodlelib.php#L5769 rather than return false? - causing all email to fail if an invalid no-reply is set? makes sense to tackle 1/2 for the release planned next week (delaying the release if necessary) - and then tacking the upgrade of phpmailer as a separate issue. Particularly for 3.1 if possible as it's support life-cycle is a lot longer and we're likely to have this crop up in external security audits. with the patch for 1/2 in place we may be able to defer the patch for 2.7 to avoid regressions as the patch for 1/2 mitigates the problem and it's only supported for another 6 months.
              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks Dan for a good suggestion to fall back to the implicit no-replay address in case the explicit one is not valid. I amended the patch to do that. I let the function still fail if the user's email is not valid though (consistently with the recipient's email handling).

              Based on our jabber chat I like and agree with the proposal to:

              1. Include this validation patch in the upcoming minor release in all supported versions.
              2. Upgrade phpmailer in 3.2 and 3.1 in a follow-up issue.
              3. Leave the phpmailer as it is in 3.0 and 2.7, having just the validation patch in place.

              I am off to bed now. I'll be happy to finish the testing instructions tomorrow morning. It will help if you find a minute to check the second revision of the patch. Thanks a lot!

              Show
              mudrd8mz David Mudrák added a comment - Thanks Dan for a good suggestion to fall back to the implicit no-replay address in case the explicit one is not valid. I amended the patch to do that. I let the function still fail if the user's email is not valid though (consistently with the recipient's email handling). Based on our jabber chat I like and agree with the proposal to: 1. Include this validation patch in the upcoming minor release in all supported versions. 2. Upgrade phpmailer in 3.2 and 3.1 in a follow-up issue. 3. Leave the phpmailer as it is in 3.0 and 2.7, having just the validation patch in place. I am off to bed now. I'll be happy to finish the testing instructions tomorrow morning. It will help if you find a minute to check the second revision of the patch. Thanks a lot!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              I'm worried about backporting "too much". Maybe also to 30 and 31. Note that we'll need to "drag" all the regression fixes that posterior branches had:

              Just sharing the issues, so the proposed 1-2-3 by David (that I basically agree) is matched against that, and regressions are taken into account.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited I'm worried about backporting "too much". Maybe also to 30 and 31. Note that we'll need to "drag" all the regression fixes that posterior branches had: 3.2: MDL-56000 (1 regression so far: MDL-57474 ) 3.1: MDL-53465 (no regressions) Own new regressions introduced by the new versions. Just sharing the issues, so the proposed 1-2-3 by David (that I basically agree) is matched against that, and regressions are taken into account.
              Hide
              danmarsden Dan Marsden added a comment -

              I don't think we can avoid backporting the phpmailer lib update to 3.1 I do think we can probably avoid 2.7 once we implement David's patch (support for 2.7 ends in 5 months and we should really avoid regressions) but there is potential for 3rd party plugins to use phpmailer directly and it will most definitely come up in any security audits performed on the code by 3rd parties.

              but - backporting the phpmailer lib can come during our normal development cycle and doesn't need to pushed through urgently once David's patch is in place.

              Show
              danmarsden Dan Marsden added a comment - I don't think we can avoid backporting the phpmailer lib update to 3.1 I do think we can probably avoid 2.7 once we implement David's patch (support for 2.7 ends in 5 months and we should really avoid regressions) but there is potential for 3rd party plugins to use phpmailer directly and it will most definitely come up in any security audits performed on the code by 3rd parties. but - backporting the phpmailer lib can come during our normal development cycle and doesn't need to pushed through urgently once David's patch is in place.
              Hide
              danmarsden Dan Marsden added a comment -

              I've added some testing instructions and looked through the code - pushing up for integration review to speed this along. I'm going to do some further extended testing myself too.

              Show
              danmarsden Dan Marsden added a comment - I've added some testing instructions and looked through the code - pushing up for integration review to speed this along. I'm going to do some further extended testing myself too.
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-57531 using repository: git://github.com/mudrd8mz/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-57531 using repository: git://github.com/mudrd8mz/moodle.git master (0 errors / 0 warnings) [branch: MDL-57531-master-phpmailer | CI Job ] More information about this report
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Just to clarify it. I agree with the 1-2-3 plan from David above, with this issue ensuring we are validating in all target branches and done.

              Just wanted to make a call about the upgrade of phpmailer to stables to require special attention, because at least 1 regression in 3.2 happened (MDL-57474, being integrated exactly this week) and we'll need to apply it to the branches where we upgrade phpmailer (agree 31, 32 and master are the "best" targets, older ones are near falling out of support and site upgrade will be needed).

              So, kudos to current validation-only approach in this issue + followup to sort out the phpmailer upgrade (with known regressions).

              Thanks, Dan, David et all!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Just to clarify it. I agree with the 1-2-3 plan from David above, with this issue ensuring we are validating in all target branches and done. Just wanted to make a call about the upgrade of phpmailer to stables to require special attention, because at least 1 regression in 3.2 happened ( MDL-57474 , being integrated exactly this week) and we'll need to apply it to the branches where we upgrade phpmailer (agree 31, 32 and master are the "best" targets, older ones are near falling out of support and site upgrade will be needed). So, kudos to current validation-only approach in this issue + followup to sort out the phpmailer upgrade (with known regressions). Thanks, Dan, David et all!
              Hide
              poltawski Dan Poltawski added a comment -

              Some small comments about the patch:

              1. I agree with the approach, thanks.
              2. I'm not totaly convinced by the validation in $CFG->handlebounces branch. If there is a problem there I think it should be handled earlier. Belt and braces isn't always a good thing because it muddies the water of where to do the cleaning. I suppose this is covered by your 'shouldnt really happen' comment
              3. That the unit test is doing assertEquals exposes that validate_email is not returning a boolean like its phpdocs suggest. I would be tempted to make the function perform as advertised (return a bool) and do assertTrue in the test. Just because sometimes weak 'truthyness' can bite us
              4. Worth noting that you can't really use PARAM_EMAIL in formslib because it gives a terrible user experience (returns blank content) and you have to roll your own validation. Thankfully not the case with adminlib
              Show
              poltawski Dan Poltawski added a comment - Some small comments about the patch: I agree with the approach, thanks. I'm not totaly convinced by the validation in $CFG->handlebounces branch. If there is a problem there I think it should be handled earlier. Belt and braces isn't always a good thing because it muddies the water of where to do the cleaning. I suppose this is covered by your 'shouldnt really happen' comment That the unit test is doing assertEquals exposes that validate_email is not returning a boolean like its phpdocs suggest. I would be tempted to make the function perform as advertised (return a bool) and do assertTrue in the test. Just because sometimes weak 'truthyness' can bite us Worth noting that you can't really use PARAM_EMAIL in formslib because it gives a terrible user experience (returns blank content) and you have to roll your own validation. Thankfully not the case with adminlib
              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks Eloy Lafuente (stronk7) and Dan Poltawski.

              re 2. validation in $CFG->handlebounces branch, I see your point and I agree. I am happy to remove the check from the code and instead introduce a unit test for the generate_email_processing_address() to make sure it returns a value that passes the validation.

              re 3. validate function truthyness - as you can imagine, I naively / intuitively started the unit tests with assertTrue() and was quite surprised it did not pass (as it returns 1, not boolean). I just did not want to change too much in this patch. But yes again, I am happy to amend the patch and make the function return proper bool as expected (shall we do it on stable branches too?)

              Show
              mudrd8mz David Mudrák added a comment - Thanks Eloy Lafuente (stronk7) and Dan Poltawski . re 2. validation in $CFG->handlebounces branch, I see your point and I agree. I am happy to remove the check from the code and instead introduce a unit test for the generate_email_processing_address() to make sure it returns a value that passes the validation. re 3. validate function truthyness - as you can imagine, I naively / intuitively started the unit tests with assertTrue() and was quite surprised it did not pass (as it returns 1, not boolean). I just did not want to change too much in this patch. But yes again, I am happy to amend the patch and make the function return proper bool as expected (shall we do it on stable branches too?)
              Hide
              mudrd8mz David Mudrák added a comment -
              • Validation of the generated bouncing email address delegated to the unit test.
              • Added PARAM_EMAIL validation for the support email, too (and not only for the noreplyaddress)
              • Master only: Fixed the type of validate_email() to be actual boolean
              • Patchsets for 3.3 and 3.2 are identical and applied cleanly.
              • Patchset for 3.1 had to be manually rewritten as the email_to_user() and admin settings location are different there.
              • Patchset for 2.7 is same as the one for 3.1, the only merge conflict was in the unit test.

              Dan Poltawski, sorry for the delay, this is ready to be reviewed now. Thanks.

              Show
              mudrd8mz David Mudrák added a comment - Validation of the generated bouncing email address delegated to the unit test. Added PARAM_EMAIL validation for the support email, too (and not only for the noreplyaddress) Master only: Fixed the type of validate_email() to be actual boolean Patchsets for 3.3 and 3.2 are identical and applied cleanly. Patchset for 3.1 had to be manually rewritten as the email_to_user() and admin settings location are different there. Patchset for 2.7 is same as the one for 3.1, the only merge conflict was in the unit test. Dan Poltawski , sorry for the delay, this is ready to be reviewed now. Thanks.
              Hide
              mudrd8mz David Mudrák added a comment -

              Patchsets for 3.3 and 3.2 are identical and applied cleanly.

              To be clear, the 3.3 (master) patchset contains that extra commit. But otherwise they are identical.

              Show
              mudrd8mz David Mudrák added a comment - Patchsets for 3.3 and 3.2 are identical and applied cleanly. To be clear, the 3.3 (master) patchset contains that extra commit. But otherwise they are identical.
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-57531 using repository: git://github.com/mudrd8mz/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-57531 using repository: git://github.com/mudrd8mz/moodle.git MOODLE_27_STABLE (3 errors / 0 warnings) [branch: MDL-57531-27-phpmailer | CI Job ] phplint (0/0) , phpcs (3/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , travis (0/0) , mustache (0/0) , Warn: The MDL-57531 -27-phpmailer branch at git://github.com/mudrd8mz/moodle.git has not been rebased recently (>20 days ago). MOODLE_31_STABLE (3 errors / 0 warnings) [branch: MDL-57531-31-phpmailer | CI Job ] phplint (0/0) , phpcs (3/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , MOODLE_32_STABLE (0 errors / 0 warnings) [branch: MDL-57531-32-phpmailer | CI Job ] master (0 errors / 1 warnings) [branch: MDL-57531-master-phpmailer | CI Job ] phplint (0/0) , phpcs (0/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks David, you missed the 30 branch but I sorted that out for you myself (another unit test conflict).

              Integrated to master, 32, 31, 30, 27

              Show
              poltawski Dan Poltawski added a comment - Thanks David, you missed the 30 branch but I sorted that out for you myself (another unit test conflict). Integrated to master, 32, 31, 30, 27
              Hide
              mudrd8mz David Mudrák added a comment -

              you missed the 30 branch but I sorted that out for you myself

              D'oh! Sorry and thanks. I owe you one.

              Show
              mudrd8mz David Mudrák added a comment - you missed the 30 branch but I sorted that out for you myself D'oh! Sorry and thanks. I owe you one.
              Hide
              poltawski Dan Poltawski added a comment -

              2. Upgrade phpmailer in 3.2 and 3.1 in a follow-up issue.

              Created MDL-57573 for that.

              Show
              poltawski Dan Poltawski added a comment - 2. Upgrade phpmailer in 3.2 and 3.1 in a follow-up issue. Created MDL-57573 for that.
              Hide
              poltawski Dan Poltawski added a comment -

              Just to note that from my testing this seems to be working ok

              Show
              poltawski Dan Poltawski added a comment - Just to note that from my testing this seems to be working ok
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              master and 32_STABLE working ok here, with the following observations:

              1) allowed domains must be set to "moodle.org", not to "*.moodle.org" (testing instructions), for allowing xxxx@moodle.org addresses to work.
              2) I was 100% unable to set a invalid address that at the same time was being considered allowed domain, I tried the suggested moodle@moodle.org>\r\nRCPT TO:<victim@example.com and others and all them were not matching allowed domains, so noreply adress was being used. So I had to hack can_send_from_real_email_address() to return true in order to allow validation to happen. That way it worked (debugging + NO mail sent).

              Hope 1), and specially 2) are not a problem. Going to test 27, 30 & 31 now...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited master and 32_STABLE working ok here, with the following observations: 1) allowed domains must be set to "moodle.org", not to "*.moodle.org" (testing instructions), for allowing xxxx@moodle.org addresses to work. 2) I was 100% unable to set a invalid address that at the same time was being considered allowed domain, I tried the suggested moodle@moodle.org>\r\nRCPT TO:<victim@example.com and others and all them were not matching allowed domains, so noreply adress was being used. So I had to hack can_send_from_real_email_address() to return true in order to allow validation to happen. That way it worked (debugging + NO mail sent). Hope 1), and specially 2) are not a problem. Going to test 27, 30 & 31 now...
              Hide
              danmarsden Dan Marsden added a comment -

              testing instructions were mine sorry - *.moodle.org probably expects @student.moodle.org, @staff.moodle.org etc.

              Show
              danmarsden Dan Marsden added a comment - testing instructions were mine sorry - *.moodle.org probably expects @student.moodle.org, @staff.moodle.org etc.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              31 and 30 stable working ok, also requiring some hacking to test when user has bad email, but all right, so far.

              Plus "support" and "replyto" cases tested manually by invoking email_to_user() in custom script.

              Heading to 27_STABLE...

              NP, Dan!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - 31 and 30 stable working ok, also requiring some hacking to test when user has bad email, but all right, so far. Plus "support" and "replyto" cases tested manually by invoking email_to_user() in custom script. Heading to 27_STABLE... NP, Dan!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              And 27 here behaving like 30 & 31, using same scripts. Just I had to use another server there because of the lack of "SMTP Auth Type" in 27_STABLE and that server always sets the from in the mails to the smtp login (email address).

              So, summary:

              • All was ok, with noreply/support/user email addresses being validated ok, in forms and later when sending mails.
              • All debugging is matching expectations.
              • When a validation problem happens, the mail is changed to the (correct) noreplyaddress one and mail is sent.
              • When a user mail (from) has validation problems, no mail is sent.

              Then, to be 100% sure with 27_STABLE... would be great to know if anybody else is getting the noreplyaddress as expected there. As commented, in the server I was able to test 27_STABLE, there is some rule overwriting unconditionally the from addresses. If nobody can verify it.. I can try tomorrow with some other server / tool not having such a rule.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited And 27 here behaving like 30 & 31, using same scripts. Just I had to use another server there because of the lack of "SMTP Auth Type" in 27_STABLE and that server always sets the from in the mails to the smtp login (email address). So, summary: All was ok, with noreply/support/user email addresses being validated ok, in forms and later when sending mails. All debugging is matching expectations. When a validation problem happens, the mail is changed to the (correct) noreplyaddress one and mail is sent. When a user mail (from) has validation problems, no mail is sent. Then, to be 100% sure with 27_STABLE... would be great to know if anybody else is getting the noreplyaddress as expected there. As commented, in the server I was able to test 27_STABLE, there is some rule overwriting unconditionally the from addresses. If nobody can verify it.. I can try tomorrow with some other server / tool not having such a rule. Ciao
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              If site is set to use localhost then value for noreplyaddress is noreply@localhost, which is invalid. Installing site via cli will not report this and you won't see this problem till you navigate to site notifications page.

              Also, this will cause behat failure if something like $CFG->behat_wwwroot = 'http://localhost/im'; One of the failures is:

              001 Scenario: Submit a text online and edit the submission # /home/rajesh/moodles/im/moodle/mod/assign/tests/behat/allow_another_attempt.feature:8
                    And I press "Save changes"                           # /home/rajesh/moodles/im/moodle/mod/assign/tests/behat/allow_another_attempt.feature:36
                      Moodle exception: Exception - Notice: Undefined property: stdClass::$noreplyaddress in [dirroot]/lib/classes/user.php on line 135 More information about this error
                      
                          
                      ×
                          
                      Debug info:
                       
                      Error code: generalexceptionmessage
                      
                      
                          
                      ×
                          
                      Stack trace:
                       
                      line 158 of /lib/behat/lib.php: Exception thrown
                      line 135 of /lib/classes/user.php: call to behat_error_handler()
                      line 174 of /lib/classes/user.php: call to core_user::get_dummy_user_record()
                      line 5831 of /mod/assign/locallib.php: call to core_user::get_noreply_user()
                      line 6827 of /mod/assign/locallib.php: call to assign->notify_student_submission_receipt()
                      line 6864 of /mod/assign/locallib.php: call to assign->save_submission()
                      line 437 of /mod/assign/locallib.php: call to assign->process_save_submission()
                      line 55 of /mod/assign/view.php: call to assign->view()
                       (Exception)
              

              Failing this issue, so we can set proper email for invalid domains or atleast for behat atleast.

              Show
              rajeshtaneja Rajesh Taneja added a comment - If site is set to use localhost then value for noreplyaddress is noreply@localhost, which is invalid. Installing site via cli will not report this and you won't see this problem till you navigate to site notifications page. Also, this will cause behat failure if something like $CFG->behat_wwwroot = 'http://localhost/im'; One of the failures is: 001 Scenario: Submit a text online and edit the submission # /home/rajesh/moodles/im/moodle/mod/assign/tests/behat/allow_another_attempt.feature:8 And I press "Save changes" # /home/rajesh/moodles/im/moodle/mod/assign/tests/behat/allow_another_attempt.feature:36 Moodle exception: Exception - Notice: Undefined property: stdClass::$noreplyaddress in [dirroot]/lib/classes/user.php on line 135 More information about this error × Debug info: Error code: generalexceptionmessage × Stack trace: line 158 of /lib/behat/lib.php: Exception thrown line 135 of /lib/classes/user.php: call to behat_error_handler() line 174 of /lib/classes/user.php: call to core_user::get_dummy_user_record() line 5831 of /mod/assign/locallib.php: call to core_user::get_noreply_user() line 6827 of /mod/assign/locallib.php: call to assign->notify_student_submission_receipt() line 6864 of /mod/assign/locallib.php: call to assign->save_submission() line 437 of /mod/assign/locallib.php: call to assign->process_save_submission() line 55 of /mod/assign/view.php: call to assign->view() (Exception) Failing this issue, so we can set proper email for invalid domains or atleast for behat atleast.
              Show
              rajeshtaneja Rajesh Taneja added a comment - Patch fixing above: wip-mdl-57531 https://github.com/rajeshtaneja/moodle/compare/wip-mdl-57531~1...wip-mdl-57531 git pull https://github.com/rajeshtaneja/moodle.git wip-mdl-57531
              Hide
              poltawski Dan Poltawski added a comment -

              Raj and I discussed to just fix the behat case briefly.

              The problem reported is correct, its the problem with us 'guessing' the noreply address

              Show
              poltawski Dan Poltawski added a comment - Raj and I discussed to just fix the behat case briefly. The problem reported is correct, its the problem with us 'guessing' the noreply address
              Hide
              poltawski Dan Poltawski added a comment - - edited

              I have pulled the behat fix into all branches which is the right way for behat I think.

              Leaving this failed so we can discuss if we want to do anything about the 'bare hostname' case for real admins:

              • The error reported is correct in my opinion and better we inform about it in Moodle than emails going into a blackhole
              • Fundamentally, the problem is that we are trying to guess an email address which we shouldn't really be guessing
              • But there is a reason we do this guessing - forcing admins to choose that address is also not great, it's not their top priority straight after installing Moodle
              • We could just stop the guessing and ensure everyone has to define an address
              Show
              poltawski Dan Poltawski added a comment - - edited I have pulled the behat fix into all branches which is the right way for behat I think. Leaving this failed so we can discuss if we want to do anything about the 'bare hostname' case for real admins: The error reported is correct in my opinion and better we inform about it in Moodle than emails going into a blackhole Fundamentally, the problem is that we are trying to guess an email address which we shouldn't really be guessing But there is a reason we do this guessing - forcing admins to choose that address is also not great, it's not their top priority straight after installing Moodle We could just stop the guessing and ensure everyone has to define an address
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Eloy Lafuente (stronk7): Confirming on 27 I am getting mail from proper norely email address...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Eloy Lafuente (stronk7) : Confirming on 27 I am getting mail from proper norely email address...
              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks Raj for fixing the Behat. I agree that setting noreply explicitly to use example.com is a good fix.

              I was aware of the issue with some currently "working" addresses become invalid with this new validation, and I commented about it above:

              This is likely to affect some (if not most) developer and test machines as values like noreply@localhost would be no longer considered as valid ones.

              My reasoning was that

              • Guessing the address based on $CFG->wwwroot is still valid and helpful for real sites. Vast majority of production sites have the wwwroot value defined correctly and it will not affect them.
              • Email addresses like noreply@localhost are not valid and it is correct that editing the form raises validation error.
              • Developers / local installations running on $CFG->wwwroot = 'http://localhost/'; and similar ones can easily run into situation where the default (guessed) address like my own noreply@glux is invalid. But even then, I do not see it as a problem. I consider it as a trade-off for usability and security of production sites.
              • The easiest way to avoid all these issues on developer machines seems to be having wwwroot set to something like localhost.local if needed.
              Show
              mudrd8mz David Mudrák added a comment - Thanks Raj for fixing the Behat. I agree that setting noreply explicitly to use example.com is a good fix. I was aware of the issue with some currently "working" addresses become invalid with this new validation, and I commented about it above: This is likely to affect some (if not most) developer and test machines as values like noreply@localhost would be no longer considered as valid ones. My reasoning was that Guessing the address based on $CFG->wwwroot is still valid and helpful for real sites. Vast majority of production sites have the wwwroot value defined correctly and it will not affect them. Email addresses like noreply@localhost are not valid and it is correct that editing the form raises validation error. Developers / local installations running on $CFG->wwwroot = 'http://localhost/'; and similar ones can easily run into situation where the default (guessed) address like my own noreply@glux is invalid. But even then, I do not see it as a problem. I consider it as a trade-off for usability and security of production sites. The easiest way to avoid all these issues on developer machines seems to be having wwwroot set to something like localhost.local if needed.
              Hide
              mudrd8mz David Mudrák added a comment -

              If this would be seen as a real issue, I suggest to introduce a new helper function like get_default_noreply_address(). It would use get_host_from_url($CFG->wwwroot) first. If that returns a value without a dot, a suffix like .local or .invalid (to be ignored as per RFC) would be added automatically to it.

              Show
              mudrd8mz David Mudrák added a comment - If this would be seen as a real issue, I suggest to introduce a new helper function like get_default_noreply_address() . It would use get_host_from_url($CFG->wwwroot) first. If that returns a value without a dot, a suffix like .local or .invalid (to be ignored as per RFC) would be added automatically to it.
              Hide
              poltawski Dan Poltawski added a comment -

              I am sending it to back to testing, I am not convinced we should do anything about the 'localhost' problem in this issue (or at all), agree with what David says.

              Show
              poltawski Dan Poltawski added a comment - I am sending it to back to testing, I am not convinced we should do anything about the 'localhost' problem in this issue (or at all), agree with what David says.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Ok, so current status is:

              • automated tests are passing without problem.
              • any site wanting to effectively send noreply emails must have a proper domain OR set a proper noreply address. And ensure the SMTP does not "eat/rewrite" them and so on (like I saw yesterday with gmail, for example).
              • still dev/testing sites using IPs will work (although surely mails won't be delivered anywhere, but can be captured/whatever).

              I think all the above is correct, and the condition for noreply to work, a logic thing that 99% of sites already fulfill.

              In the other side (1%), if you are running with non-dotted domain like "localhost", "testserver".... and want to get mails delivered, please configure the site to have a proper noreply address. We won't be inventing anything special for you (mainly because any invention may lead to non-deliverable mails). You'll get debug messages about problems with noreply, configure it correctly and done.

              So I'd consider this passed, unless somebody has any objection. Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Ok, so current status is: automated tests are passing without problem. any site wanting to effectively send noreply emails must have a proper domain OR set a proper noreply address . And ensure the SMTP does not "eat/rewrite" them and so on (like I saw yesterday with gmail, for example). still dev/testing sites using IPs will work (although surely mails won't be delivered anywhere, but can be captured/whatever). I think all the above is correct, and the condition for noreply to work, a logic thing that 99% of sites already fulfill. In the other side (1%), if you are running with non-dotted domain like "localhost", "testserver".... and want to get mails delivered, please configure the site to have a proper noreply address. We won't be inventing anything special for you (mainly because any invention may lead to non-deliverable mails). You'll get debug messages about problems with noreply, configure it correctly and done. So I'd consider this passed, unless somebody has any objection. Ciao
              Hide
              poltawski Dan Poltawski added a comment -

              Passing

              Show
              poltawski Dan Poltawski added a comment - Passing
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              “We should not only use all the brains we have, but all that we can borrow.”
              — Woodrow Wilson

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. “We should not only use all the brains we have, but all that we can borrow.” — Woodrow Wilson

                People

                • Votes:
                  8 Vote for this issue
                  Watchers:
                  25 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    9/Jan/17