Merge lp:~nurlan0000/psiphon/sprint3-576643 into lp:psiphon

Proposed by Nurlan Turdaliev
Status: Merged
Approved by: Adam Kruger
Approved revision: 123
Merged at revision: 130
Proposed branch: lp:~nurlan0000/psiphon/sprint3-576643
Merge into: lp:psiphon
Diff against target: 429 lines (+283/-19) (has conflicts)
8 files modified
trunk/mod_psiphon/httpd_conf/httpd.conf (+1/-1)
trunk/sql/upgrade-2.6.sql (+2/-0)
trunk/tools/psiphon_install.sh (+1/-1)
trunk/www/config.php (+4/-0)
trunk/www/includes/lang.php (+65/-0)
trunk/www/includes/url_helpers.php (+6/-0)
trunk/www/p.php (+193/-0)
trunk/www/reset_password.php (+11/-17)
Text conflict in trunk/www/includes/lang.php
To merge this branch: bzr merge lp:~nurlan0000/psiphon/sprint3-576643
Reviewer Review Type Date Requested Status
Adam Kruger Approve
Rod Needs Fixing
Nurlan Turdaliev (community) Needs Resubmitting
Review via email: mp+34057@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Adam Kruger (adam-kruger) wrote :

Code review comments:

- The alter table statements should have default values if NOT NULL is used

- We would like to follow the pattern of using ttl from the config file and storing only the reset date in the database

- In lang.php, reset_password_error and password_sent: change "Password reset link" to "A password reset link"

- Always return 404 for invalid conditions in p.php. (In diff line 214 403 is used)

- Use of start_session() and _SESSION is unnecessary. The token GET param will automatically be passed in the POST request as it uses REQUEST_URI

- We noticed inconsistent code formatting: inconsistent braces, we always put braces around single line statements following if statements, multiple empty lines, multiple whitespace characters around
"=" signs, inconsistent php style using <?php echo ...> vs <?=...>

- Diff lines 395-398 is no-op code

- We don't understand the purpose of the "@" symbol at diff lines 261-262

- See Bug #634452 for the correct use of redirects

- The else block at diff line 378 is useless and should be removed. The if block starting at diff line 353 should be unconditional

- The following line is absent:
<?=header("Content-type: text/html; charset=".charset());?>
It should appear above
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">

- The language module expects a global variable $record_user to be set in order to retrieve the current language.
So, $record_user should be set to $user_to_reset

- It is not necessary to escape_html the output of msg() because this is not user input

- Diff line 390: The language buttons should be removed, and instead the $record_user[last_lang] will be automatically used to set the email text and the reset link language to the user's correct language.

- Do not put html markup in the email text. The email is sent formatted as plain text.
$passwordResetLink = '<a href="'.make_password_reset_url($config, $proxy_id = $proxy_record['id'], $pass_reset_token).'">'.
"<a href=" should not appear in the email body text.

- It is no longer necessary to invalidate the user session in reset_password.php when a password reset is requested (diff line 426: session = :session_id,)

review: Disapprove (group review)
Revision history for this message
Adam Kruger (adam-kruger) :
review: Needs Fixing (group review)
122. By Nurlan Turdaliev <ubuntu@ubuntu-desktop>

Implemented psiphon team suggestions

Revision history for this message
Nurlan Turdaliev (nurlan0000) :
review: Needs Resubmitting
Revision history for this message
Rod (rod-psiphon) wrote :

Looks ok except this 403 should be 404:

reset_password.php

if (!$proxy_record || $proxy_record['login_url'] != $_GET[lu])
{
    include($_SERVER[DOCUMENT_ROOT]."/http-errors/403.php");
...

The idea is to *always* return 404 any time invalid input/incorrect secret value is provided to a page outside of an authenticated session. This is to defend against an adversary scanning arbitrary web hosts for Psiphon proxies that respond to HTTP requests with some fingerprint-able response.

review: Needs Fixing
123. By Nurlan Turdaliev <ubuntu@ubuntu-desktop>

return 404 error instead 403

Revision history for this message
Adam Kruger (adam-kruger) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'trunk/mod_psiphon/httpd_conf/httpd.conf'
2--- trunk/mod_psiphon/httpd_conf/httpd.conf 2010-10-18 21:11:30 +0000
3+++ trunk/mod_psiphon/httpd_conf/httpd.conf 2010-11-01 07:42:40 +0000
4@@ -156,7 +156,7 @@
5
6 <Location />
7 PsiphonAuthEnable On
8-PsiphonNoAuth /a.php /e.php /w.php /chk.php /reset_password.php /logout.php
9+PsiphonNoAuth /a.php /e.php /w.php /chk.php /reset_password.php /logout.php /p.php
10 PsiphonAuthLoginPath /auth.php
11 PsiphonLoginURLCheck On
12 </Location>
13
14=== added file 'trunk/sql/upgrade-2.6.sql'
15--- trunk/sql/upgrade-2.6.sql 1970-01-01 00:00:00 +0000
16+++ trunk/sql/upgrade-2.6.sql 2010-11-01 07:42:40 +0000
17@@ -0,0 +1,2 @@
18+ALTER TABLE `user` ADD COLUMN `pass_reset_token` CHAR(32) DEFAULT '' NOT NULL;
19+ALTER TABLE `user` ADD COLUMN `pass_reset_date` INTEGER DEFAULT 0 NOT NULL;
20
21=== modified file 'trunk/tools/psiphon_install.sh'
22--- trunk/tools/psiphon_install.sh 2010-06-01 17:50:42 +0000
23+++ trunk/tools/psiphon_install.sh 2010-11-01 07:42:40 +0000
24@@ -271,7 +271,7 @@
25 echo "Enter current password for mysql root (enter for none)"
26
27 cat sql/create_db.sql sql/grant.new.sql sql/structure.2.3.sql sql/init.sql \
28-sql/upgrade-2.4.sql sql/upgrade-2.5.sql sql/jsf.sql sql/youtube.sql | mysql -uroot -p
29+sql/upgrade-2.4.sql sql/upgrade-2.5.sql sql/upgrade-2.6.sql sql/jsf.sql sql/youtube.sql | mysql -uroot -p
30
31 rm sql/grant.new.sql
32
33
34=== modified file 'trunk/www/config.php'
35--- trunk/www/config.php 2010-06-28 20:44:20 +0000
36+++ trunk/www/config.php 2010-11-01 07:42:40 +0000
37@@ -247,4 +247,8 @@
38
39 $config["geoip_type"]= "country";
40 $config["geoip_data_file"] = "/usr/share/GeoIP/GeoIP.dat";
41+
42+// Password reset
43+// 3 days
44+$config['password_reset_ttl'] = 7 * 24 * 60 * 60;
45 ?>
46
47=== modified file 'trunk/www/includes/lang.php'
48--- trunk/www/includes/lang.php 2010-10-14 16:26:29 +0000
49+++ trunk/www/includes/lang.php 2010-11-01 07:42:40 +0000
50@@ -680,6 +680,7 @@
51 ),
52
53 'reset_password_error' => array(
54+<<<<<<< TREE
55 'en' => "To reset your password, enter your username or e-mail address, and click the reset button. A temporary password will be e-mailed to you.",
56 'fa' => "برای باز نشاندن رمزعبور خود، نام کاربری یا آدرس ایمیل خود را وارد کنید و روی دکمه \"باز نشاندن\" کلیک کنید. یک رمز عبور موقتی به شما ایمیل خواهد شد. ",
57 'ru' => "Для сброса пароля введите имя пользователя или электронный адрес почты и нажмите кнопку сброса. Временный пароль будет отправлен на указанный Вами адрес электронной почты",
58@@ -691,9 +692,35 @@
59 'fr' => "To reset your password, enter your username or e-mail address, and click the reset button. A temporary password will be e-mailed to you.",
60 'es' => "To reset your password, enter your username or e-mail address, and click the reset button. A temporary password will be e-mailed to you.",
61 'be' => "Для сброса пароля введите имя пользователя или электронный адрес почты и нажмите кнопку сброса. Временный пароль будет отправлен на указанный Вами адрес электронной почты",
62+=======
63+ 'en' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
64+ 'fa' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
65+ 'ru' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
66+ 'uz' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
67+ 'tk' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
68+ 'cn' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
69+ 'ar' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
70+ 'vt' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
71+ 'fr' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
72+ 'es' => "To reset your password, enter your username or e-mail address, and click the reset button. A password reset link will be e-mailed to you.",
73+ ),
74+
75+ 'password_changed' => array(
76+ 'en' => "Password changed",
77+ 'fa' => "Password changed",
78+ 'ru' => "Password changed",
79+ 'uz' => "Password changed",
80+ 'tk' => "Password changed",
81+ 'cn' => "Password changed",
82+ 'ar' => "Password changed",
83+ 'vt' => "Password changed",
84+ 'fr' => "Password changed",
85+ 'es' => "Password changed",
86+>>>>>>> MERGE-SOURCE
87 ),
88
89 'reset_password_subject' => array(
90+<<<<<<< TREE
91 'en' => "Your temporary Psiphon password",
92 'fa' => "رمز عبور موقت سایفون",
93 'ru' => "Ваш временный пароль в Psiphon ",
94@@ -705,9 +732,22 @@
95 'fr' => "Votre mot de passe Psiphon temporaire",
96 'es' => "Su contraseña temporal de Psiphon",
97 'be' => "Ваш временный пароль в Psiphon ",
98+=======
99+ 'en' => "Psiphon password reset",
100+ 'fa' => "Psiphon password reset",
101+ 'ru' => "Psiphon password reset",
102+ 'uz' => "Psiphon password reset",
103+ 'tk' => "Psiphon password reset",
104+ 'cn' => "Psiphon password reset",
105+ 'ar' => "Psiphon password reset",
106+ 'vt' => "Psiphon password reset",
107+ 'fr' => "Psiphon password reset",
108+ 'es' => "Psiphon password reset",
109+>>>>>>> MERGE-SOURCE
110 ),
111
112 'reset_password_body' => array(
113+<<<<<<< TREE
114 'en' => "Your temporary Psiphon password is: %s. Please login and change your password immediately.",
115 'fa' => "رمز عبور موقت شما: %s. لطفآ وارد حساب کاربری خود شده و این رمز را عوض کنید. ",
116 'ru' => "Ваш временный пароль в Psiphon: %s. Войдите в систему и немедленно замените свой пароль.",
117@@ -719,9 +759,22 @@
118 'fr' => "Votre mot de passe Psiphon temporaire est : %s. Veuillez vous connecter et changer immédiatement votre mot de passe.",
119 'es' => "Su contraseña temporal de Psiphon es %s. Inicie la sesión y cambie la contraseña de inmediato.",
120 'be' => "Ваш временный пароль в Psiphon: %s. Войдите в систему и немедленно замените свой пароль.",
121+=======
122+ 'en' => "Follow this link to reset your password: %s.",
123+ 'fa' => "Follow this link to reset your password: %s.",
124+ 'ru' => "Follow this link to reset your password: %s.",
125+ 'uz' => "Follow this link to reset your password: %s.",
126+ 'tk' => "Follow this link to reset your password: %s.",
127+ 'cn' => "Follow this link to reset your password: %s.",
128+ 'ar' => "Follow this link to reset your password: %s.",
129+ 'vt' => "Follow this link to reset your password: %s.",
130+ 'fr' => "Follow this link to reset your password: %s.",
131+ 'es' => "Follow this link to reset your password: %s.",
132+>>>>>>> MERGE-SOURCE
133 ),
134
135 'password_sent' => array(
136+<<<<<<< TREE
137 'en' => "A temporary password was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
138 'fa' => "یک رمز عبور موقت به آدرس ایمیل شما فرستاده شده است. اگر ایمیل دریافت نکردید نام کاربری یا آدرس ایمیل خود را چک کنید و دوباره امتحان کنید. ",
139 'ru' => "Временный пароль отправлен на указанный Вами адрес электронной почты. Если Вы не получили сообщение, еще раз проверьте правильность имени пользователя и электронного адреса и повторите попытку.",
140@@ -733,6 +786,18 @@
141 'fr' => "Un mot de passe temporaire a été envoyé à votre adresse électronique. Si vous ne recevez pas de courriel, vérifiez à nouveau votre nom utilisateur ou votre adresse électronique et essayez une nouvelle fois.",
142 'es' => "Se le ha enviado una contraseña temporal a su dirección de correo electrónico. Si no recibe el mensaje con la contraseña, verifique nuevamente su nombre de usuario o dirección de correo electrónico y vuelva a intentar.",
143 'be' => "Временный пароль отправлен на указанный Вами адрес электронной почты. Если Вы не получили сообщение, еще раз проверьте правильность имени пользователя и электронного адреса и повторите попытку.",
144+=======
145+ 'en' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
146+ 'fa' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
147+ 'ru' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
148+ 'uz' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
149+ 'tk' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
150+ 'cn' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
151+ 'ar' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
152+ 'vt' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
153+ 'fr' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
154+ 'es' => "A password reset link was sent to your e-mail address. If you do not receive an e-mail, double check your username or e-mail address and try again.",
155+>>>>>>> MERGE-SOURCE
156 ),
157
158 'no_address' => array(
159
160=== modified file 'trunk/www/includes/url_helpers.php'
161--- trunk/www/includes/url_helpers.php 2010-06-08 17:58:34 +0000
162+++ trunk/www/includes/url_helpers.php 2010-11-01 07:42:40 +0000
163@@ -106,4 +106,10 @@
164
165 }
166
167+function make_password_reset_url($config, $proxy_id, $token) {
168+ // URL --> https://<proxy ip address|hostname>/p.php?token=01234567890123456789
169+
170+ return "https://".get_proxy_host($config, $proxy_id)."/p.php?"."token=".$token;
171+}
172+
173 ?>
174
175=== added file 'trunk/www/p.php'
176--- trunk/www/p.php 1970-01-01 00:00:00 +0000
177+++ trunk/www/p.php 2010-11-01 07:42:40 +0000
178@@ -0,0 +1,193 @@
179+<?php
180+/*
181+ Psiphon Circumvention Platform
182+ Copyright (C) 2009-2010 Psiphon Inc..
183+
184+ This program is free software: you can redistribute it and/or modify
185+ it under the terms of the GNU General Public License as published by
186+ the Free Software Foundation, either version 3 of the License, or
187+ (at your option) any later version.
188+
189+ This program is distributed in the hope that it will be useful,
190+ but WITHOUT ANY WARRANTY; without even the implied warranty of
191+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
192+ GNU General Public License for more details.
193+
194+ You should have received a copy of the GNU General Public License
195+ along with this program. If not, see <http://www.gnu.org/licenses/>.
196+*/
197+
198+
199+include_once($_SERVER[DOCUMENT_ROOT]."/includes/common_includes.php");
200+include_once($_SERVER[DOCUMENT_ROOT]."/includes/cr.php");
201+
202+
203+
204+$token = $_GET['token'];
205+$passwords_submitted = isset($_POST['submit_button']);
206+
207+
208+// check token
209+if (strlen($token) != 32) {
210+ include($_SERVER[DOCUMENT_ROOT]."/http-errors/404.php");
211+}
212+
213+
214+/// Fetch the user to reset password
215+$record_user = null;
216+
217+if ( $token )
218+{
219+ $query =
220+ "SELECT * FROM user ".
221+ "WHERE pass_reset_token = :token AND UNIX_TIMESTAMP() < pass_reset_date + ".$config['password_reset_ttl'];
222+
223+ $result = db_query_execute($config, $query,
224+ array(":token" => $token)
225+ );
226+
227+ $record_user = db_fetch_result($config, $result);
228+
229+
230+ if ( ! $record_user )
231+ {
232+ include($_SERVER[DOCUMENT_ROOT]."/http-errors/404.php");
233+ }
234+}
235+
236+$query = "SELECT * FROM proxy WHERE hostname = :hostname";
237+$result = db_query_execute($config, $query, convert_null_array(array(":hostname" => $_SERVER[HTTP_HOST])));
238+$proxy_record = db_fetch_result($config, $result);
239+
240+if ( ! can_current_user_access_proxy($config, $record_user, $proxy_record['id']))
241+{
242+ include($_SERVER[DOCUMENT_ROOT]."/http-errors/404.php");
243+}
244+
245+
246+
247+$password_error = null;
248+
249+if ($passwords_submitted)
250+{
251+ $password1 = $_POST['password1'];
252+ $password2 = $_POST['password2'];
253+
254+
255+ // password validation
256+
257+ if (!is_valid_password($config, $password1, $record_user['uname']))
258+ $password_error = sprintf(msg("password_invalid"), $config[min_pass_length]);
259+
260+ if ($password1 != $password2)
261+ $password_error = msg("password_mismatch");
262+
263+ // Update user password
264+ if ( ! $password_error )
265+ {
266+ $pass_salt = bin2hex(secure_rand());
267+ $password = sha1($password1.$pass_salt);
268+
269+ // unguessable session ID
270+ $session_id = bin2hex(secure_rand());
271+
272+
273+ // reset password, password salt and invalidate password reset token.
274+ $query =
275+ 'UPDATE `user` '.
276+ 'SET '.
277+ 'pass = :password, '.
278+ 'pass_salt = :pass_salt, '.
279+ 'session = :session, '.
280+ 'pass_reset_token = "" '.
281+ 'WHERE id = :id';
282+
283+ db_query_execute(
284+ $config,
285+ $query,
286+ array(
287+ ':password' => $password,
288+ ':pass_salt' => $pass_salt,
289+ ':session' => $session_id,
290+ ':id' => $record_user['id']
291+ )
292+ );
293+ }
294+}
295+
296+$password_changed = $passwords_submitted && ! $password_error;
297+if ($password_changed)
298+{
299+ $login_url = $proxy_record['login_url'];
300+ header('Location: '.$config['ret'].$login_url, true, 302);
301+ exit;
302+}
303+
304+
305+// set page title
306+$page_title = msg("reset_password");
307+
308+header("Content-type: text/html; charset=".charset());
309+?>
310+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
311+<html>
312+ <head>
313+ <title><?= $page_title; ?></title>
314+ <?=echo_css()?>
315+ </head>
316+
317+ <body style="direction: <?=direction()?>">
318+
319+ <table>
320+ <tr>
321+ <td style="width:50%">
322+<?php
323+ include($_SERVER[DOCUMENT_ROOT]."/images/psiphonlogo.html");
324+?>
325+ </td>
326+ <td>
327+ <?php if ( ! $password_changed ) : ?>
328+ <font class="sixteenbluebold"><?=msg("reset_password")?></font>
329+ <?php else: ?>
330+ <font class="sixteenbluebold"><?=msg("password_changed")?></font>
331+ <?php endif; ?>
332+ </td>
333+ </tr>
334+ </table>
335+ <br>
336+
337+
338+ <?php if ($password_error) : ?>
339+ <font class="processing_error"><?= $password_error; ?></font>
340+ <br /><br />
341+ <?php endif;?>
342+
343+ <form name="reset_password" action="<?=escape_html(getenv("REQUEST_URI"))?>" method="post">
344+ <table>
345+ <tr>
346+ <td>
347+ <label for="password1"><?= msg('password'); ?></label></td>
348+ <td>
349+ <input type="password" name="password1" id="password1" />
350+ </td>
351+ </tr>
352+ <tr>
353+ <td>
354+ <label for="password2"><?= msg('verify_password'); ?></label>
355+ </td>
356+ <td>
357+ <input type="password" name="password2" id="password2" /><br />
358+ </td>
359+ </tr>
360+ <tr>
361+ <td colspan="2">
362+ <input type="submit" name="submit_button" value="<?= msg('save'); ?>" />
363+ </td>
364+ </tr>
365+ </table>
366+ </form>
367+
368+ <br>
369+ <br>
370+ </body>
371+</html>
372\ No newline at end of file
373
374=== modified file 'trunk/www/reset_password.php'
375--- trunk/www/reset_password.php 2010-07-07 15:55:51 +0000
376+++ trunk/www/reset_password.php 2010-11-01 07:42:40 +0000
377@@ -86,33 +86,27 @@
378 break;
379 }
380
381- // The temporary password should be strong relative to the minimum allowed password.
382- // We'll make it as strong as a password that's twice as long as the minimum.
383-
384- $temp_password_length =
385- log(pow(75, $config[min_pass_length]), // there are about 75 possible characters in a strong password
386- 16); // temp password is hex
387- $temp_password_length = ceil($temp_password_length);
388- $random_bytes_needed = ceil($temp_password_length/2.0);
389- $login_pass = strtoupper(substr(bin2hex(secure_rand($random_bytes_needed)), 0, $temp_password_length));
390-
391- $pass_salt = bin2hex(secure_rand());
392+ $pass_reset_token = bin2hex(secure_rand());
393
394 // update user record with new, temporary password
395 // user.session is set to a large random number to invalidate the session.
396 // TODO: Set session to null instead.
397 $query = "UPDATE user ".
398- "SET pass = :set_pass, pass_salt = :pass_salt, session = :session_id ".
399+ "SET pass_reset_token = :token, ".
400+ "pass_reset_date = UNIX_TIMESTAMP() ".
401 "WHERE id = :user_id";
402
403 db_query_execute(
404 $config,
405 $query,
406 convert_null_array(array(
407- ":set_pass" => sha1($login_pass.$pass_salt),
408- ":pass_salt" => $pass_salt,
409- ":session_id" => bin2hex(secure_rand()),
410- ":user_id" => $user_record['id'])));
411+ ":token" => $pass_reset_token,
412+ ":user_id" => $user_record['id']
413+ ))
414+ );
415+
416+ $passwordResetLink = make_password_reset_url($config, $proxy_id = $proxy_record['id'], $pass_reset_token).
417+ ' '.msg("reset_password_subject", $user_record['last_lang']);
418
419 // send the email
420 // TODO: should be some throttling here, as anyone can send an
421@@ -121,7 +115,7 @@
422 $config,
423 $user_record['email'],
424 msg("reset_password_subject", $user_record['last_lang']),
425- sprintf(msg("reset_password_body", $user_record['last_lang']),$login_pass),
426+ sprintf(msg("reset_password_body", $user_record['last_lang']), $passwordResetLink),
427 $user_record['last_lang']);
428
429 // The feedback here leaked information and allowed anyone to probe

Subscribers

People subscribed via source and target branches