519559 - Registration stores passwords unencrypted

Status: VERIFIED FIXED

(support.mozilla.org :: Knowledge Base Software, task)

Description

[James Socol [:jsocol, :james]]

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.

Comment 1

[James Socol [:jsocol, :james]]

This is worth getting in to 1.4.0.1.

Comment 2

[James Socol [:jsocol, :james]]

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?

Comment 3

[James Socol [:jsocol, :james]]

Attachment: 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.

Comment 4

[James Socol [:jsocol, :james]]

Attachment: 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.

Comment 5

[James Socol [:jsocol, :james]]

To give the TikiWiki community time to address this and disclose it, we should probably keep this confidential even after it's resolved.

Comment 6

[Marc Laporte]

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.

Comment 7

[Michael Morgan [:morgamic]]

Comment on attachment 403891 [details] [diff] [review]
sql patch

Scary, but I like it.

Comment 8

[Michael Morgan [:morgamic]]

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.

Comment 9

[James Socol [:jsocol, :james]]

r52709 / r52710 /r52711

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

James, is there a way QA could verify this, or is it pretty much backend/run-SQL-to-verify?  Thx.

Comment 11

[James Socol [:jsocol, :james]]

QA can check registration/login/logout but mostly it's back-end. Any new database errors are suspect.

Closing now that bug 520059 ran.

Comment 12

[James Socol [:jsocol, :james]]

r52714 < Mobile branch

Comment 13

[James Socol [:jsocol, :james]]

We can verify this on sumotools with any new registrations after tonight's push.

Comment 14

[James Socol [:jsocol, :james]]

Verified FIXED on tools. 600 new registrations since the push and no new passwords in the database.

Comment 15

[Louis-Philippe Huberdeau]

Apparently, it was fixed, it now seems to contain a hash.

Comment 17

[Will Kahn-Greene [:willkg] ET needinfo? me]

These bugs are all resolved, so I'm removing the security flag from them.