@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Fix some security issues with email password resets
Summary:
Via HackerOne, there are two related low-severity issues with this workflow:
- We don't check if you're already logged in, so an attacker can trick a victim (whether they're logged in or not) into clicking a reset link for an account the attacker controls (maybe via an invisible iframe) and log the user in under a different account.
- We don't check CSRF tokens either, so after fixing the first thing, an attacker can still trick a //logged-out// victim in the same way.
It's not really clear that doing this opens up any significant attacks afterward, but both of these behaviors aren't good.
I'll probably land this for audit in a few hours if @btrahan doesn't have a chance to take a look at it since he's probably on a plane for most of the day, I'm pretty confident it doesn't break anything.
Test Plan:
- As a logged-in user, clicked another user's password reset link and was not logged in.
- As a logged-out user, clicked a password reset link and needed to submit a form to complete the workflow.
Reviewers: btrahan
CC: chad, btrahan, aran
Differential Revision: https://secure.phabricator.com/D8079
+64
-29
+64
-29
src/applications/auth/controller/PhabricatorEmailTokenController.php
+64
-29
src/applications/auth/controller/PhabricatorEmailTokenController.php
···
16
16
public function processRequest() {
17
17
$request = $this->getRequest();
18
18
19
+
if ($request->getUser()->isLoggedIn()) {
20
+
return $this->renderError(
21
+
pht('You are already logged in.'));
22
+
}
23
+
19
24
$token = $this->token;
20
25
$email = $request->getStr('email');
21
26
22
-
// NOTE: We need to bind verification to **addresses**, not **users**,
23
-
// because we verify addresses when they're used to login this way, and if
24
-
// we have a user-based verification you can:
25
-
//
26
-
// - Add some address you do not own;
27
-
// - request a password reset;
28
-
// - change the URI in the email to the address you don't own;
29
-
// - login via the email link; and
30
-
// - get a "verified" address you don't control.
31
-
32
27
$target_email = id(new PhabricatorUserEmail())->loadOneWhere(
33
28
'address = %s',
34
29
$email);
···
40
35
$target_email->getUserPHID());
41
36
}
42
37
38
+
// NOTE: We need to bind verification to **addresses**, not **users**,
39
+
// because we verify addresses when they're used to login this way, and if
40
+
// we have a user-based verification you can:
41
+
//
42
+
// - Add some address you do not own;
43
+
// - request a password reset;
44
+
// - change the URI in the email to the address you don't own;
45
+
// - login via the email link; and
46
+
// - get a "verified" address you don't control.
47
+
43
48
if (!$target_email ||
44
49
!$target_user ||
45
50
!$target_user->validateEmailToken($target_email, $token)) {
···
65
70
));
66
71
}
67
72
68
-
// Verify email so that clicking the link in the "Welcome" email is good
69
-
// enough, without requiring users to go through a second round of email
70
-
// verification.
73
+
if ($request->isFormPost()) {
74
+
// Verify email so that clicking the link in the "Welcome" email is good
75
+
// enough, without requiring users to go through a second round of email
76
+
// verification.
77
+
78
+
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
79
+
$target_email->setIsVerified(1);
80
+
$target_email->save();
81
+
unset($unguarded);
82
+
83
+
$next = '/';
84
+
if (!PhabricatorAuthProviderPassword::getPasswordProvider()) {
85
+
$next = '/settings/panel/external/';
86
+
} else if (PhabricatorEnv::getEnvConfig('account.editable')) {
87
+
$next = (string)id(new PhutilURI('/settings/panel/password/'))
88
+
->setQueryParams(
89
+
array(
90
+
'token' => $token,
91
+
'email' => $email,
92
+
));
93
+
}
71
94
72
-
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
73
-
$target_email->setIsVerified(1);
74
-
$target_email->save();
75
-
unset($unguarded);
95
+
PhabricatorCookies::setNextURICookie($request, $next, $force = true);
76
96
77
-
$next = '/';
78
-
if (!PhabricatorAuthProviderPassword::getPasswordProvider()) {
79
-
$next = '/settings/panel/external/';
80
-
} else if (PhabricatorEnv::getEnvConfig('account.editable')) {
81
-
$next = (string)id(new PhutilURI('/settings/panel/password/'))
82
-
->setQueryParams(
83
-
array(
84
-
'token' => $token,
85
-
'email' => $email,
86
-
));
97
+
return $this->loginUser($target_user);
87
98
}
88
99
89
-
PhabricatorCookies::setNextURICookie($request, $next, $force = true);
100
+
// NOTE: We need to CSRF here so attackers can't generate an email link,
101
+
// then log a user in to an account they control via sneaky invisible
102
+
// form submissions.
90
103
91
-
return $this->loginUser($target_user);
104
+
// TODO: Since users can arrive here either through password reset or
105
+
// through welcome emails, it might be nice to include the workflow type
106
+
// in the URI or query params so we can tailor the messaging. Right now,
107
+
// it has to be generic enough to make sense in either workflow, which
108
+
// leaves it feeling a little awkward.
109
+
110
+
$dialog = id(new AphrontDialogView())
111
+
->setUser($request->getUser())
112
+
->setTitle(pht('Login to Phabricator'))
113
+
->addHiddenInput('email', $email)
114
+
->appendParagraph(
115
+
pht(
116
+
'Use the button below to log in as: %s',
117
+
phutil_tag('strong', array(), $email)))
118
+
->appendParagraph(
119
+
pht(
120
+
'After logging in you should set a password for your account, or '.
121
+
'link your account to an external account that you can use to '.
122
+
'authenticate in the future.'))
123
+
->addSubmitButton(pht('Login (%s)', $email))
124
+
->addCancelButton('/');
125
+
126
+
return id(new AphrontDialogResponse())->setDialog($dialog);
92
127
}
93
128
}