Fossil SCM

Improved comments. Extra defensive code.

drh 2023-01-07 15:18 self-service-password-reset
Commit d860e2b5f6222f8780c4f0e3aa105faebcc856315c6143a4ff50103cc4901d77
Parent 41bb73e9ba49c12…
1 file changed +41 -25
~ src/login.c
M src/login.c
+41 -25
--- src/login.c
+++ src/login.c
@@ -569,11 +569,13 @@
569569
 const int noAnon = P("noanon")!=0;
570570
 int rememberMe; /* If true, use persistent cookie, else
571571
 session cookie. Toggled per
572572
 checkbox. */
573573
574
- if( P("pwreset")!=0 ){
574
+ if( P("pwreset")!=0 && login_self_password_reset_available() ){
575
+ /* If the "Reset Password" button in the form was pressed, render
576
+ ** the Request Password Reset page in place of this one. */
575577
 login_reqpwreset_page();
576578
 return;
577579
 }
578580
 login_check_credentials();
579581
 fossil_redirect_to_https_if_needed(1);
@@ -868,15 +870,15 @@
868870
 **
869871
 ** Where UID and TIMESTAMP are the parameters to this function, and HASH
870872
 ** is constructed from information that is unique to the user in question
871873
 ** and which is not publicly available. In particular, the HASH includes
872874
 ** the existing user password. Thus, in order to construct a URL that can
873
-** change a password, the attacker must know the current password, in which
874
-** case that do not need to construct the URL in order to take over the
875
-** account.
875
+** change a password, an attacker must know the current password, in which
876
+** case the attacker does not need to construct the URL in order to take
877
+** over the account.
876878
 **
877
-** Return a pointer to the resulting string in memory obtained
879
+** Return a pointer to the resulting string in memory obtained
878880
 ** from fossil_malloc().
879881
 */
880882
 char *login_resetpw_suffix(int uid, i64 timestamp){
881883
 char *zHash;
882884
 char *zInnerSql;
@@ -895,10 +897,15 @@
895897
 " FROM user WHERE uid=%d", timestamp, uid);
896898
 }
897899
 zHash = db_text(0, "SELECT lower(hex(sha3_query(%Q)))", zInnerSql);
898900
 fossil_free(zInnerSql);
899901
 zResult = mprintf("%x-%llx-%s", uid, timestamp, zHash);
902
+ if( strlen(zHash)<64 || strlen(zResult)<70 ){
903
+ /* This should never happen, but if it does, we don't want it to lead
904
+ ** to a security breach. */
905
+ fossil_panic("insecure password reset hash generated\n");
906
+ }
900907
 fossil_free(zHash);
901908
 return zResult;
902909
 }
903910
904911
 /*
@@ -911,10 +918,11 @@
911918
 int i, j;
912919
 int uid;
913920
 i64 timestamp;
914921
 i64 now;
915922
 char *zHash;
923
+ if( zName==0 || strlen(zName)<70 ) goto not_valid_suffix;
916924
 for(i=0; fossil_isxdigit(zName[i]); i++){}
917925
 if( i<1 || zName[i]!='-' ) goto not_valid_suffix;
918926
 for(j=i+1; fossil_isxdigit(zName[j]); j++){}
919927
 if( j<=i+1 || zName[j]!='-' ) goto not_valid_suffix;
920928
 uid = strtol(zName, 0, 16);
@@ -932,43 +940,47 @@
932940
 }
933941
 fossil_free(zHash);
934942
 return uid;
935943
936944
 not_valid_suffix:
937
- sleep(2); /* Introduce a small delay on an invalid suffix as an
938
- ** extra defense against search attacks */
939945
 return 0;
940946
 }
941947
942948
 /*
943949
 ** COMMAND: test-resetpw-url
944950
 ** Usage: fossil test-resetpw-url UID
945951
 **
946952
 ** Generate and verify a /resetpw URL for user UID.
953
+**
954
+** This command is intended for unit testing the login_resetpw_suffix()
955
+** and login_resetpw_suffix_is_valid() functions.
947956
 */
948957
 void test_resetpw_url(void){
949958
 char *zSuffix;
950959
 int uid;
951960
 int xuid;
952961
 char *zLogin;
962
+ int i;
953963
 db_find_and_open_repository(0, 0);
954964
 verify_all_options();
955
- if( g.argc!=3 ){
956
- usage("UID");
957
- }
958
- uid = atoi(g.argv[2]);
959
- zSuffix = login_resetpw_suffix(uid, 0);
960
- xuid = login_resetpw_suffix_is_valid(zSuffix);
961
- if( xuid>0 ){
962
- zLogin = db_text(0, "SELECT login FROM user WHERE uid=%d", xuid);
963
- }else{
964
- zLogin = 0;
965
- }
966
- fossil_print("/resetpw/%s %d (%s)\n",
967
- zSuffix, xuid, zLogin ? zLogin : "???");
968
- fossil_free(zSuffix);
969
- fossil_free(zLogin);
965
+ if( g.argc<3 ){
966
+ usage("UID ...");
967
+ }
968
+ for(i=2; i<g.argc; i++){
969
+ uid = atoi(g.argv[i]);
970
+ zSuffix = login_resetpw_suffix(uid, 0);
971
+ xuid = login_resetpw_suffix_is_valid(zSuffix);
972
+ if( xuid>0 ){
973
+ zLogin = db_text(0, "SELECT login FROM user WHERE uid=%d", xuid);
974
+ }else{
975
+ zLogin = 0;
976
+ }
977
+ fossil_print("/resetpw/%s %d (%s)\n",
978
+ zSuffix, xuid, zLogin ? zLogin : "???");
979
+ fossil_free(zSuffix);
980
+ fossil_free(zLogin);
981
+ }
970982
 }
971983
972984
 /*
973985
 ** WEBPAGE: resetpw
974986
 **
@@ -1000,10 +1012,12 @@
10001012
 @ <p><span class="loginError">
10011013
 @ This password-reset URL is invalid, probably because it has expired.
10021014
 @ Password-reset URLs have a short lifespan.
10031015
 @ </span></p>
10041016
 style_finish_page();
1017
+ sleep(1); /* Introduce a small delay on an invalid suffix as an
1018
+ ** extra defense against search attacks */
10051019
 return;
10061020
 }
10071021
 login_set_uid(uid, 0);
10081022
 if( g.perm.Setup || g.perm.Admin || !g.perm.Password || g.zLogin==0 ){
10091023
 @ <p><span class="loginError">
@@ -1934,11 +1948,13 @@
19341948
 @ <p>This project does not allow user self-registration. Please contact the
19351949
 @ project administrator to obtain an account.</p>
19361950
 style_finish_page();
19371951
 return;
19381952
 }
1939
- if( P("pwreset")!=0 ){
1953
+ if( P("pwreset")!=0 && login_self_password_reset_available() ){
1954
+ /* The "Request Password Reset" button was pressed, so render the
1955
+ ** "Request Password Reset" page instead of this one. */
19401956
 login_reqpwreset_page();
19411957
 return;
19421958
 }
19431959
 zPerms = db_get("default-perms", "u");
19441960
@@ -2128,11 +2144,11 @@
21282144
 @ <td><input aria-labelledby="emaddr" type="text" name="ea" \
21292145
 @ value="%h(zEAddr)" size="30"></td>
21302146
 @ </tr>
21312147
 if( iErrLine==3 ){
21322148
 @ <tr><td><td><span class='loginError'>&uarr; %h(zErr)</span>
2133
- if( uid>0 ){
2149
+ if( uid>0 && login_self_password_reset_available() ){
21342150
 @ <br />
21352151
 @ <input type="submit" name="pwreset" \
21362152
 @ value="Request Password Reset For %h(zEAddr)">
21372153
 }
21382154
 @ </td></tr>
@@ -2205,11 +2221,11 @@
22052221
 const char *zErr = 0;
22062222
 int uid = 0; /* User id with the email zEAddr */
22072223
 int captchaIsCorrect = 0; /* True on a correct captcha */
22082224
 char *zCaptcha = ""; /* Value of the captcha text */
22092225
2210
- if( !db_get_boolean("self-pw-reset", 0) || !alert_tables_exist() ){
2226
+ if( !login_self_password_reset_available() ){
22112227
 style_header("Password reset not possible");
22122228
 @ <p>This project does not allow users to reset their own passwords.
22132229
 @ If you need a password reset, you will have to negotiate that directly
22142230
 @ with the project administrator.
22152231
 style_finish_page();
22162232
--- src/login.c
+++ src/login.c
@@ -569,11 +569,13 @@
569 const int noAnon = P("noanon")!=0;
570 int rememberMe; /* If true, use persistent cookie, else
571 session cookie. Toggled per
572 checkbox. */
573
574 if( P("pwreset")!=0 ){
 
 
575 login_reqpwreset_page();
576 return;
577 }
578 login_check_credentials();
579 fossil_redirect_to_https_if_needed(1);
@@ -868,15 +870,15 @@
868 **
869 ** Where UID and TIMESTAMP are the parameters to this function, and HASH
870 ** is constructed from information that is unique to the user in question
871 ** and which is not publicly available. In particular, the HASH includes
872 ** the existing user password. Thus, in order to construct a URL that can
873 ** change a password, the attacker must know the current password, in which
874 ** case that do not need to construct the URL in order to take over the
875 ** account.
876 **
877 ** Return a pointer to the resulting string in memory obtained
878 ** from fossil_malloc().
879 */
880 char *login_resetpw_suffix(int uid, i64 timestamp){
881 char *zHash;
882 char *zInnerSql;
@@ -895,10 +897,15 @@
895 " FROM user WHERE uid=%d", timestamp, uid);
896 }
897 zHash = db_text(0, "SELECT lower(hex(sha3_query(%Q)))", zInnerSql);
898 fossil_free(zInnerSql);
899 zResult = mprintf("%x-%llx-%s", uid, timestamp, zHash);
 
 
 
 
 
900 fossil_free(zHash);
901 return zResult;
902 }
903
904 /*
@@ -911,10 +918,11 @@
911 int i, j;
912 int uid;
913 i64 timestamp;
914 i64 now;
915 char *zHash;
 
916 for(i=0; fossil_isxdigit(zName[i]); i++){}
917 if( i<1 || zName[i]!='-' ) goto not_valid_suffix;
918 for(j=i+1; fossil_isxdigit(zName[j]); j++){}
919 if( j<=i+1 || zName[j]!='-' ) goto not_valid_suffix;
920 uid = strtol(zName, 0, 16);
@@ -932,43 +940,47 @@
932 }
933 fossil_free(zHash);
934 return uid;
935
936 not_valid_suffix:
937 sleep(2); /* Introduce a small delay on an invalid suffix as an
938 ** extra defense against search attacks */
939 return 0;
940 }
941
942 /*
943 ** COMMAND: test-resetpw-url
944 ** Usage: fossil test-resetpw-url UID
945 **
946 ** Generate and verify a /resetpw URL for user UID.
 
 
 
947 */
948 void test_resetpw_url(void){
949 char *zSuffix;
950 int uid;
951 int xuid;
952 char *zLogin;
 
953 db_find_and_open_repository(0, 0);
954 verify_all_options();
955 if( g.argc!=3 ){
956 usage("UID");
957 }
958 uid = atoi(g.argv[2]);
959 zSuffix = login_resetpw_suffix(uid, 0);
960 xuid = login_resetpw_suffix_is_valid(zSuffix);
961 if( xuid>0 ){
962 zLogin = db_text(0, "SELECT login FROM user WHERE uid=%d", xuid);
963 }else{
964 zLogin = 0;
965 }
966 fossil_print("/resetpw/%s %d (%s)\n",
967 zSuffix, xuid, zLogin ? zLogin : "???");
968 fossil_free(zSuffix);
969 fossil_free(zLogin);
 
 
970 }
971
972 /*
973 ** WEBPAGE: resetpw
974 **
@@ -1000,10 +1012,12 @@
1000 @ <p><span class="loginError">
1001 @ This password-reset URL is invalid, probably because it has expired.
1002 @ Password-reset URLs have a short lifespan.
1003 @ </span></p>
1004 style_finish_page();
 
 
1005 return;
1006 }
1007 login_set_uid(uid, 0);
1008 if( g.perm.Setup || g.perm.Admin || !g.perm.Password || g.zLogin==0 ){
1009 @ <p><span class="loginError">
@@ -1934,11 +1948,13 @@
1934 @ <p>This project does not allow user self-registration. Please contact the
1935 @ project administrator to obtain an account.</p>
1936 style_finish_page();
1937 return;
1938 }
1939 if( P("pwreset")!=0 ){
 
 
1940 login_reqpwreset_page();
1941 return;
1942 }
1943 zPerms = db_get("default-perms", "u");
1944
@@ -2128,11 +2144,11 @@
2128 @ <td><input aria-labelledby="emaddr" type="text" name="ea" \
2129 @ value="%h(zEAddr)" size="30"></td>
2130 @ </tr>
2131 if( iErrLine==3 ){
2132 @ <tr><td><td><span class='loginError'>&uarr; %h(zErr)</span>
2133 if( uid>0 ){
2134 @ <br />
2135 @ <input type="submit" name="pwreset" \
2136 @ value="Request Password Reset For %h(zEAddr)">
2137 }
2138 @ </td></tr>
@@ -2205,11 +2221,11 @@
2205 const char *zErr = 0;
2206 int uid = 0; /* User id with the email zEAddr */
2207 int captchaIsCorrect = 0; /* True on a correct captcha */
2208 char *zCaptcha = ""; /* Value of the captcha text */
2209
2210 if( !db_get_boolean("self-pw-reset", 0) || !alert_tables_exist() ){
2211 style_header("Password reset not possible");
2212 @ <p>This project does not allow users to reset their own passwords.
2213 @ If you need a password reset, you will have to negotiate that directly
2214 @ with the project administrator.
2215 style_finish_page();
2216
--- src/login.c
+++ src/login.c
@@ -569,11 +569,13 @@
569 const int noAnon = P("noanon")!=0;
570 int rememberMe; /* If true, use persistent cookie, else
571 session cookie. Toggled per
572 checkbox. */
573
574 if( P("pwreset")!=0 && login_self_password_reset_available() ){
575 /* If the "Reset Password" button in the form was pressed, render
576 ** the Request Password Reset page in place of this one. */
577 login_reqpwreset_page();
578 return;
579 }
580 login_check_credentials();
581 fossil_redirect_to_https_if_needed(1);
@@ -868,15 +870,15 @@
870 **
871 ** Where UID and TIMESTAMP are the parameters to this function, and HASH
872 ** is constructed from information that is unique to the user in question
873 ** and which is not publicly available. In particular, the HASH includes
874 ** the existing user password. Thus, in order to construct a URL that can
875 ** change a password, an attacker must know the current password, in which
876 ** case the attacker does not need to construct the URL in order to take
877 ** over the account.
878 **
879 ** Return a pointer to the resulting string in memory obtained
880 ** from fossil_malloc().
881 */
882 char *login_resetpw_suffix(int uid, i64 timestamp){
883 char *zHash;
884 char *zInnerSql;
@@ -895,10 +897,15 @@
897 " FROM user WHERE uid=%d", timestamp, uid);
898 }
899 zHash = db_text(0, "SELECT lower(hex(sha3_query(%Q)))", zInnerSql);
900 fossil_free(zInnerSql);
901 zResult = mprintf("%x-%llx-%s", uid, timestamp, zHash);
902 if( strlen(zHash)<64 || strlen(zResult)<70 ){
903 /* This should never happen, but if it does, we don't want it to lead
904 ** to a security breach. */
905 fossil_panic("insecure password reset hash generated\n");
906 }
907 fossil_free(zHash);
908 return zResult;
909 }
910
911 /*
@@ -911,10 +918,11 @@
918 int i, j;
919 int uid;
920 i64 timestamp;
921 i64 now;
922 char *zHash;
923 if( zName==0 || strlen(zName)<70 ) goto not_valid_suffix;
924 for(i=0; fossil_isxdigit(zName[i]); i++){}
925 if( i<1 || zName[i]!='-' ) goto not_valid_suffix;
926 for(j=i+1; fossil_isxdigit(zName[j]); j++){}
927 if( j<=i+1 || zName[j]!='-' ) goto not_valid_suffix;
928 uid = strtol(zName, 0, 16);
@@ -932,43 +940,47 @@
940 }
941 fossil_free(zHash);
942 return uid;
943
944 not_valid_suffix:
 
 
945 return 0;
946 }
947
948 /*
949 ** COMMAND: test-resetpw-url
950 ** Usage: fossil test-resetpw-url UID
951 **
952 ** Generate and verify a /resetpw URL for user UID.
953 **
954 ** This command is intended for unit testing the login_resetpw_suffix()
955 ** and login_resetpw_suffix_is_valid() functions.
956 */
957 void test_resetpw_url(void){
958 char *zSuffix;
959 int uid;
960 int xuid;
961 char *zLogin;
962 int i;
963 db_find_and_open_repository(0, 0);
964 verify_all_options();
965 if( g.argc<3 ){
966 usage("UID ...");
967 }
968 for(i=2; i<g.argc; i++){
969 uid = atoi(g.argv[i]);
970 zSuffix = login_resetpw_suffix(uid, 0);
971 xuid = login_resetpw_suffix_is_valid(zSuffix);
972 if( xuid>0 ){
973 zLogin = db_text(0, "SELECT login FROM user WHERE uid=%d", xuid);
974 }else{
975 zLogin = 0;
976 }
977 fossil_print("/resetpw/%s %d (%s)\n",
978 zSuffix, xuid, zLogin ? zLogin : "???");
979 fossil_free(zSuffix);
980 fossil_free(zLogin);
981 }
982 }
983
984 /*
985 ** WEBPAGE: resetpw
986 **
@@ -1000,10 +1012,12 @@
1012 @ <p><span class="loginError">
1013 @ This password-reset URL is invalid, probably because it has expired.
1014 @ Password-reset URLs have a short lifespan.
1015 @ </span></p>
1016 style_finish_page();
1017 sleep(1); /* Introduce a small delay on an invalid suffix as an
1018 ** extra defense against search attacks */
1019 return;
1020 }
1021 login_set_uid(uid, 0);
1022 if( g.perm.Setup || g.perm.Admin || !g.perm.Password || g.zLogin==0 ){
1023 @ <p><span class="loginError">
@@ -1934,11 +1948,13 @@
1948 @ <p>This project does not allow user self-registration. Please contact the
1949 @ project administrator to obtain an account.</p>
1950 style_finish_page();
1951 return;
1952 }
1953 if( P("pwreset")!=0 && login_self_password_reset_available() ){
1954 /* The "Request Password Reset" button was pressed, so render the
1955 ** "Request Password Reset" page instead of this one. */
1956 login_reqpwreset_page();
1957 return;
1958 }
1959 zPerms = db_get("default-perms", "u");
1960
@@ -2128,11 +2144,11 @@
2144 @ <td><input aria-labelledby="emaddr" type="text" name="ea" \
2145 @ value="%h(zEAddr)" size="30"></td>
2146 @ </tr>
2147 if( iErrLine==3 ){
2148 @ <tr><td><td><span class='loginError'>&uarr; %h(zErr)</span>
2149 if( uid>0 && login_self_password_reset_available() ){
2150 @ <br />
2151 @ <input type="submit" name="pwreset" \
2152 @ value="Request Password Reset For %h(zEAddr)">
2153 }
2154 @ </td></tr>
@@ -2205,11 +2221,11 @@
2221 const char *zErr = 0;
2222 int uid = 0; /* User id with the email zEAddr */
2223 int captchaIsCorrect = 0; /* True on a correct captcha */
2224 char *zCaptcha = ""; /* Value of the captcha text */
2225
2226 if( !login_self_password_reset_available() ){
2227 style_header("Password reset not possible");
2228 @ <p>This project does not allow users to reset their own passwords.
2229 @ If you need a password reset, you will have to negotiate that directly
2230 @ with the project administrator.
2231 style_finish_page();
2232

Keyboard Shortcuts

Open search /
Next entry (timeline) j
Previous entry (timeline) k
Open focused entry Enter
Show this help ?
Toggle theme Top nav button