From b5a9068479a38c2b901b1954a57c51f43e84be2d Mon Sep 17 00:00:00 2001 From: Mark Wells Date: Thu, 12 Nov 2015 16:49:39 -0800 Subject: [PATCH] limit password reuse, core and svc_acct, #29354 --- FS/FS/ClientAPI/MyAccount.pm | 11 +- FS/FS/ClientAPI/Signup.pm | 3 + FS/FS/Conf.pm | 7 ++ FS/FS/Mason.pm | 1 + FS/FS/Password_Mixin.pm | 165 +++++++++++++++++++++++++ FS/FS/Schema.pm | 46 +++++++ FS/FS/password_history.pm | 174 +++++++++++++++++++++++++++ FS/FS/svc_acct.pm | 15 ++- FS/MANIFEST | 2 + FS/t/password_history.t | 5 + httemplate/edit/process/svc_acct.cgi | 7 +- httemplate/misc/process/change-password.html | 4 +- 12 files changed, 433 insertions(+), 7 deletions(-) create mode 100644 FS/FS/Password_Mixin.pm create mode 100644 FS/FS/password_history.pm create mode 100644 FS/t/password_history.t diff --git a/FS/FS/ClientAPI/MyAccount.pm b/FS/FS/ClientAPI/MyAccount.pm index 10b26529f..9bbde882b 100644 --- a/FS/FS/ClientAPI/MyAccount.pm +++ b/FS/FS/ClientAPI/MyAccount.pm @@ -2947,13 +2947,15 @@ sub myaccount_passwd { ) && ! $svc_acct->check_password($p->{'old_password'}); + # should move password length checks into is_password_allowed $error = 'Password too short.' if length($p->{'new_password'}) < ($conf->config('passwordmin') || 6); $error = 'Password too long.' if length($p->{'new_password'}) > ($conf->config('passwordmax') || 8); - $svc_acct->set_password($p->{'new_password'}); - $error ||= $svc_acct->replace(); + $error ||= $svc_acct->is_password_allowed($p->{'new_password'}) + || $svc_acct->set_password($p->{'new_password'}) + || $svc_acct->replace(); #regular pw change in self-service should change contact pw too, otherwise its #way too confusing. hell its confusing they're separate at all, but alas. @@ -3217,8 +3219,9 @@ sub process_reset_passwd { if ( $svc_acct ) { - $svc_acct->set_password($p->{'new_password'}); - my $error = $svc_acct->replace(); + my $error ||= $svc_acct->is_password_allowed($p->{'new_password'}) + || $svc_acct->set_password($p->{'new_password'}) + || $svc_acct->replace(); return { %$info, 'error' => $error } if $error; diff --git a/FS/FS/ClientAPI/Signup.pm b/FS/FS/ClientAPI/Signup.pm index 3208396b9..a9678b051 100644 --- a/FS/FS/ClientAPI/Signup.pm +++ b/FS/FS/ClientAPI/Signup.pm @@ -698,6 +698,9 @@ sub new_customer { map { $_ => $packet->{$_} } qw( username _password sec_phrase popnum domsvc ), }; + + my $error = $svc->is_password_allowed($packet->{_password}); + return { error => $error } if $error; my @acct_snarf; my $snarfnum = 1; diff --git a/FS/FS/Conf.pm b/FS/FS/Conf.pm index fad786ca3..c259b7a62 100644 --- a/FS/FS/Conf.pm +++ b/FS/FS/Conf.pm @@ -4252,6 +4252,13 @@ and customer address. Include units.', }, { + 'key' => 'password-no_reuse', + 'section' => 'password', + 'description' => 'Minimum number of password changes before a password can be reused. By default, passwords can be reused without restriction.', + 'type' => 'text', + }, + + { 'key' => 'datavolume-forcemegabytes', 'section' => 'UI', 'description' => 'All data volumes are expressed in megabytes', diff --git a/FS/FS/Mason.pm b/FS/FS/Mason.pm index 9a4a89205..ba56cff38 100644 --- a/FS/FS/Mason.pm +++ b/FS/FS/Mason.pm @@ -384,6 +384,7 @@ if ( -e $addl_handler_use_file ) { use FS::report_batch; use FS::report_batch; use FS::report_batch; + use FS::password_history; # Sammath Naur if ( $FS::Mason::addl_handler_use ) { diff --git a/FS/FS/Password_Mixin.pm b/FS/FS/Password_Mixin.pm new file mode 100644 index 000000000..af4c5e2b7 --- /dev/null +++ b/FS/FS/Password_Mixin.pm @@ -0,0 +1,165 @@ +package FS::Password_Mixin; + +use FS::Record qw(qsearch); +use FS::Conf; +use FS::password_history; +use Authen::Passphrase; +# use Authen::Passphrase::BlowfishCrypt; # ha ha, no. +# https://rt.cpan.org/Ticket/Display.html?id=72743 + +our $DEBUG = 1; +our $conf; +FS::UID->install_callback( sub { + $conf = FS::Conf->new; + # this is safe + eval "use Authen::Passphrase::BlowfishCrypt;"; +}); + +our $me = '[' . __PACKAGE__ . ']'; + +our $BLOWFISH_COST = 10; + +=head1 NAME + +FS::Password_Mixin - Object methods for accounts that have passwords governed +by the password policy. + +=head1 METHODS + +=over 4 + +=item is_password_allowed PASSWORD + +Checks the password against the system password policy. Returns an error +message on failure, an empty string on success. + +This MUST NOT be called from check(). It should be called by the office UI, +self-service ClientAPI, or other I code that processes a +password change, and only if the user has taken some action with the intent +of changing the password. + +=cut + +sub is_password_allowed { + my $self = shift; + my $password = shift; + + # check length and complexity here + + if ( $conf->config('password-no_reuse') =~ /^(\d+)$/ ) { + + my $no_reuse = $1; + + # "the last N" passwords includes the current password and the N-1 + # passwords before that. + warn "$me checking password reuse limit of $no_reuse\n" if $DEBUG; + my @latest = qsearch({ + 'table' => 'password_history', + 'hashref' => { $self->password_history_key => $self->get($self->primary_key) }, + 'order_by' => " ORDER BY created DESC LIMIT $no_reuse", + }); + + # don't check the first one; reusing the current password is allowed. + shift @latest; + + foreach my $history (@latest) { + warn "$me previous password created ".$history->created."\n" if $DEBUG; + if ( $history->password_equals($password) ) { + my $message; + if ( $no_reuse == 1 ) { + $message = "This password is the same as your previous password."; + } else { + $message = "This password was one of the last $no_reuse passwords on this account."; + } + return $message; + } + } #foreach $history + + } # end of no_reuse checking + + ''; +} + +=item password_history_key + +Returns the name of the field in L that's the foreign +key to this table. + +=cut + +sub password_history_key { + my $self = shift; + $self->table . '__' . $self->primary_key; +} + +=item insert_password_history + +Creates a L record linked to this object, with its +current password. + +=cut + +sub insert_password_history { + my $self = shift; + my $encoding = $self->_password_encoding; + my $password = $self->_password; + my $auth; + + if ( $encoding eq 'bcrypt' or $encoding eq 'crypt' ) { + + # it's smart enough to figure this out + $auth = Authen::Passphrase->from_crypt($password); + + } elsif ( $encoding eq 'ldap' ) { + + $password =~ s/^{PLAIN}/{CLEARTEXT}/i; # normalize + $auth = Authen::Passphrase->from_rfc2307($password); + if ( $auth->isa('Authen::Passphrase::Clear') ) { + # then we've been given the password in cleartext + $auth = $self->_blowfishcrypt( $auth->passphrase ); + } + + } elsif ( $encoding eq 'plain' ) { + + $auth = $self->_blowfishcrypt( $password ); + + } + + my $password_history = FS::password_history->new({ + _password => $auth->as_rfc2307, + created => time, + $self->password_history_key => $self->get($self->primary_key), + }); + + my $error = $password_history->insert; + return "recording password history: $error" if $error; + ''; + +} + +=item _blowfishcrypt PASSWORD + +For internal use: takes PASSWORD and returns a new +L object representing it. + +=cut + +sub _blowfishcrypt { + my $class = shift; + my $passphrase = shift; + return Authen::Passphrase::BlowfishCrypt->new( + cost => $BLOWFISH_COST, + salt_random => 1, + passphrase => $passphrase, + ); +} + +=back + +=head1 SEE ALSO + +L + +=cut + +1; diff --git a/FS/FS/Schema.pm b/FS/FS/Schema.pm index d0b69ca1a..e55a8f1b7 100644 --- a/FS/FS/Schema.pm +++ b/FS/FS/Schema.pm @@ -4904,6 +4904,52 @@ sub tables_hashref { ], }, + 'password_history' => { + 'columns' => [ + 'passwordnum', 'serial', '', '', '', '', + '_password', 'varchar', 'NULL', $char_d, '', '', + 'encryption_method', 'varchar', 'NULL', $char_d, '', '', + 'created', @date_type, '', '', + # each table that needs password history gets a column here, and + # an entry in foreign_keys. + 'svc_acct__svcnum', 'int', 'NULL', '', '', '', + 'svc_dsl__svcnum', 'int', 'NULL', '', '', '', + 'svc_alarm__svcnum', 'int', 'NULL', '', '', '', + 'agent__agentnum', 'int', 'NULL', '', '', '', + 'contact__contactnum', 'int', 'NULL', '', '', '', + 'access_user__usernum', 'int', 'NULL', '', '', '', + ], + 'primary_key' => 'passwordnum', + 'unique' => [], + 'index' => [], + 'foreign_keys' => [ + { columns => [ 'svc_acct__svcnum' ], + table => 'svc_acct', + references => [ 'svcnum' ], + }, + { columns => [ 'svc_dsl__svcnum' ], + table => 'svc_dsl', + references => [ 'svcnum' ], + }, + { columns => [ 'svc_alarm__svcnum' ], + table => 'svc_alarm', + references => [ 'svcnum' ], + }, + { columns => [ 'agent__agentnum' ], + table => 'agent', + references => [ 'agentnum' ], + }, + { columns => [ 'contact__contactnum' ], + table => 'contact', + references => [ 'contactnum' ], + }, + { columns => [ 'access_user__usernum' ], + table => 'access_user', + references => [ 'usernum' ], + }, + ], + }, + # name type nullability length default local #'new_table' => { diff --git a/FS/FS/password_history.pm b/FS/FS/password_history.pm new file mode 100644 index 000000000..dd527b980 --- /dev/null +++ b/FS/FS/password_history.pm @@ -0,0 +1,174 @@ +package FS::password_history; +use base qw( FS::Record ); + +use strict; +use FS::Record qw( qsearch qsearchs ); +use Authen::Passphrase; + +# the only bit of autogenerated magic in here +our @foreign_keys; +FS::UID->install_callback(sub { + @foreign_keys = grep /__/, __PACKAGE__->dbdef_table->columns; +}); + +=head1 NAME + +FS::password_history - Object methods for password_history records + +=head1 SYNOPSIS + + use FS::password_history; + + $record = new FS::password_history \%hash; + $record = new FS::password_history { 'column' => 'value' }; + + $error = $record->insert; + + $error = $new_record->replace($old_record); + + $error = $record->delete; + + $error = $record->check; + +=head1 DESCRIPTION + +An FS::password_history object represents a current or past password used +by a login account, employee, or other account managed within Freeside. +FS::password_history inherits from FS::Record. The following fields are +currently supported: + +=over 4 + +=item passwordnum - primary key + +=item _password - the encrypted password, as an RFC2307-style string +("{CRYPT}$2a$08$..." or "{MD5}1ab201f..." or similar). This is a serialized +L object. + +=item created - the date the password was set to this value. The record with +the most recent created time is the current password. + +=back + +Plus one of the following foreign keys: + +=over 4 + +=item svc_acct__svcnum + +=item svc_dsl__svcnum + +=item svc_alarm__svcnum + +=item agent__agentnum + +=item contact__contactnum + +=item access_user__usernum + +=back + +=head1 METHODS + +=over 4 + +=item new HASHREF + +Creates a new password history record. To add the record to the database, +see L<"insert">. + +=cut + +sub table { 'password_history'; } + +=item insert + +=item delete + +=item replace OLD_RECORD + +=item check + +Checks all fields to make sure this is a valid password history record. If +there is an error, returns the error, otherwise returns false. Called by the +insert and replace methods. + +=cut + +sub check { + my $self = shift; + + my $error = + $self->ut_numbern('passwordnum') + || $self->ut_anything('_password') + || $self->ut_numbern('create') + || $self->ut_numbern('create') + ; + return $error if $error; + + # FKs are mutually exclusive + my $fk_in_use; + foreach my $fk ( @foreign_keys ) { + if ( $self->get($fk) ) { + $self->ut_numbern($fk); + return "multiple records linked to this password_history" if $fk_in_use; + $fk_in_use = $fk; + } + } + + $self->SUPER::check; +} + +=item linked_acct + +Returns the object that's using this password. + +=cut + +sub linked_acct { + my $self = shift; + + foreach my $fk ( @foreign_keys ) { + if ( my $val = $self->get($fk) ) { + my ($table, $key) = split(/__/, $fk); + return qsearchs($table, { $key => $val }); + } + } +} + +=item password_equals PASSWORD + +Returns true if PASSWORD (plaintext) is the same as the one stored in the +history record, false if not. + +=cut + +sub password_equals { + + my ($self, $check_password) = @_; + + # _password here is always LDAP-style. + try { + my $auth = Authen::Passphrase->from_rfc2307($self->_password); + return $auth->match($check_password); + } catch { + # if there's somehow bad data in the _password field, then it doesn't + # match anything. much better than having it match _everything_. + warn "password_history #" . $self->passwordnum . ": $_"; + return ''; + } + +} + +=back + +=head1 BUGS + +=head1 SEE ALSO + +L + +=cut + +1; + diff --git a/FS/FS/svc_acct.pm b/FS/FS/svc_acct.pm index 9636b3ed1..a8cb7ba5b 100644 --- a/FS/FS/svc_acct.pm +++ b/FS/FS/svc_acct.pm @@ -7,7 +7,11 @@ use base qw( FS::svc_Domain_Mixin FS::svc_Radius_Mixin FS::svc_Tower_Mixin FS::svc_IP_Mixin - FS::svc_Common ); + FS::Password_Mixin + FS::svc_Common + ); + +use strict; use vars qw( $DEBUG $me $conf $skip_fuzzyfiles $dir_prefix @shells $usernamemin $usernamemax $passwordmin $passwordmax @@ -701,6 +705,9 @@ sub insert { 'child_objects' => $self->child_objects, %options, ); + + $error ||= $self->insert_password_history; + if ( $error ) { $dbh->rollback if $oldAutoCommit; return $error; @@ -985,6 +992,12 @@ sub replace { my $dbh = dbh; $error = $new->SUPER::replace($old, @_); # usergroup here + + # don't need to record this unless the password was changed + if ( $old->_password ne $new->_password ) { + $error ||= $new->insert_password_history; + } + if ( $error ) { $dbh->rollback if $oldAutoCommit; return $error if $error; diff --git a/FS/MANIFEST b/FS/MANIFEST index 4f3b6aa0d..b3679b5f0 100644 --- a/FS/MANIFEST +++ b/FS/MANIFEST @@ -798,3 +798,5 @@ FS/cust_pkg_reason_fee.pm t/cust_pkg_reason_fee.t FS/report_batch.pm t/report_batch.t +FS/password_history.pm +t/password_history.t diff --git a/FS/t/password_history.t b/FS/t/password_history.t new file mode 100644 index 000000000..b7a05fd9f --- /dev/null +++ b/FS/t/password_history.t @@ -0,0 +1,5 @@ +BEGIN { $| = 1; print "1..1\n" } +END {print "not ok 1\n" unless $loaded;} +use FS::password_history; +$loaded=1; +print "ok 1\n"; diff --git a/httemplate/edit/process/svc_acct.cgi b/httemplate/edit/process/svc_acct.cgi index 9cac2c568..d75ff92c1 100755 --- a/httemplate/edit/process/svc_acct.cgi +++ b/httemplate/edit/process/svc_acct.cgi @@ -81,7 +81,12 @@ if ( $cgi->param('clear_password') eq '*HIDDEN*' || $cgi->param('clear_password') =~ /^\(.* encrypted\)$/ ) { die "fatal: no previous account to recall hidden password from!" unless $old; } else { - $error ||= $new->set_password($cgi->param('clear_password')); + my $newpass = $cgi->param('clear_password'); + if ( ! $old->check_password($newpass) ) { + # then the password is being changed + $error ||= $new->is_password_allowed($newpass) + || $new->set_password($newpass); + } } if ( ! $error ) { diff --git a/httemplate/misc/process/change-password.html b/httemplate/misc/process/change-password.html index 7cab9c4e3..d58ce544d 100644 --- a/httemplate/misc/process/change-password.html +++ b/httemplate/misc/process/change-password.html @@ -11,7 +11,9 @@ die "access denied" unless ( ( $curuser->access_right('Edit password') and ! $part_svc->restrict_edit_password ) ); -my $error = $svc_acct->set_password($cgi->param('password')) +my $newpass = $cgi->param('password'); +my $error = $svc_acct->is_password_allowed($newpass) + || $svc_acct->set_password($newpass) || $svc_acct->replace; # annoyingly specific to view/svc_acct.cgi, for now... -- 2.11.0