e81efb1074d9786436ed8cdb626fc3bdeb6ac14c max Thu Aug 6 09:20:30 2026 -0700 hgLogin: fix the social-login/email-link code review issues from #38008 #Preview2 week - bugs introduced now will need a build patch to fix Brian, thanks for the thorough review - every one of these was real. Here is what I changed for the six items that stayed on the ticket (the pre-existing XSS and the site-wide httpsCertCheck default went to #38011 and #38012). 1. Reflected XSS in the account chooser and the other new pages. Every address and username now goes through htmlEncode() before it lands in the HTML or an attribute (chooseAccountPage, completeAccountPage, emailLinkPage, changeEmailPage and the confirmation pages). I also gated the email-link side of the chooser on emailLinkEnabled(), so your chooseAccount + emailLogin_email=<img ...> URL now renders the tag as text and does nothing at all where the feature is off. 2. Registering someone else's address to steal their social login. The two OAuth email-match queries (resolveIdentity and chooseAccount) now require accountActivated='Y', so an unactivated row someone planted with a victim's address can no longer be matched or linked. completeAccount only marks the new account activated when the provider actually verified the address and the user kept it; otherwise it creates the account inactive and sends the usual confirmation mail, so an unverified address can never be planted as a trusted one. 3. OAuth requests not enforcing the server certificate. Rather than poke the env var, I added a small library knob, httpsSetCertCheck() in lib/https.c, that pins the cert-check mode for the rest of the process and is not overwritten by openSslInit() or hg.conf. hgLogin's httpRequest() calls it with "abort", so those requests refuse a bad certificate no matter how the site is configured, and it no longer depends on being the first HTTPS connection. 4. The pending-identity signature. It now also covers the hguid (which survives the provider redirect, unlike the hgsid) and the time it was minted, with a 15-minute expiry, and it is cleared on the failure paths too. A signature that leaks into a saved or shared session is now useless to another browser and dies quickly anyway. 5. changeEmail. It now asks for the current password where the account has one, and it no longer changes the address on the spot - it emails a one-time signed confirmation link to the new address and only applies the change when that link is opened. When the change lands it also mails the OLD address to say the account's email was changed and who to contact if that wasn't them, so a hijack gets noticed. One honest caveat: an account with no password (social-only) still can't be re-checked before the change, so a stolen cookie could still start it - but the old-address alert now gives the owner a way to catch it. Expiring login cookies is the deeper fix and feels like its own ticket. 6. isalnum() on a signed char in suggestUsername - now cast to unsigned char. Build is clean, no new warnings. Set back to you for another look. refs #38008 diff --git src/hg/hgLogin/hgLogin.c src/hg/hgLogin/hgLogin.c index db2f1aede6e..3445e3459d7 100644 --- src/hg/hgLogin/hgLogin.c +++ src/hg/hgLogin/hgLogin.c @@ -1,26 +1,27 @@ /* hgLogin - Administer UCSC Genome Browser membership - signup, lost password, etc. */ /* Copyright (C) 2014 The Regents of the University of California * See kent/LICENSE or http://genome.ucsc.edu/license/ for licensing information. */ #include <openssl/evp.h> #include <openssl/opensslv.h> #include <openssl/md5.h> #include "common.h" #include "hash.h" +#include "portable.h" #include "hmac.h" #include "obscure.h" #include "hgConfig.h" #include "cheapcgi.h" #include "memalloc.h" #include "jksql.h" #include "htmshell.h" #include "cart.h" #include "hPrint.h" #include "hdb.h" #include "hui.h" #include "web.h" #include "ra.h" #include "hgColors.h" #include "net.h" @@ -34,31 +35,32 @@ #include "autoUpgrade.h" #include "hCommon.h" #include "botDelay.h" #include "errCatch.h" #define EMAILSEP ";" /* ---- Global variables. ---- */ char msg[4096] = ""; char *incorrectUsernameOrPassword="The username or password you entered is incorrect."; char *incorrectUsername="The username you entered is incorrect."; /* The excludeVars are not saved to the cart. */ char *excludeVars[] = { "submit", "Submit", "debug", "fixMembers", "update", "hgLogin_password", "hgLogin_password2", "hgLogin_newPassword1", "hgLogin_newPassword2", "hgLogin_newEmail1", "hgLogin_newEmail2", - "code", "state", "provider", "user", "token", NULL }; + "hgLogin_curPassword", "code", "state", "provider", "user", "token", + "newEmail", "exp", "sig", NULL }; struct cart *cart; /* This holds cgi and other variables between clicks. */ char *database; /* Name of genome database - hg15, mm3, or the like. */ struct hash *oldCart; /* Old cart hash. */ char *errMsg = NULL; /* Error message to show user when form data rejected */ char brwName[64]; char brwAddr[256]; char signature[256]; char returnAddr[256]; char *hgLoginUrl = NULL; /* full absolute URL to hgLogin as seen from browser, e.g. http://genome.ucsc.edu/cgi-bin/hgLogin. Can be a relative URL /cgi-bin/hgLogin if hg.conf login.relativeLink is on. */ boolean pwdEyeIconEnabled = TRUE; /* show/hide eye icon on password fields; set from hg.conf login.pwdEyeIcon in doMiddle() */ /* for earlyBotCheck() function at the beginning of main() */ @@ -1011,60 +1013,129 @@ hPrintf("<h2>%s</h2>", brwName); hPrintf( "<p align=\"left\">" "</p>" "<h3>Password has been changed.</h3>"); cartRemove(cart, "hgLogin_password"); cartRemove(cart, "hgLogin_newPassword1"); cartRemove(cart, "hgLogin_newPassword2"); sqlSafef(query,sizeof(query),"SELECT * FROM gbMembers WHERE userName='%s'", user); struct gbMembers *m = gbMembersLoadByQuery(conn, query); struct dyString *cookieJS = getLoginCookieJS(user, m->idx); jsInline(cookieJS->string); returnToURL(150); } +static char *changeEmailSig(char *user, char *newEmail, char *expStr) +/* HMAC-MD5 over a pending email change, keyed by the secret login.cookieSalt. It goes in the + * confirmation link so that clicking the link -- and only clicking it -- applies the change, + * proving the new address really reaches the requester. Result is allocd. */ +{ +char *salt = cfgOption(CFG_LOGIN_COOKIE_SALT); +if (isEmpty(salt)) + errAbort("Confirming an email change requires %s in hg.conf, set to a secret random " + "string. Without a secret we cannot sign the confirmation link.", CFG_LOGIN_COOKIE_SALT); +char buf[1024]; +safef(buf, sizeof(buf), "changeEmail|%s|%s|%s", + emptyForNull(user), emptyForNull(newEmail), emptyForNull(expStr)); +return hmacMd5(salt, buf); +} + +static void sendChangeEmailConfirmMail(char *newEmail, char *user) +/* Email a one-time link to newEmail that, when opened, changes user's address to newEmail. */ +{ +char expStr[32]; +safef(expStr, sizeof(expStr), "%ld", clock1() + 3600); // link good for one hour +char *sig = changeEmailSig(user, newEmail, expStr); +char url[1024]; +safef(url, sizeof(url), + "%s?hgLogin.do.confirmChangeEmail=1&user=%s&newEmail=%s&exp=%s&sig=%s", + hgLoginUrl, cgiEncode(user), cgiEncode(newEmail), expStr, sig); +char subject[256]; +safef(subject, sizeof(subject), "Confirm your new %s email address", brwName); +char *remoteAddr = getenv("REMOTE_ADDR"); +char message[4096]; +safef(message, sizeof(message), + "Someone (probably you, from IP address %s) asked to change the email address on the %s " + "account \"%s\" to this address.\nTo confirm the change, open this link in your browser:\n\n" + "%s\n\nThe link works once and expires in one hour. If you did not request this, you can " + "safely ignore this email and your address will stay as it is.\n\n%s\n%s", + emptyForNull(remoteAddr), brwName, user, url, signature, returnAddr); +sendActMailOut(newEmail, subject, message); +freeMem(sig); +} + +static void sendChangeEmailAlertMail(char *oldEmail, char *user, char *newEmail) +/* Tell the OLD address that the account's email was just changed, so its owner finds out if the + * change was not theirs and can ask us to undo it. This is the notice that protects the current + * owner -- confirming the new address only proves the new mailbox is reachable. */ +{ +char subject[256]; +safef(subject, sizeof(subject), "Your %s email address was changed", brwName); +char *remoteAddr = getenv("REMOTE_ADDR"); +char message[4096]; +safef(message, sizeof(message), + "The email address on the %s account \"%s\" was just changed to %s (request from IP address " + "%s).\n\nIf you made this change, nothing more is needed. If you did NOT, please reply to " + "this message right away so we can help you secure the account.\n\n%s\n%s", + brwName, user, newEmail, emptyForNull(remoteAddr), signature, returnAddr); +sendActMailOut(oldEmail, subject, message); +} + void changeEmailPage(struct sqlConnection *conn) /* Draw the change-email page for the currently logged-in user. The account is taken from * the validated login cookie (wikiLinkUserName), never from a form field, so a user can only - * change their own email. Being logged in is the authorization; no password is required, - * which also lets social-login accounts (which have no password) change their email. */ + * change their own email. Where the account has a password we also ask for it here, so a + * borrowed login cookie alone cannot change the address (and from there take over the account + * via password recovery). Social-login accounts have no password and are asked for none; for + * them the new address is instead confirmed by email before it takes effect (see changeEmail). */ { if (!emailLinkEnabled()) { displayLoginPage(conn); return; } char *user = wikiLinkUserName(); if (isEmpty(user)) { freez(&errMsg); errMsg = cloneString("Please log in first to change your email address."); displayLoginPage(conn); return; } char query[256]; sqlSafef(query, sizeof(query), "SELECT email FROM gbMembers WHERE userName='%s'", user); char *curEmail = sqlQuickString(conn, query); +sqlSafef(query, sizeof(query), "SELECT password FROM gbMembers WHERE userName='%s'", user); +boolean hasPassword = isNotEmpty(sqlQuickString(conn, query)); +char *encUser = htmlEncode(user); +char *encCurEmail = htmlEncode(isNotEmpty(curEmail) ? curEmail : "(none)"); hPrintf("<div id=\"changeEmailBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<h3>Change Email</h3>"); hPrintf("<p><span style='color:red;'>%s</span></p>", errMsg ? errMsg : ""); hPrintf("<form method=\"post\" action=\"%s\" name=\"changeEmailForm\">", hgLoginUrl); hPrintf("<p>Signed in as <b>%s</b>.<br>Current email address: <b>%s</b></p>", - user, isNotEmpty(curEmail) ? curEmail : "(none)"); + encUser, encCurEmail); +freeMem(encUser); +freeMem(encCurEmail); +if (hasPassword) + hPrintf("<div class=\"inputGroup\">" + "<label for=\"curPassword\">Current password</label>" + "<input type=\"password\" name=\"hgLogin_curPassword\" value=\"\" size=\"30\" id=\"curPassword\">" + "</div>"); hPrintf("<div class=\"inputGroup\">" "<label for=\"newEmail1\">New email address</label>" "<input type=\"text\" name=\"hgLogin_newEmail1\" value=\"\" size=\"30\" id=\"newEmail1\">" "</div>"); hPrintf("<div class=\"inputGroup\">" "<label for=\"newEmail2\">Re-enter new email address</label>" "<input type=\"text\" name=\"hgLogin_newEmail2\" value=\"\" size=\"30\" id=\"newEmail2\">" "</div>"); hPrintf("<div class=\"formControls\">" "<input type=\"submit\" name=\"hgLogin.do.changeEmail\" value=\"Change Email\" class=\"largeButton\">" " <a href=\"%s\" class=\"cancelButton\">Cancel</a>" "</div></form></div><!-- END - changeEmailBox -->", getReturnToURL()); cartSaveSession(cart); } @@ -1088,39 +1159,109 @@ char *email2 = cartUsualString(cart, "hgLogin_newEmail2", ""); if (isEmpty(email1) || spc_email_isvalid(email1) == 0) { freez(&errMsg); errMsg = cloneString("Please enter a valid email address."); changeEmailPage(conn); return; } if (differentString(email1, email2)) { freez(&errMsg); errMsg = cloneString("Email addresses do not match."); changeEmailPage(conn); return; } +/* Re-authenticate where we can: if the account has a password, require the current one. A + * stolen login cookie by itself must not be enough to change the address. */ char query[512]; -sqlSafef(query, sizeof(query), - "UPDATE gbMembers SET email='%s', lastUse=NOW() WHERE userName='%s'", email1, user); -sqlUpdate(conn, query); +sqlSafef(query, sizeof(query), "SELECT password FROM gbMembers WHERE userName='%s'", user); +char *curPwd = sqlQuickString(conn, query); +if (isNotEmpty(curPwd)) + { + char *given = cartUsualString(cart, "hgLogin_curPassword", ""); + if (isEmpty(given) || !checkPwd(given, curPwd)) + { + freez(&errMsg); + errMsg = cloneString("Please enter your current password."); + changeEmailPage(conn); + return; + } + } +/* Do not change the address yet: email a one-time confirmation link to the NEW address and + * apply the change only when it is clicked (see confirmChangeEmail). This proves the address + * is real and controlled by the requester, so an unconfirmed address cannot silently become + * the account's recovery address. */ +sendChangeEmailConfirmMail(email1, user); cartRemove(cart, "hgLogin_newEmail1"); cartRemove(cart, "hgLogin_newEmail2"); +cartRemove(cart, "hgLogin_curPassword"); +char *encEmail = htmlEncode(email1); +hPrintf("<div class=\"centeredContainer formBox\"><h2>%s</h2>", brwName); +hPrintf("<h3>Almost done — please check your email</h3>"); +hPrintf("<p>We sent a confirmation link to <b>%s</b>. Open the link in that message to finish " + "changing your email address. The link works once and expires in one hour.</p></div>", + encEmail); +freeMem(encEmail); +returnToURL(3000); +} + +void confirmChangeEmail(struct sqlConnection *conn) +/* Apply a confirmed email change. Reached by opening the signed link sent to the new address + * (see sendChangeEmailConfirmMail); the signature and its expiry are the authorization, so this + * does not require a login cookie -- the link may be opened from the new mailbox in any browser. */ +{ +if (!emailLinkEnabled()) + { + displayLoginPage(conn); + return; + } +char *user = cgiUsualString("user", ""); +char *newEmail = cgiUsualString("newEmail", ""); +char *expStr = cgiUsualString("exp", ""); +char *sig = cgiUsualString("sig", ""); +char *expected = changeEmailSig(user, newEmail, expStr); +boolean sigOk = isNotEmpty(sig) && sameString(sig, expected); +freeMem(expected); +if (!sigOk || isEmpty(user) || spc_email_isvalid(newEmail) == 0) + { + freez(&errMsg); + errMsg = cloneString("This confirmation link is not valid."); + displayLoginPage(conn); + return; + } +if (clock1() > atol(expStr)) + { + freez(&errMsg); + errMsg = cloneString("This confirmation link has expired. Please request the change again."); + displayLoginPage(conn); + return; + } +char query[512]; +sqlSafef(query, sizeof(query), "SELECT email FROM gbMembers WHERE userName='%s'", user); +char *oldEmail = sqlQuickString(conn, query); +sqlSafef(query, sizeof(query), + "UPDATE gbMembers SET email='%s', lastUse=NOW() WHERE userName='%s'", newEmail, user); +sqlUpdate(conn, query); +/* Alert the previous address that the change happened, so a hijack is noticed. */ +if (isNotEmpty(oldEmail) && differentWord(oldEmail, newEmail)) + sendChangeEmailAlertMail(oldEmail, user, newEmail); +char *encEmail = htmlEncode(newEmail); hPrintf("<div class=\"centeredContainer formBox\"><h2>%s</h2>", brwName); hPrintf("<h3>Your email address has been changed.</h3>"); -hPrintf("<p>Your email address is now <b>%s</b>.</p></div>", email1); +hPrintf("<p>Your email address is now <b>%s</b>.</p></div>", encEmail); +freeMem(encEmail); returnToURL(1500); } void signupPage(struct sqlConnection *conn) /* draw the signup page */ { hPrintf("<div id=\"signUpBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf( "<p>Signing up enables you to save multiple sessions, share your sessions with others via short and stable session links and manage previously uploaded custom tracks and track hubs.</p>" "\n"); hPrintf("<p>Already have an account? " "<a href=\"%s?hgLogin.do.displayLoginPage=1\">Go to the login page</a>.</p>", hgLoginUrl); printSocialButtons(FALSE, TRUE, "Sign up"); hPrintf("<h3>Sign Up Using Email</h3>" @@ -1636,172 +1777,198 @@ { char raw[256]; raw[0] = 0; if (isNotEmpty(email) && strchr(email, '@')) { safecpy(raw, sizeof(raw), email); char *at = strchr(raw, '@'); *at = 0; } else if (isNotEmpty(displayName)) safecpy(raw, sizeof(raw), displayName); char clean[256]; int j = 0; char *s; for (s = raw; *s != 0 && j < (int)sizeof(clean)-1; s++) - if (isalnum(*s) || *s == '_' || *s == '-') + if (isalnum((unsigned char)*s) || *s == '_' || *s == '-') clean[j++] = tolower((unsigned char)*s); clean[j] = 0; if (strlen(clean) < 2) safecpy(clean, sizeof(clean), "user"); char candidate[288]; safecpy(candidate, sizeof(candidate), clean); int n = 1; while (userNameTaken(conn, candidate)) safef(candidate, sizeof(candidate), "%s%d", clean, ++n); return cloneString(candidate); } static struct gbMembers *memberForIdentity(struct sqlConnection *conn, struct oauthIdentity *id) /* Return the gbMembers account already linked to this provider identity, or NULL. */ { char query[512]; sqlSafef(query, sizeof(query), "SELECT idx FROM gbMemberIdentity WHERE provider='%s' AND subject='%s'", id->provider, id->subject); uint idx = (uint)sqlQuickLongLong(conn, query); if (idx == 0) return NULL; sqlSafef(query, sizeof(query), "SELECT * FROM gbMembers WHERE idx=%u", idx); return gbMembersLoadByQuery(conn, query); } -static char *oauthPendingSig(char *provider, char *subject, char *email) +/* A pending social identity is good for this many seconds -- long enough to choose a username + * or an account, short enough that a leaked signature is quickly useless. */ +#define OAUTH_PENDING_TTL 900 + +static char *oauthPendingSig(char *provider, char *subject, char *email, char *timeStr) /* HMAC-MD5 over a pending social identity, keyed by the secret login.cookieSalt. Only * resolveIdentity (which runs after a genuine provider verification) can produce a valid one, - * so a pending identity injected through cart/CGI variables will not validate. (Not bound to - * the session id: the hgsid is regenerated across the provider redirect, so a session-bound - * signature would never match on the way back.) Result is allocd. */ + * so a pending identity injected through cart/CGI variables will not validate. The signature + * also covers this browser's hguid (cart->userId, which -- unlike the hgsid -- survives the + * provider redirect) and the time it was minted, so a signature that leaks into a saved or + * shared session cannot be replayed by a different browser or after it expires (see + * pendingIdentityValid). Result is allocd. */ { char *salt = cfgOption(CFG_LOGIN_COOKIE_SALT); if (isEmpty(salt)) errAbort("Signing in with an external identity provider requires %s in hg.conf, set to a " "secret random string. Without a secret we cannot sign the pending identity, and the " "account chooser would accept a forged one.", CFG_LOGIN_COOKIE_SALT); char buf[1024]; -safef(buf, sizeof(buf), "%s|%s|%s", - emptyForNull(provider), emptyForNull(subject), emptyForNull(email)); +safef(buf, sizeof(buf), "%s|%s|%s|%s|%s", + emptyForNull(provider), emptyForNull(subject), emptyForNull(email), + emptyForNull(cart->userId), emptyForNull(timeStr)); return hmacMd5(salt, buf); } static boolean pendingIdentityValid() -/* TRUE only if the pending-identity cart variables carry a signature we minted this session. - * Guards the OAuth chooser and completeAccount against forged/injected pending identities. */ +/* TRUE only if the pending-identity cart variables carry a signature we minted, for this + * browser, within the last OAUTH_PENDING_TTL seconds. Guards the OAuth chooser and + * completeAccount against forged, injected, replayed, or stale pending identities. */ { char *sig = cartUsualString(cart, "oauth_pending_sig", ""); -if (isEmpty(sig)) +char *timeStr = cartUsualString(cart, "oauth_pending_time", ""); +if (isEmpty(sig) || isEmpty(timeStr)) + return FALSE; +if (clock1() - atol(timeStr) > OAUTH_PENDING_TTL) return FALSE; char *expected = oauthPendingSig(cartUsualString(cart, "oauth_pending_provider", ""), cartUsualString(cart, "oauth_pending_subject", ""), - cartUsualString(cart, "oauth_pending_email", "")); + cartUsualString(cart, "oauth_pending_email", ""), + timeStr); boolean ok = sameString(sig, expected); freeMem(expected); return ok; } static void setPendingIdentity(struct oauthIdentity *id) /* Stash an authenticated-but-not-yet-linked identity in the cart so it survives a form * round-trip (the "choose a username" or "choose an account" page). The signature is what - * proves, on the way back, that we really verified this identity. */ + * proves, on the way back, that we really verified this identity, for this browser, recently. */ { +char timeStr[32]; +safef(timeStr, sizeof(timeStr), "%ld", clock1()); cartSetString(cart, "oauth_pending_provider", id->provider); cartSetString(cart, "oauth_pending_subject", id->subject); cartSetString(cart, "oauth_pending_email", emptyForNull(id->email)); +cartSetString(cart, "oauth_pending_email_verified", id->emailVerified ? "1" : "0"); cartSetString(cart, "oauth_pending_name", emptyForNull(id->displayName)); +cartSetString(cart, "oauth_pending_time", timeStr); cartSetString(cart, "oauth_pending_sig", - oauthPendingSig(id->provider, id->subject, emptyForNull(id->email))); + oauthPendingSig(id->provider, id->subject, emptyForNull(id->email), timeStr)); } static void clearPendingIdentity() -/* Remove the pending-identity cart variables once the account is linked. */ +/* Remove the pending-identity cart variables. Call this on every path that finishes with the + * pending identity -- success or definitive failure -- so a stale signature is not left behind + * in the cart to be swept into a saved session. */ { cartRemove(cart, "oauth_pending_provider"); cartRemove(cart, "oauth_pending_subject"); cartRemove(cart, "oauth_pending_email"); +cartRemove(cart, "oauth_pending_email_verified"); cartRemove(cart, "oauth_pending_name"); +cartRemove(cart, "oauth_pending_time"); cartRemove(cart, "oauth_pending_sig"); } static void linkIdentity(struct sqlConnection *conn, uint idx, struct oauthIdentity *id) /* Insert or refresh the gbMemberIdentity row linking idx to this provider identity. */ { char query[1024]; char *email = emptyForNull(id->email); sqlSafef(query, sizeof(query), "INSERT INTO gbMemberIdentity SET idx=%u, provider='%s', subject='%s', email='%s', " "created=NOW(), lastUse=NOW() " "ON DUPLICATE KEY UPDATE idx=%u, email='%s', lastUse=NOW()", idx, id->provider, id->subject, email, idx, email); sqlUpdate(conn, query); } void completeAccountPage(struct sqlConnection *conn) /* Ask a first-time social-login user to confirm a username (and email) for a new account. */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char *email = cartUsualString(cart, "oauth_pending_email", ""); char *name = cartUsualString(cart, "oauth_pending_name", ""); if (isEmpty(provider) || !pendingIdentityValid()) { + clearPendingIdentity(); displayLoginPage(conn); return; } char *suggested = cartUsualString(cart, "hgLogin_userName", ""); if (isEmpty(suggested)) suggested = suggestUsername(conn, email, name); +char *encSuggested = htmlEncode(suggested); // both go into value="" attributes; escape (XSS) +char *encEmail = htmlEncode(email); hPrintf("<div id=\"completeAccountBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<h3>Choose a username</h3>"); hPrintf("<p>You signed in with %s. Pick a username for your new %s account. " "You can change the suggested name below.</p>", oauthProviderLabel(provider), brwName); printUsernameNote(); hPrintf("<span style='color:red;'>%s</span>", errMsg ? errMsg : ""); hPrintf("<form method=\"post\" action=\"%s\" name=\"completeAccountForm\">", hgLoginUrl); hPrintf("<div class=\"inputGroup\">" "<label for=\"userName\">Username</label>" "<input type=\"text\" name=\"hgLogin_userName\" value=\"%s\" size=\"30\" id=\"userName\">" - "</div>", suggested); + "</div>", encSuggested); hPrintf("<div class=\"inputGroup\">" "<label for=\"emailAddr\">Email address</label>" "<input type=\"text\" name=\"hgLogin_email\" value=\"%s\" size=\"30\" id=\"emailAddr\">" - "</div>", email); + "</div>", encEmail); hPrintf("<div class=\"formControls\">" "<input type=\"submit\" name=\"hgLogin.do.completeAccount\" value=\"Create Account\" class=\"largeButton\">" " <a href=\"%s\" class=\"cancelButton\">Cancel</a>" "</div></form></div><!-- END - completeAccountBox -->", getReturnToURL()); cartSaveSession(cart); +freeMem(encSuggested); +freeMem(encEmail); } void completeAccount(struct sqlConnection *conn) /* Create the account for a first-time social-login user, link the identity, and log in. */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char *subject = cartUsualString(cart, "oauth_pending_subject", ""); if (isEmpty(provider) || isEmpty(subject) || !pendingIdentityValid()) { + clearPendingIdentity(); freez(&errMsg); errMsg = cloneString("Your login session expired. Please sign in again."); displayLoginPage(conn); return; } char *user = cartUsualString(cart, "hgLogin_userName", ""); char *encUserName = cgiEncodeFull(user); if (isEmpty(user)) { freez(&errMsg); errMsg = cloneString("Please enter a username."); completeAccountPage(conn); return; } if (strlen(user) < 2) @@ -1831,119 +1998,148 @@ freez(&errMsg); errMsg = cloneString("Please enter an email address."); completeAccountPage(conn); return; } if (spc_email_isvalid(email) == 0) { freez(&errMsg); errMsg = cloneString("Invalid email address format."); completeAccountPage(conn); return; } char *name = cartUsualString(cart, "oauth_pending_name", ""); char *realName = isNotEmpty(name) ? name : user; +/* The new account is created "activated" -- its email trusted for future auto-linking (see + * resolveIdentity) -- only when the provider actually verified this address and the user kept + * it unchanged. If the address is unverified (the provider released none, e.g. ORCID, or the + * user typed a different one), create the account inactive and send the usual confirmation + * mail, so an unverified address can never be planted as a trusted one. The user still signs + * in now either way: their provider identity, not the email, is what logs them in. */ +char *verifiedEmail = cartUsualString(cart, "oauth_pending_email", ""); +boolean emailVerified = cartUsualBoolean(cart, "oauth_pending_email_verified", FALSE) + && isNotEmpty(verifiedEmail) && sameString(email, verifiedEmail); + struct dyString *q = sqlDyStringCreate( "INSERT INTO gbMembers SET userName='%s', realName='%s', password='', email='%s', " - "lastUse=NOW(), dateActivated=NOW(), accountActivated='Y'", - user, realName, emptyForNull(email)); + "lastUse=NOW(), dateActivated=NOW(), accountActivated='%s'", + user, realName, emptyForNull(email), emailVerified ? "Y" : "N"); sqlUpdate(conn, dyStringContents(q)); dyStringFree(&q); uint idx = sqlLastAutoId(conn); struct oauthIdentity pending; ZeroVar(&pending); pending.provider = provider; pending.subject = subject; pending.email = email; linkIdentity(conn, idx, &pending); clearPendingIdentity(); +if (!emailVerified) + setupNewAccount(conn, email, user); // send confirmation mail for the unverified address loginAndReturn(user, idx); } void chooseAccountPage(struct sqlConnection *conn) /* Ask the user which of several accounts sharing an email address to sign in to. Used by * two flows: OAuth (oauth_pending_* in the cart -> the chosen account is linked to the social * identity) and the passwordless email link (emailLogin_* in the cart -> just sign in). */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); boolean emailMode = isEmpty(provider); +if (emailMode && !emailLinkEnabled()) + { + // The email-link chooser must not run where passwordless login is switched off. + displayLoginPage(conn); + return; + } char *email = emailMode ? cartUsualString(cart, "emailLogin_email", "") : cartUsualString(cart, "oauth_pending_email", ""); if (isEmpty(email) || (!emailMode && !pendingIdentityValid())) { + if (!emailMode) + clearPendingIdentity(); displayLoginPage(conn); return; } +char *encEmail = htmlEncode(email); // the address is displayed; never trust it raw (XSS) char query[512]; if (emailMode) // Only the accounts that hold the just-validated login token, matching what emailLogin saw. sqlSafef(query, sizeof(query), "SELECT * FROM gbMembers WHERE (email='%s' OR recovEmail='%s') AND loginToken='%s' " "AND loginToken<>'' AND loginTokenExpires > NOW() ORDER BY idx", email, email, cartUsualString(cart, "emailLogin_tokenMd5", "")); else sqlSafef(query, sizeof(query), "SELECT * FROM gbMembers WHERE email='%s' ORDER BY idx", email); struct gbMembers *list = gbMembersLoadByQuery(conn, query), *m; hPrintf("<div id=\"chooseAccountBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<h3>Choose an account</h3>"); if (emailMode) hPrintf("<p>The email address <b>%s</b> is associated with more than one %s account. " - "Select the account you would like to sign in to.</p>", email, brwName); + "Select the account you would like to sign in to.</p>", encEmail, brwName); else hPrintf("<p>The email address <b>%s</b> is associated with more than one %s account. " "Select the account you would like to sign in to; your %s login will be linked to it.</p>", - email, brwName, oauthProviderLabel(provider)); + encEmail, brwName, oauthProviderLabel(provider)); hPrintf("<span style='color:red;'>%s</span>", errMsg ? errMsg : ""); hPrintf("<form method=\"post\" action=\"%s\" name=\"chooseAccountForm\">", hgLoginUrl); hPrintf("<div class=\"inputGroup\">"); boolean first = TRUE; for (m = list; m != NULL; m = m->next) { + char *encUserName = htmlEncode(m->userName); hPrintf("<div class=\"acctHelpSection\">" "<input name=\"hgLogin_chosenIdx\" type=\"radio\" value=\"%u\" id=\"acct_%u\"%s>" "<label for=\"acct_%u\" class=\"radioLabel\">%s</label></div>", - m->idx, m->idx, first ? " checked" : "", m->idx, m->userName); + m->idx, m->idx, first ? " checked" : "", m->idx, encUserName); + freeMem(encUserName); first = FALSE; } hPrintf("</div>"); hPrintf("<div class=\"formControls\">" "<input type=\"submit\" name=\"hgLogin.do.chooseAccount\" value=\"Sign In\" class=\"largeButton\">" " <a href=\"%s\" class=\"cancelButton\">Cancel</a>" "</div></form></div><!-- END - chooseAccountBox -->", getReturnToURL()); cartSaveSession(cart); +freeMem(encEmail); gbMembersFreeList(&list); } void chooseAccount(struct sqlConnection *conn) /* Finish the "which account?" chooser: for OAuth, link the pending identity to the chosen * account; for the email link, just sign in. Either way, only accept an account that really * matches the verified email (and, for the email link, still holds the valid token), never an * arbitrary username the client might submit. */ { int chosenIdx = cartUsualInt(cart, "hgLogin_chosenIdx", 0); char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char query[512]; if (isEmpty(provider)) { /* Passwordless email-link mode. */ + if (!emailLinkEnabled()) + { + displayLoginPage(conn); + return; + } char *email = cartUsualString(cart, "emailLogin_email", ""); char *tokenMd5 = cartUsualString(cart, "emailLogin_tokenMd5", ""); if (isEmpty(email) || isEmpty(tokenMd5)) { freez(&errMsg); errMsg = cloneString("Your login link expired. Please request a new one."); displayLoginPage(conn); return; } sqlSafef(query, sizeof(query), "SELECT * FROM gbMembers WHERE idx=%d AND (email='%s' OR recovEmail='%s') " "AND loginToken='%s' AND loginToken<>'' AND loginTokenExpires > NOW()", chosenIdx, email, email, tokenMd5); struct gbMembers *m = gbMembersLoadByQuery(conn, query); if (m == NULL) @@ -1959,37 +2155,41 @@ email, email, tokenMd5); sqlUpdate(conn, query); cartRemove(cart, "emailLogin_email"); cartRemove(cart, "emailLogin_tokenMd5"); cartRemove(cart, "hgLogin_chosenIdx"); loginAndReturn(m->userName, m->idx); gbMembersFree(&m); return; } /* OAuth mode. */ char *subject = cartUsualString(cart, "oauth_pending_subject", ""); char *email = cartUsualString(cart, "oauth_pending_email", ""); if (isEmpty(subject) || isEmpty(email) || !pendingIdentityValid()) { + clearPendingIdentity(); freez(&errMsg); errMsg = cloneString("Your login session expired. Please sign in again."); displayLoginPage(conn); return; } +/* Only an activated account counts: an unactivated row can hold any address someone typed + * without ever proving they own it (see resolveIdentity), so it must not receive a social link. */ sqlSafef(query, sizeof(query), - "SELECT * FROM gbMembers WHERE idx=%d AND email='%s'", chosenIdx, email); + "SELECT * FROM gbMembers WHERE idx=%d AND email='%s' AND accountActivated='Y'", + chosenIdx, email); struct gbMembers *m = gbMembersLoadByQuery(conn, query); if (m == NULL) { freez(&errMsg); errMsg = cloneString("Please choose one of the listed accounts."); chooseAccountPage(conn); return; } struct oauthIdentity pending; ZeroVar(&pending); pending.provider = provider; pending.subject = subject; pending.email = email; linkIdentity(conn, m->idx, &pending); clearPendingIdentity(); @@ -2003,32 +2203,38 @@ * 1. If the provider gave a verified email matching MORE THAN ONE account, always let the * user pick which one -- even if this identity was linked before. Because login cookies * never expire, a user goes through OAuth very rarely, so an occasional pick is cheap * and it lets a person with several same-email accounts choose freely each time. * 2. Else if the (provider,subject) is already linked, log into that account. * 3. Else if the verified email matches exactly one account, auto-link and log in. * 4. Else send the user to the "choose a username" page to finish a new account. * (Providers that don't release an email, e.g. ORCID, never reach step 1 or 3 and rely on * the stored link from step 2.) */ { struct gbMembers *matches = NULL; int n = 0; if (id->emailVerified && isNotEmpty(id->email)) { char query[512]; + /* Match only activated accounts. gbMembers has no unique key on email, and the plain + * signup form will create an unactivated row for any address a person types -- the + * activation mail goes to the address's real owner, who ignores it. Without this filter + * someone could pre-register a victim's address, and the victim's first social login would + * then auto-link to (and sign in as) the attacker's account. */ sqlSafef(query, sizeof(query), - "SELECT * FROM gbMembers WHERE email='%s' ORDER BY idx", id->email); + "SELECT * FROM gbMembers WHERE email='%s' AND accountActivated='Y' ORDER BY idx", + id->email); matches = gbMembersLoadByQuery(conn, query); n = slCount(matches); } if (n > 1) { setPendingIdentity(id); gbMembersFreeList(&matches); chooseAccountPage(conn); return; } struct gbMembers *linked = memberForIdentity(conn, id); if (linked != NULL) { @@ -2129,46 +2335,48 @@ void emailLinkPage(struct sqlConnection *conn) /* Standalone page that asks for an email address and sends a one-time login link. */ { if (!emailLinkEnabled()) { displayLoginPage(conn); return; } hPrintf("<div id=\"emailLinkBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<h3>Email me a sign-in link</h3>"); hPrintf("<p>Enter your email address and we'll send you a link that signs you in without a " "password. This is handy on a computer where you don't have your password saved.</p>"); hPrintf("<span style='color:red;'>%s</span>", errMsg ? errMsg : ""); hPrintf("<form method=\"post\" action=\"%s\" name=\"emailLinkForm\">", hgLoginUrl); +char *encEmail = htmlEncode(cartUsualString(cart, "hgLogin_email", "")); hPrintf("<div class=\"inputGroup\">" "<label for=\"emailLink\">Email address</label>" "<input type=\"text\" name=\"hgLogin_email\" value=\"%s\" size=\"30\" id=\"emailLink\">" - "</div>", cartUsualString(cart, "hgLogin_email", "")); + "</div>", encEmail); +freeMem(encEmail); hPrintf("<div class=\"formControls\">" "<input type=\"submit\" name=\"hgLogin.do.sendEmailLink\" value=\"Send login link\" class=\"largeButton\">" " <a href=\"%s\" class=\"cancelButton\">Cancel</a>" "</div></form></div><!-- END - emailLinkBox -->", getReturnToURL()); cartSaveSession(cart); } void displayLoginLinkSuccess() /* Confirmation shown after a passwordless login link is (possibly) emailed. Phrased so it * does not reveal whether an account exists for the address. */ { -char *email = cartUsualString(cart, "hgLogin_sendMailTo", ""); +char *email = htmlEncode(cartUsualString(cart, "hgLogin_sendMailTo", "")); hPrintf("<div id=\"confirmationBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<p id=\"confirmationMsg\" class=\"confirmationTxt\">If an account exists for " "<B>%s</B>, a login link has been sent to that address.<BR><BR>" "Click the link in that email to sign in — no password needed. " "The link works once and expires in one hour.</p>", email); hPrintf("<p>If you don't see the email, please check your spam folder.</p>"); hPrintf("<p><a href=\"%s?hgLogin.do.displayLoginPage=1\">Return to Login</a></p>\n", hgLoginUrl); cartRemove(cart, "hgLogin_email"); cartRemove(cart, "hgLogin_sendMailTo"); cartRemove(cart, "hgLogin_helpWith"); } void sendLoginLinkMail(char *email, char *token) /* Email a one-time passwordless login link to an address. The link identifies the address, @@ -2323,30 +2531,32 @@ chooseAccount(conn); else if (cartVarExists(cart, "hgLogin.do.emailLinkPage")) emailLinkPage(conn); else if (cartVarExists(cart, "hgLogin.do.sendEmailLink")) sendEmailLink(conn); else if (cartVarExists(cart, "hgLogin.do.emailLogin")) emailLogin(conn); else if (cartVarExists(cart, "hgLogin.do.changePasswordPage")) changePasswordPage(conn); else if (cartVarExists(cart, "hgLogin.do.changePassword")) changePassword(conn); else if (cartVarExists(cart, "hgLogin.do.changeEmailPage")) changeEmailPage(conn); else if (cartVarExists(cart, "hgLogin.do.changeEmail")) changeEmail(conn); +else if (cartVarExists(cart, "hgLogin.do.confirmChangeEmail")) + confirmChangeEmail(conn); else if (cartVarExists(cart, "hgLogin.do.displayAccHelpPage")) displayAccHelpPage(conn); else if (cartVarExists(cart, "hgLogin.do.accountHelp")) accountHelp(conn); else if (cartVarExists(cart, "hgLogin.do.activateAccount")) activateAccount(conn); else if (cartVarExists(cart, "hgLogin.do.displayActMailSuccess")) displayActMailSuccess(); else if (cartVarExists(cart, "hgLogin.do.displayMailSuccess")) displayMailSuccess(); else if (cartVarExists(cart, "hgLogin.do.displayMailSuccessPwd")) displayMailSuccessPwd(); else if (cartVarExists(cart, "hgLogin.do.displayLoginPage")) displayLoginPage(conn); else if (cartVarExists(cart, "hgLogin.do.displayLogin"))