From 45d0f6c6325fb8ab5fdc478a7dc278872defa479 Mon Sep 17 00:00:00 2001 From: Jonathan Prykop Date: Sat, 21 Nov 2015 01:54:21 -0600 Subject: [PATCH] RT#29354: Password Security in Email --- FS/FS/ClientAPI/MyAccount.pm | 6 --- FS/FS/Password_Mixin.pm | 45 +++++++++++++++++++- FS/FS/svc_acct.pm | 25 +++++++++++ debian/control | 3 +- httemplate/edit/svc_acct.cgi | 7 +++- httemplate/elements/change_password.html | 6 +++ httemplate/elements/random_pass.html | 18 ++++++-- httemplate/elements/validate_password.html | 58 ++++++++++++++++++++++++++ httemplate/misc/xmlhttp-validate_password.html | 50 ++++++++++++++++++++++ 9 files changed, 205 insertions(+), 13 deletions(-) create mode 100644 httemplate/elements/validate_password.html create mode 100644 httemplate/misc/xmlhttp-validate_password.html diff --git a/FS/FS/ClientAPI/MyAccount.pm b/FS/FS/ClientAPI/MyAccount.pm index a6bde824a..9847e5f90 100644 --- a/FS/FS/ClientAPI/MyAccount.pm +++ b/FS/FS/ClientAPI/MyAccount.pm @@ -2995,12 +2995,6 @@ 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); - $error ||= $svc_acct->is_password_allowed($p->{'new_password'}) || $svc_acct->set_password($p->{'new_password'}) || $svc_acct->replace(); diff --git a/FS/FS/Password_Mixin.pm b/FS/FS/Password_Mixin.pm index 990f73595..bcad54637 100644 --- a/FS/FS/Password_Mixin.pm +++ b/FS/FS/Password_Mixin.pm @@ -6,8 +6,13 @@ use FS::password_history; use Authen::Passphrase; use Authen::Passphrase::BlowfishCrypt; # https://rt.cpan.org/Ticket/Display.html?id=72743 +use Data::Password qw(:all); our $DEBUG = 0; +our $conf; +FS::UID->install_callback( sub { + $conf = FS::Conf->new; +}); our $me = '[' . __PACKAGE__ . ']'; @@ -38,7 +43,34 @@ sub is_password_allowed { my $self = shift; my $password = shift; - # check length and complexity here + # basic checks using Data::Password; + # options for Data::Password + $DICTIONARY = 4; # minimum length of disallowed words + $MINLEN = $conf->config('passwordmin') || 6; + $MAXLEN = $conf->config('passwordmax') || 8; + $GROUPS = 4; # must have all 4 'character groups': numbers, symbols, uppercase, lowercase + # other options use the defaults listed below: + # $FOLLOWING = 3; # disallows more than 3 chars in a row, by alphabet or keyboard (ie abcd or asdf) + # $SKIPCHAR = undef; # set to true to skip checking for bad characters + # # lists of disallowed words + # @DICTIONARIES = qw( /usr/share/dict/web2 /usr/share/dict/words /usr/share/dict/linux.words ); + + my $error = IsBadPassword($password); + $error = 'must contain at least one each of numbers, symbols, and lowercase and uppercase letters' + if $error eq 'contains less than 4 character groups'; # avoid confusion + $error = 'Invalid password - ' . $error if $error; + return $error if $error; + + #check against known usernames + my @disallowed_names = $self->password_disallowed_names; + foreach my $noname (@disallowed_names) { + if ($password =~ /$noname/i) { + #keeping message ambiguous to avoid leaking personal info + return 'Password contains a disallowed word'; + } + } + + return '' unless $self->get($self->primary_key); # for validating new passwords pre-insert my $no_reuse = 3; # allow override here if we really must @@ -75,6 +107,17 @@ sub is_password_allowed { ''; } +=item password_disallowed_names + +Override to return a list additional words (eg usernames) not +to be used by passwords on this service. + +=cut + +sub password_disallowed_names { + return (); +} + =item password_history_key Returns the name of the field in L that's the foreign diff --git a/FS/FS/svc_acct.pm b/FS/FS/svc_acct.pm index 9323976cb..e7ec4a231 100644 --- a/FS/FS/svc_acct.pm +++ b/FS/FS/svc_acct.pm @@ -2676,6 +2676,31 @@ sub virtual_maildir { $self->domain. '/maildirs/'. $self->username. '/'; } +=item password_disallowed_names + +Override, for L. Not really intended for other use. + +=cut + +sub password_disallowed_names { + my $self = shift; + my $dbh = dbh; + my $results = {}; + foreach my $field ( qw( username finger ) ) { + my $sql = 'SELECT DISTINCT '.$field.' FROM svc_acct'; + my $sth = $dbh->prepare($sql) + or die "Error preparing $sql: ". $dbh->errstr; + $sth->execute() + or die "Error executing $sql: ". $sth->errstr; + foreach my $row (@{$sth->fetchall_arrayref}, $self->get($field)) { + foreach my $word (split(/\s+/,$$row[0])) { + $results->{lc($word)} = 1; + } + } + } + return keys %$results; +} + =back =head1 CLASS METHODS diff --git a/debian/control b/debian/control index 2764f706d..30e9f31dd 100644 --- a/debian/control +++ b/debian/control @@ -27,7 +27,8 @@ Depends: gnupg,ghostscript,gsfonts,gzip,latex-xcolor, libcache-simple-timedexpiry-perl,libchart-perl,libclass-container-perl, libclass-data-inheritable-perl,libclass-returnvalue-perl,libcolor-scheme-perl, libcompress-zlib-perl,libconvert-binhex-perl,libcrypt-passwdmd5-perl, - libcrypt-ssleay-perl,libcss-squish-perl,libdate-manip-perl,libdbd-mysql-perl, + libcrypt-ssleay-perl,libcss-squish-perl, + libdata-password-perl,libdate-manip-perl,libdbd-mysql-perl, libdbd-pg-perl,libdbi-perl,libdbix-dbschema-perl,libdbix-searchbuilder-perl, libdevel-stacktrace-perl,libdevel-symdump-perl,liberror-perl, libexcel-writer-xlsx-perl,libexception-class-perl,libfile-counterfile-perl, diff --git a/httemplate/edit/svc_acct.cgi b/httemplate/edit/svc_acct.cgi index 31678a991..0cf0c20e1 100755 --- a/httemplate/edit/svc_acct.cgi +++ b/httemplate/edit/svc_acct.cgi @@ -50,7 +50,12 @@ 'required' => $part_svc->part_svc_column('_password')->required ) %> MAXLENGTH=<% $pmax %>> - <& /elements/random_pass.html, 'clear_password' &> + <& /elements/random_pass.html, 'clear_password' &>
+
+ <& '/elements/validate_password.html', + 'fieldid' => 'clear_password', + 'svcnum' => $svcnum + &> %}else{ diff --git a/httemplate/elements/change_password.html b/httemplate/elements/change_password.html index 625ba1fb5..7d8daaeaf 100644 --- a/httemplate/elements/change_password.html +++ b/httemplate/elements/change_password.html @@ -16,6 +16,12 @@ <& /elements/random_pass.html, $pre.'password', 'randomize' &> +
+ <& '/elements/validate_password.html', + 'fieldid' => $pre.'password', + 'svcnum' => $svc_acct->svcnum, + + &> % if ( $error ) {
<% $error |h %> % } diff --git a/httemplate/elements/random_pass.html b/httemplate/elements/random_pass.html index b215b77d9..14bbb581d 100644 --- a/httemplate/elements/random_pass.html +++ b/httemplate/elements/random_pass.html @@ -1,13 +1,23 @@ diff --git a/httemplate/elements/validate_password.html b/httemplate/elements/validate_password.html new file mode 100644 index 000000000..fd2cb6ca0 --- /dev/null +++ b/httemplate/elements/validate_password.html @@ -0,0 +1,58 @@ +<%doc> + +To validate passwords via javascript/xmlhttp: + + +
+ <& '/elements/validate_password.html', + fieldid => 'password_field', + svcnum => $svcnum + &> + +The ID of the input field can be anything; the ID of the DIV in which to display results +should be the input id plus '_result'. + + + +<& '/elements/xmlhttp.html', + 'url' => $p.'misc/xmlhttp-validate_password.html', + 'subs' => [ 'validate_password' ], + 'method' => 'POST', # important not to put passwords in url +&> + + +<%init> +my %opt = @_; + + + diff --git a/httemplate/misc/xmlhttp-validate_password.html b/httemplate/misc/xmlhttp-validate_password.html new file mode 100644 index 000000000..28dbf6460 --- /dev/null +++ b/httemplate/misc/xmlhttp-validate_password.html @@ -0,0 +1,50 @@ +<%doc> +Requires cgi params 'password' (plaintext) and 'sub' ('validate_password' is only +acceptable value.) Also accepts 'svcnum' (for svc_acct, will otherwise create an +empty dummy svc_acct) and 'fieldid' (for html post-processing, passed along in +results for convenience.) + +Returns a json-encoded hashref with keys of 'valid' (set to 1 if object is valid), +'error' (error text if password is invalid) or 'syserror' (error text if password +could not be validated.) Only one of these keys will be set. Will also set +'fieldid' if it was passed. + + +<% encode_json($result) %> + +<%init> + +my $validate_password = sub { + my %arg = $cgi->param('arg'); + my %result; + + $result{'fieldid'} = $arg{'fieldid'} + if $arg{'fieldid'} =~ /^\w+$/; + + $result{'syserror'} = 'Request is not POST' unless $cgi->request_method eq 'POST'; + return \%result if $result{'syserror'}; + + my $password = $arg{'password'}; + $result{'syserror'} = 'Invoked without password' unless $password; + return \%result if $result{'syserror'}; + + my $svcnum = $arg{'svcnum'}; + $result{'syserror'} = 'Invalid svcnum' unless $svcnum =~ /^\d*$/; + return \%result if $result{'syserror'}; + + my $svc_acct = $svcnum + ? qsearchs('svc_acct',{'svcnum' => $svcnum}) + : (new FS::svc_acct {}); + $result{'syserror'} = 'Could not find service' unless $svc_acct; + return \%result if $result{'syserror'}; + + $result{'error'} = $svc_acct->is_password_allowed($password); + $result{'valid'} = 1 unless $result{'error'}; + return \%result; +}; + +my $result = ($cgi->param('sub') eq 'validate_password') + ? &$validate_password() + : { 'syserror' => 'Invalid sub' }; + + -- 2.11.0