Bug: Password is mangled after user registration (DEVELOP)
From
InvalidActionInRegister:
From the registration page, a different password is stored than the one entered. When .htpasswd is edited manually and the newly inserted encrypted code is replaced with a different code from another installation, registration works as expected.
Could it be that the password sent from the registration form is a different password that I entered? That somehow the password is mangled before it is stored in .htpasswd? (see also
Support.ApachePasswordsUnix2)
Test case
Create a fresh install from the current DEVELOP (
SVN 3857). Add a new user. Try to edit a page and log in.
Environment
--
ArthurClemens - 23 Mar 2005
Impact and Available Solutions
Follow up
In
ApachePasswordsUnix2,
AntonAylward says:
I've been doing a lot of testing of Dakar/DEVELOP I-A-A and often work from resetting the password from the command line.
I'm getting to wonder if the code doesn't produce the same results as the Apache utility, since I can
always log in after a command line reset but can
never log in after I've requested a password reset as an anonymous user (
ResetPassword) and when I try resetting my password after logging in successfully from the papche POV (
ChangePassword) the "Old Password" is not accepted.
I'd [...] suggest the use of the
CPAN module rather than the current code.
--
ArthurClemens - 23 Mar 2005
I'll take a look tomorrow morning. M.
--
MartinCleaver - 24 Mar 2005
I've done a quick instrumentation of the password system by inserting a proxy object in front of it.
- $this->{users} = new TWiki::Users( $this, 'HtPasswdUser' );
+ $this->{users} = new TWiki::Users( $this, 'ProxyPasswdUser' );
All argument calls being logged in the debug log.
Although I haven't solved the problem I think I've made it much easier to do so.
Committed revision 3869.
--
MartinCleaver - 24 Mar 2005
The debug log currently shows all calls down to the password system. Here's a new registration of account
WikiName NewAccount
LoginName newaccount. It looks ok to me.
| 26 Mar 2005 - 00:23 | TWiki::UI::Register::_addUserToPasswordSystem:655 => TWiki::User::passwordExists:951
| 26 Mar 2005 - 00:23 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:23 | => Calling TWiki::Users::HtPasswdUser:UserPasswordExists (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:23 | UserPasswordExists returned:
$VAR1 = 0;
| 26 Mar 2005 - 00:23 | TWiki::UI::Register::_addUserToPasswordSystem:655 => TWiki::User::addPassword:966
| 26 Mar 2005 - 00:23 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:23 | => Calling TWiki::Users::HtPasswdUser:AddUserPassword (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
$VAR3 = 'password';
)
| 26 Mar 2005 - 00:23 | AddUserPassword returned:
$VAR1 = '1';
And now a reset:
| 26 Mar 2005 - 00:27 | TWiki::UI::Register::_resetUsersPassword:444 => TWiki::User::passwordExists:481
| 26 Mar 2005 - 00:27 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:27 | => Calling TWiki::Users::HtPasswdUser:UserPasswordExists (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:27 | UserPasswordExists returned:
$VAR1 = 1;
| 26 Mar 2005 - 00:27 | TWiki::User::resetPassword:490 => TWiki::User::passwordExists:285
| 26 Mar 2005 - 00:27 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:27 | => Calling TWiki::Users::HtPasswdUser:UserPasswordExists (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:27 | UserPasswordExists returned:
$VAR1 = 1;
| 26 Mar 2005 - 00:27 | TWiki::User::resetPassword:490 => TWiki::User::removePassword:286
| 26 Mar 2005 - 00:27 | [ hasargs = 1, wantarray = , evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:27 | => Calling TWiki::Users::HtPasswdUser:RemoveUser (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:27 | TWiki::Users::ProxyPasswdUser::AUTOLOAD:230 => TWiki::Users::HtPasswdUser::RemoveUser:35
| 26 Mar 2005 - 00:27 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:27 | => Calling TWiki::Users::HtPasswdUser:htpasswdReadPasswd (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:27 | htpasswdReadPasswd returned:
$VAR1 = 'X/GWqnmr/8ueo';
| 26 Mar 2005 - 00:27 | TWiki::Users::ProxyPasswdUser::AUTOLOAD:230 => TWiki::Users::HtPasswdUser::RemoveUser:35
| 26 Mar 2005 - 00:27 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:27 | => Calling TWiki::Users::HtPasswdUser:htpasswdUpdateUser (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount:X/GWqnmr/8ueo';
$VAR3 = '';
)
| 26 Mar 2005 - 00:27 | htpasswdUpdateUser returned:
$VAR1 = '1';
| 26 Mar 2005 - 00:27 | RemoveUser returned:
$VAR1 = '1';
| 26 Mar 2005 - 00:27 | TWiki::User::resetPassword:490 => TWiki::User::addPassword:289
| 26 Mar 2005 - 00:27 | [ hasargs = 1, wantarray = , evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:27 | => Calling TWiki::Users::HtPasswdUser:AddUserPassword (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
$VAR3 = 1672;
)
| 26 Mar 2005 - 00:27 | AddUserPassword returned:
$VAR1 = '1';
And finally an attempted change, which fails because the password '1672' is deemed to be incorrect because the last two crypted strings don't match:
| 26 Mar 2005 - 00:28 | TWiki::UI::Register::changePassword:131 => TWiki::User::checkPassword:567
| 26 Mar 2005 - 00:28 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:28 | => Calling TWiki::Users::HtPasswdUser:CheckUserPasswd (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
$VAR3 = '1672';
)
| 26 Mar 2005 - 00:28 | TWiki::Users::ProxyPasswdUser::AUTOLOAD:211 => TWiki::Users::HtPasswdUser::CheckUserPasswd:35
| 26 Mar 2005 - 00:28 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:28 | => Calling TWiki::Users::HtPasswdUser:htpasswdReadPasswd (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:28 | htpasswdReadPasswd returned:
$VAR1 = 'w7ADTSVhNDxz2';
| 26 Mar 2005 - 00:28 | TWiki::Users::ProxyPasswdUser::AUTOLOAD:211 => TWiki::Users::HtPasswdUser::CheckUserPasswd:35
| 26 Mar 2005 - 00:28 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:28 | => Calling TWiki::Users::HtPasswdUser:htpasswdGeneratePasswd (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
$VAR3 = '1672';
$VAR4 = 1;
)
| 26 Mar 2005 - 00:28 | TWiki::Users::ProxyPasswdUser::AUTOLOAD:247 => TWiki::Users::HtPasswdUser::htpasswdGeneratePasswd:35
| 26 Mar 2005 - 00:28 | [ hasargs = 1, wantarray = 0, evaltext = , is_require = , hints = 0, bitmask = UUUUUUUUUUUU ]
| 26 Mar 2005 - 00:28 | => Calling TWiki::Users::HtPasswdUser:htpasswdReadPasswd (
$VAR1 = bless( {
'proxy' => bless( {
'session' => 'TWiki=HASH(0x80f58a8)'
}, 'TWiki::Users::HtPasswdUser' ),
'session' => $VAR1->{'proxy'}{'session'}
}, 'TWiki::Users::ProxyPasswdUser' );
$VAR2 = 'newaccount';
)
| 26 Mar 2005 - 00:28 | htpasswdReadPasswd returned:
$VAR1 = 'w7ADTSVhNDxz2';
| 26 Mar 2005 - 00:28 | htpasswdGeneratePasswd returned:
$VAR1 = 'w7m5c1u6ldE8g';
| 26 Mar 2005 - 00:28 | CheckUserPasswd returned:
$VAR1 = '';
We could debug this but...
Crawford Currie wrote in an email:
>
It really should be using CPAN:Htpasswd
AntonAylward wrote in response:
>
Let me put on my professional hat for a moment ...
>
From a security POV ... security isn't just about "keeping hackers out".
>
Correct and reliable function ("integrity' and 'availability' of
>
service) are predicates.
And I wrote:
>
The CPAN version is undoubtedly better
>
tested; it could even be better code. One might argue that the
>
security of this module is important so should use better code.
>
Another might argue that changing it is fine in principle but they are
>
not going to volunteer to change it.
So, three questions:
- Do we think our passwd system is robust enough?
- Given that PasswordsAreMangled, does anyone care to rewrite it? Even if they did, would a home grown version be of any value to us vis-a-vis a standard one?
- Do we have consensus to wire TWiki::Users::HtPasswdUser to use CPAN:Apache::Htpasswd for implementation?
Passwords are very likely to remain managled until we get consensus on this.
--
MartinCleaver - 26 Mar 2005
--
MartinCleaver - 26 Mar 2005
N.B. I read the trace above to mean that its
HtPasswdUserDotPm, rather than the code changed by the
RegisterCgiScriptRewrite is misbehaving. If someone validates my finding, suggests a reason or disputes my claim I'll use more time to look at this today. Much appreciated, Thanks.
I tried to get onto
TWikiIRC to ask about this but it I can't connect
--
MartinCleaver - 27 Mar 2005
THis is an important function. It is one of the key components to a professionally functioning system and is a road hurdle to the adoption of the Dakar release.
I agree with
Martin about the evidence of the trace. In addition, that
HtPasswdDotPm banks out the current password then adds a new one at the end leads to a sloppy looking file. Whatever else, things that aesthetically displeasing probably have other flaws, I've learnt.
In short we have three ways we can go:
- Rewrite HttasswdUserDotPm
It would helpt if we knew what was wrong with it!
- In HtPasswdUserDotPm there is a comment that the external program 'htpasswd' cannot be used because it won't accept the new password as STDIN. While true, this is only marginally relevant. The 'htpasswd' program can accept the new password on the command line.
This is a security risk, and the documentation on 'htpasswd' mentions this. It is a risk becuase someone could run 'ps' at just the right time and see what the password is. This is a small exposure and many may consider it acceptable.
- In terms of risk, the weakness of the algorithm used by the basic authentication and the fact that the clear-text password is being transmitted in clear over the network and that password resets are being transmitted in clear-text by e-mail over the network to store and forward respositories of questiobnale security may be more significant.
- If the main threat comes from another user on the host machine, then the protection of the password file is also an issue.
- In the light of the above I consider the use of 'htpassword' in 'batch mode' (i.e. password on the command line) to be a minor risk.
- Use CPAN:Apache::Htpaaswd
My opinion is that the second alternative - use the command line 'htpasswd' is the route we should take first. My reasoning for this is as follows.
- It is a minimal code change
- It uses code that is completely compatible - by definition
- It very quickly and easily proves or disproves the correctness of the existing code in HtPasswdDotPm
- It elimiates the blank line problem
We will, of course, have to make sure that certain charecters don't appear in passwords
--
AntonAylward - 27 Mar 2005
I acknowledge that this is still a problem in
DevelopBranchVersion. (I was a bit surprised as I know Crawford checked in a big change that I'd read as fixing this problem and addressing Anton's point).
My password still fails to log me in and hitting escape at the authentication prompt gives me the following error:
Can't call method "param" on an undefined value at /home/.balder/mrjc/testwiki.mrjc.com/twiki/lib/TWiki/UI/Oops.pm line 56.
TWiki::UI::__ANON__('Can\'t call method "param" on an undefined value at /home/.balde...') called at /home/.balder/mrjc/testwiki.mrjc.com/twiki/lib/TWiki/UI/Oops.pm line 56
TWiki::UI::Oops::oops_cgi(undef) called at /home/.balder/mrjc/testwiki.mrjc.com/twiki/lib/TWiki/UI.pm line 133
TWiki::UI::run('CODE(0x85aa0a0)') called
Error::subs::run_clauses('HASH(0x85bd7fc)', 'Can\'t call method "param" on an undefined value at /home/.balde...', undef, 'ARRAY(0x815f8d8)') called at /home/.balder/mrjc/testwiki.mrjc.com/twiki/lib/Error.pm line 394
Error::subs::try('CODE(0x80f58a8)', 'HASH(0x85bd7fc)') called at /home/.balder/mrjc/testwiki.mrjc.com/twiki/lib/TWiki/UI.pm line 190
TWiki::UI::run('CODE(0x85aa0a0)') called at oops line 29
--
MartinCleaver - 19 Apr 2005
Can someone comment whether this issue still exists? Thx.
--
MartinCleaver - 16 May 2005
In my view this bug does still exist. Test case: Reset a password, this will send you a new temporary one. Then try to change the password - TWiki tells you that the old (temporary) password is wrong. However, you can log in with that temporary password.
--
MartinCleaver - 28 Jun 2005
Fix record
I suspect this was cleaned up by Crawford's cleanup of this code. I have therefore closed this bug report.
--
MartinCleaver - 30 May 2005
A scan of a bug -
http://develop.twiki.org/~develop/cgi-bin/view/Bugs/Item29 got me digging. It seesm that reset works but that change does not: I've made the failure explicit by dieing as soon as possible: changePassword is complaining that the password assigned by reset is invalid but I disagree as you can actually log in with that password.
HtPasswdUser is so messy; if I could get paid to spend the time I'd ditch it;
ApacheHtpasswdUser was an attempt to call the
CPAN modules in its place. At least that way we don't spend effort on where well tested functionlity exists elsewhere.
Revision 4430.
--
MartinCleaver - 25 Jun 2005
FYI I ran tests on the apache module versus the TWiki one; they behaved identically, so I never bothered with apache (which requires a
CPAN module) i.e. if it ain't broke, why fix it?
--
CrawfordCurrie - 28 Jun 2005
I was working on DB authentication method for Users.pm and I beleive I found the bug at lib/TWiki/UI/Register.pm , line 615:
if ($user->changePassword($passwordB)) {
as you can see, it passes just the new password as an argument. The correct should be:
if ($user->changePassword($oldpassword ,$passwordB)) {
that should fix the problem.
--
GabrielAraujo - 29 Jun 2005
I've added that to the DEVELOP
svn
. Revision 4465
sigh Why can't perl do checking ....
--
AntonAylward - 29 Jun 2005
Nice one Gabriel! Well spotted! Anton, thanks for putting it into the code; did you add a unit test so we don't risk having the same problem repeated again in the future?
--
CrawfordCurrie - 30 Jun 2005
Discussion