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/lib/https.c src/lib/https.c index ece524484b8..9c8e09e3ccb 100644 --- src/lib/https.c +++ src/lib/https.c @@ -12,30 +12,32 @@ #include <sys/socket.h> #include <unistd.h> #include <pthread.h> #include <signal.h> #include "common.h" #include "internet.h" #include "errAbort.h" #include "hash.h" #include "net.h" char *https_cert_check = "log"; // DEFAULT certificate check is log. char *https_cert_check_depth = "9"; // DEFAULT depth check level is 9. char *https_cert_check_verbose = "off"; // DEFAULT verbose is off. char *https_cert_check_domain_exceptions = ""; // DEFAULT space separated list is empty string. +static boolean https_cert_check_forced = FALSE; // TRUE once httpsSetCertCheck() pins the mode, + // so openSslInit() won't override it from env. char *https_proxy = NULL; char *log_proxy = NULL; char *SCRIPT_NAME = NULL; // For use with callback. Set a variable into the connection itself, // and then use that during the callback. struct myData { char *hostName; }; int myDataIndex = -1; @@ -83,39 +85,53 @@ { fprintf(stderr, "%s\n", msg); fflush(stderr); } void initDomainWhiteListHash(); // forward declaration void myGetenv(char **pMySetting, char *envSetting) /* avoid setenv which causes problems in multi-threaded programs * cloning the env var helps isolate it from other threads activity. */ { char *value = getenv(envSetting); if (value) *pMySetting = cloneString(value); } +void httpsSetCertCheck(char *mode) +/* Pin the TLS certificate-check mode ("abort", "warn", or "log") for every HTTPS connection this + * process makes from here on, overriding hg.conf's httpsCertCheck and the https_cert_check env + * var. Use where a caller must never accept an unverified certificate however the site is + * configured (e.g. hgLogin's OAuth requests, which carry a client secret). Order does not + * matter: openSslInit() will not overwrite a pinned value from the environment, and + * verify_callback reads the live value on each connection, so this takes effect whether it is + * called before or after the first HTTPS connection. */ +{ +https_cert_check = cloneString(mode); +https_cert_check_forced = TRUE; +} + void openSslInit() /* do only once */ { static boolean done = FALSE; static pthread_mutex_t osiMutex = PTHREAD_MUTEX_INITIALIZER; pthread_mutex_lock( &osiMutex ); if (!done) { // setenv avoided since not thread-safe + if (!https_cert_check_forced) // httpsSetCertCheck() wins over the env var myGetenv(&https_cert_check, "https_cert_check"); myGetenv(&https_cert_check_depth, "https_cert_check_depth"); myGetenv(&https_cert_check_verbose, "https_cert_check_verbose"); myGetenv(&https_cert_check_domain_exceptions, "https_cert_check_domain_exceptions"); myGetenv(&https_proxy, "https_proxy"); myGetenv(&log_proxy, "log_proxy"); myGetenv(&SCRIPT_NAME, "SCRIPT_NAME"); SSL_library_init(); ERR_load_crypto_strings(); SSL_load_error_strings(); // ERR_load_SSL_strings(); deprecated. OpenSSL_add_all_algorithms(); openssl_pthread_setup(); myDataIndex = SSL_get_ex_new_index(0, "myDataIndex", NULL, NULL, NULL); initDomainWhiteListHash();