Registration stores passwords unencrypted

VERIFIED FIXED in 1.4.0.1

Status

support.mozilla.org
Knowledge Base Software
--
critical
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: jsocol, Assigned: jsocol)

Tracking

unspecified
1.4.0.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tiki_bug tiki_upstreamed)

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
This is worth getting in to 1.4.0.1.
Target Milestone: 1.4.1 → 1.4.0.1
(Assignee)

Comment 2

9 years ago
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?

Updated

9 years ago
Whiteboard: tiki_triage
(Assignee)

Comment 3

9 years ago
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)
(Assignee)

Comment 4

9 years ago
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)
(Assignee)

Comment 5

9 years ago
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

9 years ago
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 403891 [details] [diff] [review]
sql patch

Scary, but I like it.
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+
(Assignee)

Comment 9

9 years ago
r52709 / r52710 /r52711
James, is there a way QA could verify this, or is it pretty much backend/run-SQL-to-verify?  Thx.
(Assignee)

Comment 11

9 years ago
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
(Assignee)

Comment 12

9 years ago
r52714 < Mobile branch
(Assignee)

Comment 13

9 years ago
We can verify this on sumotools with any new registrations after tonight's push.
Whiteboard: tiki_triage → tiki_bug
(Assignee)

Comment 14

9 years ago
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
(Assignee)

Updated

9 years ago
Duplicate of this bug: 484974
These bugs are all resolved, so I'm removing the security flag from them.
Group: websites-security
You need to log in before you can comment on or make changes to this bug.