The users_users.valid column stores a copy of the user's password at registration time in plain text. See UsersLib::add_user: * http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/trunk/lib/userslib.php?view=markup#l_2199 * http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/trunk/lib/userslib.php?view=markup#l_2215 and tiki-register.php: * http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/trunk/tiki-register.php?view=markup#l_154 * http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/trunk/tiki-register.php?view=markup#l_158 This issue appears to exist in both SUMO and TikiWiki trunk. Marc, is there anyone you could ask about this column? Is it used anywhere else, or is it safe to set it to NULL (or even drop it) for everyone? I didn't see any initial problems after blanking it out with NULLs. There are other columns named 'valid' (tiki_event_subscriptions) but I didn't find any other references in my search.
This is worth getting in to 188.8.131.52.
Target Milestone: 1.4.1 → 184.108.40.206
I have set the entire column to NULL in my local database and have not yet found any regressions, but my own testing is very limited. Marc, any input from TikiWiki?
Created attachment 403879 [details] [diff] [review] patch, v1 So after evaluating this with Marc and Sylvie from Tiki, it looks like the "valid" column is used for e-mail validation, which is a feature we don't use. It also looks like this issue is only present when e-mail validation is disabled. It looks like there was a flag in the add_user function specifically for this, but that it was not set correctly when validation was off.
Attachment #403879 - Flags: review?(morgamic)
Created attachment 403891 [details] [diff] [review] sql patch users_users.valid is the offending column. I am sure that blanking it out is OK. And yet because it's something like 110,000 rows of data, I remain nervous about it.
Attachment #403891 - Flags: review?(morgamic)
To give the TikiWiki community time to address this and disclose it, we should probably keep this confidential even after it's resolved.
Yes, please keep confidential. We have 2 other related potential vulnerabilities. Security Team has been sent a proposal: *To make one major commit which solves the 3 issues. This commit should include all three fixes and have sql upgrade scripts which encrypt any values that could be lying around. Waiting for feedback from more people in the security team.
Attachment #403891 - Flags: review?(morgamic) → review+
Comment on attachment 403879 [details] [diff] [review] patch, v1 I would vote for fixing this reordering of args BS caused by app preferences, but that might have really not-awesome side effects, so I'm ok with this.
Attachment #403879 - Flags: review?(morgamic) → review+
James, is there a way QA could verify this, or is it pretty much backend/run-SQL-to-verify? Thx.
QA can check registration/login/logout but mostly it's back-end. Any new database errors are suspect. Closing now that bug 520059 ran.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Depends on: 520059
Resolution: --- → FIXED
r52714 < Mobile branch
We can verify this on sumotools with any new registrations after tonight's push.
Whiteboard: tiki_triage → tiki_bug
Verified FIXED on tools. 600 new registrations since the push and no new passwords in the database.
Status: RESOLVED → VERIFIED
Apparently, it was fixed, it now seems to contain a hash.
Whiteboard: tiki_bug → tiki_bug tiki_upstreamed
These bugs are all resolved, so I'm removing the security flag from them.
You need to log in before you can comment on or make changes to this bug.