Disallow email addresses which will overflow MySQL storage
Summary: Via HackerOne. An attacker can bypass `auth.email-domains` by registering with an email like: aaaaa...aaaaa@evil.com@company.com We'll validate the full string, then insert it into the database where it will be truncated, removing the `@company.com` part. Then we'll send an email to `@evil.com`. Instead, reject email addresses which won't fit in the table. `STRICT_ALL_TABLES` stops this attack, I'm going to add a setup warning encouraging it. Test Plan: - Set `auth.email-domains` to `@company.com`. - Registered with `aaa...aaa@evil.com@company.com`. Previously this worked, now it is rejected. - Did a valid registration. - Tried to add `aaa...aaaa@evil.com@company.com` as an email address. Previously this worked, now it is rejected. - Did a valid email add. - Added and executed unit tests. Reviewers: btrahan, arice Reviewed By: arice CC: aran, chad Differential Revision: https://secure.phabricator.com/D8308
This commit is contained in:
		| @@ -2175,6 +2175,7 @@ phutil_register_library_map(array( | ||||
|     'PhabricatorUserCustomFieldStringIndex' => 'applications/people/storage/PhabricatorUserCustomFieldStringIndex.php', | ||||
|     'PhabricatorUserDAO' => 'applications/people/storage/PhabricatorUserDAO.php', | ||||
|     'PhabricatorUserEditor' => 'applications/people/editor/PhabricatorUserEditor.php', | ||||
|     'PhabricatorUserEditorTestCase' => 'applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php', | ||||
|     'PhabricatorUserEmail' => 'applications/people/storage/PhabricatorUserEmail.php', | ||||
|     'PhabricatorUserLog' => 'applications/people/storage/PhabricatorUserLog.php', | ||||
|     'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php', | ||||
| @@ -4997,6 +4998,7 @@ phutil_register_library_map(array( | ||||
|     'PhabricatorUserCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage', | ||||
|     'PhabricatorUserDAO' => 'PhabricatorLiskDAO', | ||||
|     'PhabricatorUserEditor' => 'PhabricatorEditor', | ||||
|     'PhabricatorUserEditorTestCase' => 'PhabricatorTestCase', | ||||
|     'PhabricatorUserEmail' => 'PhabricatorUserDAO', | ||||
|     'PhabricatorUserLog' => 'PhabricatorUserDAO', | ||||
|     'PhabricatorUserPreferences' => 'PhabricatorUserDAO', | ||||
|   | ||||
| @@ -200,8 +200,11 @@ final class PhabricatorAuthRegisterController | ||||
|         if (!strlen($value_email)) { | ||||
|           $e_email = pht('Required'); | ||||
|           $errors[] = pht('Email is required.'); | ||||
|         } else if (!PhabricatorUserEmail::isAllowedAddress($value_email)) { | ||||
|         } else if (!PhabricatorUserEmail::isValidAddress($value_email)) { | ||||
|           $e_email = pht('Invalid'); | ||||
|           $errors[] = PhabricatorUserEmail::describeValidAddresses(); | ||||
|         } else if (!PhabricatorUserEmail::isAllowedAddress($value_email)) { | ||||
|           $e_email = pht('Disallowed'); | ||||
|           $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); | ||||
|         } else { | ||||
|           $e_email = null; | ||||
|   | ||||
| @@ -570,6 +570,10 @@ final class PhabricatorUserEditor extends PhabricatorEditor { | ||||
|     // user friendly errors for us, but we omit the courtesy checks on some | ||||
|     // pathways like administrative scripts for simplicity. | ||||
|  | ||||
|     if (!PhabricatorUserEmail::isValidAddress($email->getAddress())) { | ||||
|       throw new Exception(PhabricatorUserEmail::describeValidAddresses()); | ||||
|     } | ||||
|  | ||||
|     if (!PhabricatorUserEmail::isAllowedAddress($email->getAddress())) { | ||||
|       throw new Exception(PhabricatorUserEmail::describeAllowedAddresses()); | ||||
|     } | ||||
|   | ||||
| @@ -0,0 +1,68 @@ | ||||
| <?php | ||||
|  | ||||
| final class PhabricatorUserEditorTestCase extends PhabricatorTestCase { | ||||
|  | ||||
|   protected function getPhabricatorTestCaseConfiguration() { | ||||
|     return array( | ||||
|       self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true, | ||||
|     ); | ||||
|   } | ||||
|  | ||||
|   public function testRegistrationEmailOK() { | ||||
|     $env = PhabricatorEnv::beginScopedEnv(); | ||||
|     $env->overrideEnvConfig('auth.email-domains', array('example.com')); | ||||
|  | ||||
|     $this->registerUser( | ||||
|       'PhabricatorUserEditorTestCaseOK', | ||||
|       'PhabricatorUserEditorTestCase@example.com'); | ||||
|   } | ||||
|  | ||||
|   public function testRegistrationEmailInvalid() { | ||||
|     $env = PhabricatorEnv::beginScopedEnv(); | ||||
|     $env->overrideEnvConfig('auth.email-domains', array('example.com')); | ||||
|  | ||||
|     $prefix = str_repeat('a', PhabricatorUserEmail::MAX_ADDRESS_LENGTH); | ||||
|     $email = $prefix.'@evil.com@example.com'; | ||||
|  | ||||
|     try { | ||||
|       $this->registerUser( | ||||
|         'PhabricatorUserEditorTestCaseInvalid', | ||||
|         $email); | ||||
|     } catch (Exception $ex) { | ||||
|       $caught = $ex; | ||||
|     } | ||||
|  | ||||
|     $this->assertEqual(true, ($caught instanceof Exception)); | ||||
|   } | ||||
|  | ||||
|   public function testRegistrationEmailDomain() { | ||||
|     $env = PhabricatorEnv::beginScopedEnv(); | ||||
|     $env->overrideEnvConfig('auth.email-domains', array('example.com')); | ||||
|  | ||||
|     $caught = null; | ||||
|     try { | ||||
|       $this->registerUser( | ||||
|         'PhabricatorUserEditorTestCaseDomain', | ||||
|         'PhabricatorUserEditorTestCase@whitehouse.gov'); | ||||
|     } catch (Exception $ex) { | ||||
|       $caught = $ex; | ||||
|     } | ||||
|  | ||||
|     $this->assertEqual(true, ($caught instanceof Exception)); | ||||
|   } | ||||
|  | ||||
|   private function registerUser($username, $email) { | ||||
|     $user = id(new PhabricatorUser()) | ||||
|       ->setUsername($username) | ||||
|       ->setRealname($username); | ||||
|  | ||||
|     $email = id(new PhabricatorUserEmail()) | ||||
|       ->setAddress($email) | ||||
|       ->setIsVerified(0); | ||||
|  | ||||
|     id(new PhabricatorUserEditor()) | ||||
|       ->setActor($user) | ||||
|       ->createNewUser($user, $email); | ||||
|   } | ||||
|  | ||||
| } | ||||
| @@ -12,6 +12,8 @@ final class PhabricatorUserEmail extends PhabricatorUserDAO { | ||||
|   protected $isPrimary; | ||||
|   protected $verificationCode; | ||||
|  | ||||
|   const MAX_ADDRESS_LENGTH = 128; | ||||
|  | ||||
|   public function getVerificationURI() { | ||||
|     return '/emailverify/'.$this->getVerificationCode().'/'; | ||||
|   } | ||||
| @@ -27,10 +29,36 @@ final class PhabricatorUserEmail extends PhabricatorUserDAO { | ||||
| /* -(  Domain Restrictions  )------------------------------------------------ */ | ||||
|  | ||||
|  | ||||
|   /** | ||||
|    * @task restrictions | ||||
|    */ | ||||
|   public static function isValidAddress($address) { | ||||
|     if (strlen($address) > self::MAX_ADDRESS_LENGTH) { | ||||
|       return false; | ||||
|     } | ||||
|  | ||||
|     return true; | ||||
|   } | ||||
|  | ||||
|  | ||||
|   /** | ||||
|    * @task restrictions | ||||
|    */ | ||||
|   public static function describeValidAddresses() { | ||||
|     return pht( | ||||
|       'The maximum length of an email address is %d character(s).', | ||||
|       new PhutilNumber(self::MAX_ADDRESS_LENGTH)); | ||||
|   } | ||||
|  | ||||
|  | ||||
|   /** | ||||
|    * @task restrictions | ||||
|    */ | ||||
|   public static function isAllowedAddress($address) { | ||||
|     if (!self::isValidAddress($address)) { | ||||
|       return false; | ||||
|     } | ||||
|  | ||||
|     $allowed_domains = PhabricatorEnv::getEnvConfig('auth.email-domains'); | ||||
|     if (!$allowed_domains) { | ||||
|       return true; | ||||
|   | ||||
| @@ -173,8 +173,11 @@ final class PhabricatorSettingsPanelEmailAddresses | ||||
|       if (!strlen($email)) { | ||||
|         $e_email = pht('Required'); | ||||
|         $errors[] = pht('Email is required.'); | ||||
|       } else if (!PhabricatorUserEmail::isAllowedAddress($email)) { | ||||
|       } else if (!PhabricatorUserEmail::isValidAddress($email)) { | ||||
|         $e_email = pht('Invalid'); | ||||
|         $errors[] = PhabricatorUserEmail::describeValidAddresses(); | ||||
|       } else if (!PhabricatorUserEmail::isAllowedAddress($email)) { | ||||
|         $e_email = pht('Disallowed'); | ||||
|         $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); | ||||
|       } | ||||
|  | ||||
|   | ||||
		Reference in New Issue
	
	Block a user
	 epriestley
					epriestley