RT#29354: Password Security in Email

9 files changed, 205 insertions, 13 deletions

diff --git a/FS/FS/ClientAPI/MyAccount.pm b/FS/FS/ClientAPI/MyAccount.pm
index a6bde82..9847e5f 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 990f735..bcad546 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<FS::password_history> that's the foreign
diff --git a/FS/FS/svc_acct.pm b/FS/FS/svc_acct.pm
index 9323976..e7ec4a2 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<FS::Password_Mixin>.  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 6534e2f..5408f4c 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 31678a9..0cf0c20 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 ) %>
   <TD>
     <INPUT TYPE="text" ID="clear_password" NAME="clear_password" VALUE="<% $password %>" SIZE=<% $pmax2 %> MAXLENGTH=<% $pmax %>>
-    <& /elements/random_pass.html, 'clear_password' &>
+    <& /elements/random_pass.html, 'clear_password' &><BR>
+    <DIV ID="clear_password_result" STYLE="font-size: smaller"></DIV>
+    <& '/elements/validate_password.html', 
+         'fieldid' => 'clear_password',
+         'svcnum' => $svcnum 
+    &>
   </TD>
 </TR>
 %}else{
diff --git a/httemplate/elements/change_password.html b/httemplate/elements/change_password.html
index 625ba1f..7d8daae 100644
--- a/httemplate/elements/change_password.html
+++ b/httemplate/elements/change_password.html
@@ -16,6 +16,12 @@
     <& /elements/random_pass.html, $pre.'password', 'randomize' &>
     <INPUT TYPE="submit" VALUE="change">
     <INPUT TYPE="button" VALUE="cancel" onclick="<%$pre%>toggle(false)">
+    <DIV ID="<%$pre%>password_result" STYLE="font-size: smaller"></DIV>
+    <& '/elements/validate_password.html', 
+         'fieldid' => $pre.'password',
+         'svcnum'  => $svc_acct->svcnum,
+
+    &>
 % if ( $error ) {
     <BR><SPAN STYLE="color: #ff0000"><% $error |h %></SPAN>
 % }
diff --git a/httemplate/elements/random_pass.html b/httemplate/elements/random_pass.html
index b215b77..14bbb58 100644
--- a/httemplate/elements/random_pass.html
+++ b/httemplate/elements/random_pass.html
@@ -1,13 +1,23 @@
 <INPUT TYPE="button" VALUE="<% emt($label) %>" onclick="randomPass()">
 <SCRIPT TYPE="text/javascript">
 function randomPass() {
+  var lower='<% join('', 'a'..'z') %>';
+  var upper='<% join('', 'A'..'Z') %>';
+  var number='<% join('', '0'..'9') %>';
+  var symbol='`~!@#$%^&*-_=+:;<>,.?';
+  var pw_set=lower+upper+number+symbol;
+  var pass=[];
+  pass.push(lower.charAt(Math.floor(Math.random() * lower.length)));
+  pass.push(upper.charAt(Math.floor(Math.random() * lower.length)));
+  pass.push(number.charAt(Math.floor(Math.random() * number.length)));
+  pass.push(symbol.charAt(Math.floor(Math.random() * symbol.length)));
   var i=0;
-  var pw_set='<% join('', 'a'..'z', 'A'..'Z', '0'..'9' ) %>';
-  var pass='';
-  while(i < 8) {
+  while(i < 4) {
     i++;
-    pass += pw_set.charAt(Math.floor(Math.random() * pw_set.length));
+    pass.push(pw_set.charAt(Math.floor(Math.random() * pw_set.length)));
   }
+  for(var j, x, i = pass.length; i; j = Math.floor(Math.random() * i), x = pass[--i], pass[i] = pass[j], pass[j] = x);
+  pass = pass.join('');
   document.getElementById('<% $id %>').value = pass;
 }
 </SCRIPT>
diff --git a/httemplate/elements/validate_password.html b/httemplate/elements/validate_password.html
new file mode 100644
index 0000000..fd2cb6c
--- /dev/null
+++ b/httemplate/elements/validate_password.html
@@ -0,0 +1,58 @@
+<%doc>
+
+To validate passwords via javascript/xmlhttp:
+
+  <INPUT ID="password_field" TYPE="text">
+  <DIV ID="password_field_result">
+  <& '/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'.
+
+</%doc>
+
+<& '/elements/xmlhttp.html',
+    'url'  => $p.'misc/xmlhttp-validate_password.html',
+    'subs' => [ 'validate_password' ],
+    'method' => 'POST', # important not to put passwords in url
+&>
+<SCRIPT>
+function add_password_validation (fieldid) {
+  var inputfield = document.getElementById(fieldid);
+  inputfield.onchange = function () {
+    var fieldid = this.id+'_result';
+    var resultfield = document.getElementById(fieldid);
+    if (this.value) {
+      resultfield.innerHTML = '<SPAN STYLE="color: blue;">Validating password...</SPAN>';
+      validate_password('fieldid',fieldid,'svcnum','<% $opt{'svcnum'} %>','password',this.value,
+        function (result) {
+          result = JSON.parse(result);
+          var resultfield = document.getElementById(result.fieldid);
+          if (resultfield) {
+            if (result.valid) {
+              resultfield.innerHTML = '<SPAN STYLE="color: green;">Password valid!</SPAN>';
+            } else if (result.error) {
+              resultfield.innerHTML = '<SPAN STYLE="color: red;">'+result.error+'</SPAN>';
+            } else {
+              result.syserror = result.syserror || 'Server error';
+              resultfield.innerHTML = '<SPAN STYLE="color: red;">'+result.syserror+'</SPAN>';
+            }
+          }
+        }
+      );
+    } else {
+      resultfield.innerHTML = '';
+    }
+  };
+}
+add_password_validation('<% $opt{'fieldid'} %>');
+</SCRIPT>
+
+<%init>
+my %opt = @_;
+</%init>
+
+
diff --git a/httemplate/misc/xmlhttp-validate_password.html b/httemplate/misc/xmlhttp-validate_password.html
new file mode 100644
index 0000000..28dbf64
--- /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.
+</%doc>
+
+<% 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' };
+
+</%init>