From f3d6e11ca0a48fa2d383461e9f54fa21ae2f51cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20Mudr=C3=A1k?= <david@moodle.com>
Date: Wed, 4 Jan 2017 13:05:03 +0100
Subject: [PATCH] MDL-57531 mail: Validate the sender's email address

The patch adds validation for the noreplyaddress setting variable, for
the explicit $replyto parameter and for the sender's email. In case of
misconfigured noreplyaddress setting, it falls back to the default
noreply address value. In case of invalid email in the user's record,
the email is not sent.

The patch also adds unit test for the value returned by the function
generate_email_processing_address() so that it can be considered as a
valid email, too.

This is supposed to significantly minimise the risk of exploiting the
vulnerability in PHPMailer's Sender field.
---
 lib/moodlelib.php            | 29 ++++++++++++++++++++++++++---
 lib/tests/moodlelib_test.php | 23 +++++++++++++++++++++++
 lib/tests/weblib_test.php    | 15 +++++++++++++++
 3 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/lib/moodlelib.php b/lib/moodlelib.php
index 344390a585a..764992b8026 100644
--- a/lib/moodlelib.php
+++ b/lib/moodlelib.php
@@ -5670,6 +5670,18 @@ function email_to_user($user, $from, $subject, $messagetext, $messagehtml = '',
     $tempreplyto = array();
 
     $supportuser = core_user::get_support_user();
+    $noreplyaddressdefault = 'noreply@' . get_host_from_url($CFG->wwwroot);
+    $noreplyaddress = empty($CFG->noreplyaddress) ? $noreplyaddressdefault : $CFG->noreplyaddress;
+
+    if (!validate_email($noreplyaddress)) {
+        debugging('email_to_user: Invalid noreply-email '.s($noreplyaddress));
+        $noreplyaddress = $noreplyaddressdefault;
+    }
+
+    if (!validate_email($supportuser->email)) {
+        debugging('email_to_user: Invalid support-email '.s($supportuser->email));
+        $supportuser->email = $noreplyaddress;
+    }
 
     // Make up an email address for handling bounces.
     if (!empty($CFG->handlebounces)) {
@@ -5687,17 +5699,28 @@ function email_to_user($user, $from, $subject, $messagetext, $messagehtml = '',
         }
     }
 
+    // Make sure that the explicit replyto is valid, fall back to the implicit one.
+    if (!empty($replyto) && !validate_email($replyto)) {
+        debugging('email_to_user: Invalid replyto-email '.s($replyto));
+        $replyto = $noreplyaddress;
+    }
+
     if (is_string($from)) { // So we can pass whatever we want if there is need.
-        $mail->From     = $CFG->noreplyaddress;
+        $mail->From     = $noreplyaddress;
         $mail->FromName = $from;
     } else if ($usetrueaddress and $from->maildisplay) {
+        if (!validate_email($from->email)) {
+            debugging('email_to_user: Invalid from-email '.s($from->email).' - not sending');
+            // Better not to use $noreplyaddress in this case.
+            return false;
+        }
         $mail->From     = $from->email;
         $mail->FromName = fullname($from);
     } else {
-        $mail->From     = $CFG->noreplyaddress;
+        $mail->From     = $noreplyaddress;
         $mail->FromName = fullname($from);
         if (empty($replyto)) {
-            $tempreplyto[] = array($CFG->noreplyaddress, get_string('noreplyname'));
+            $tempreplyto[] = array($noreplyaddress, get_string('noreplyname'));
         }
     }
 
diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php
index 353187e7225..5cc6e0da966 100644
--- a/lib/tests/moodlelib_test.php
+++ b/lib/tests/moodlelib_test.php
@@ -3130,4 +3130,27 @@ class core_moodlelib_testcase extends advanced_testcase {
         $this->assertSame('', $result);
         $this->assertDebuggingCalled();
     }
+
+    /**
+     * Test that generate_email_processing_address() returns valid email address.
+     */
+    public function test_generate_email_processing_address() {
+        global $CFG;
+        $this->resetAfterTest();
+
+        $data = (object)[
+            'id' => 42,
+            'email' => 'my.email+from_moodle@example.com',
+        ];
+
+        $modargs = 'B'.base64_encode(pack('V', $data->id)).substr(md5($data->email), 0, 16);
+
+        $CFG->maildomain = 'example.com';
+        $CFG->mailprefix = 'mdl+';
+        $this->assertEquals(1, validate_email(generate_email_processing_address(0, $modargs)));
+
+        $CFG->maildomain = 'mail.example.com';
+        $CFG->mailprefix = 'mdl-';
+        $this->assertEquals(1, validate_email(generate_email_processing_address(23, $modargs)));
+    }
 }
diff --git a/lib/tests/weblib_test.php b/lib/tests/weblib_test.php
index 73165b37bb4..d978a80e461 100644
--- a/lib/tests/weblib_test.php
+++ b/lib/tests/weblib_test.php
@@ -638,4 +638,19 @@ EXPECTED;
         );
     }
 
+    /**
+     * Tests for validate_email() function.
+     */
+    public function test_validate_email() {
+
+        $this->assertEquals(1, validate_email('moodle@example.com'));
+        $this->assertEquals(1, validate_email('moodle@localhost.local'));
+        $this->assertEquals(1, validate_email('verp_email+is=mighty@moodle.org'));
+        $this->assertEquals(1, validate_email("but_potentially'dangerous'too@example.org"));
+        $this->assertEquals(1, validate_email('posts+AAAAAAAAAAIAAAAAAAAGQQAAAAABFSXz1eM/P/lR2bYyljM+@posts.moodle.org'));
+
+        $this->assertEquals(0, validate_email('moodle@localhost'));
+        $this->assertEquals(0, validate_email('"attacker\\" -oQ/tmp/ -X/var/www/vhost/moodle/backdoor.php  some"@email.com'));
+        $this->assertEquals(0, validate_email("moodle@example.com>\r\nRCPT TO:<victim@example.com"));
+    }
 }