Project

General

Profile

Bug #5693

Hide/Mask stored passwords when presenting GUI forms

Added by Jim Pingle over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Web Interface
Target version:
Start date:
12/24/2015
Due date:
% Done:

50%

Estimated time:
Affected Version:
2.3
Affected Architecture:

Description

Currently there are a number of places in the GUI for the user to enter passwords used by additional or external systems (Proxies, Notifications, auth servers, HA Sync, etc) and when a password is stored, it is presented back to the user in the form. Ideally this should not be the case. Even though the passwords are stored plain in config.xml if a user has limited access they couldn't see the raw config, but they could potentially get a stored password from the GUI.

A couple ideas (not necessarily the best way, suggestions only):
  • Come up with a "standard" password entry method that has both a password and confirm password box
  • Don't present the real password to the user, but some fake/pre-determined value that the code will recognize and ignore so it doesn't clobber the real password when saving, it would only save the entered value if both the password and confirm password match.
  • On pages where the password is optional, retain the ability to leave the password blank/have no password

It would be extra nice if we could find some way to prevent browsers from auto-fill spamming everything with the GUI password if the user has it stored as well, which is another common source of misery in this area.

Mostly complete list of affected pages/fields:
  • system_hasync.php: HA sync password
  • services_captiveportal_vouchers.php: Voucher sync settings
  • system_advanced_notifications.php: Growl password, SMTP password
  • services_captiveportal.php: RADIUS shared secrets, RADIUS MAC auth password
  • system_authservers.php: RADIUS shared secret, LDAP bind user credentials
  • wizards/setup_wizard.xml: PPPoE/L2TP/PPTP WAN passwords
  • services_pppoe_edit.php: PPPoE user password, RADIUS shared secrets
  • interfaces_ppps_edit.php: PPPoE/L2TP/PPTP WAN passwords
  • interfaces.php: PPPoE/L2TP/PPTP WAN passwords, 802.1x RADIUS shared secrets, DHCPv6 advanced keyinfo statement secret (maybe?)
  • services_dyndns_edit.php: DynDNS service account password
  • system_advanced_misc.php: Proxy password
  • vpn_l2tp_users_edit.php: L2TP user password
  • firewall_virtual_ip_edit.php: CARP VIP password
  • vpn_openvpn_client.php: Proxy password, connection user password
  • wizards/openvpn_wizard.xml: Wizard stores RADIUS/LDAP password/secret, might be good to just blank that when finishing the wizard
  • services_dhcpv6.php: DNS domain key secret (maybe?)
  • vpn_ipsec_keys.php: User PSK/Passwords (might be overkill to hide those)

Associated revisions

Revision 5ea90990 (diff)
Added by Steve Beaver over 3 years ago

Experimental: Fixed #5693
Added new functionality to PHP classes, and used it ONLY in system_advanced_notifications.php -> smtp password

History

#1 Updated by Jim Thompson over 3 years ago

  • Assignee set to Jim Pingle

#2 Updated by Jim Pingle over 3 years ago

  • Assignee changed from Jim Pingle to Steve Beaver

Nudging this over toward Steve Beaver, he told me last week he had an idea to propose for a fix.

#3 Updated by Steve Beaver over 3 years ago

If we take a simple example like system_advanced_notifications.php, one solution that would seem to fill the bill is this:

On displaying the password control, we always display some fixed text, such as:

define(DMYPWD, "********");

$section->addInput(new Form_Input(
    'smtppassword',
    'Email password',
    'password',
    DMYPWD,
    ['placeholder' => 'Redacted']
))->setHelp('Enter your password here');

Then in the $_POST handler, we just need:
if ($_POST[smtppassword'] != DMYPWD) {
    $config['notifications']['smtp']['password'] = $_POST['smtppassword'];
}

We never display the password. If the user edits it, it is updated. If not it remains unchanged.
Not every page is this simple, and there are places we would need to retrieve the existing password from the config file so we can write it back, but it should be fairly easy to generalize.

#4 Updated by Steve Beaver over 3 years ago

  • Status changed from New to Feedback
  • % Done changed from 0 to 100

#5 Updated by Steve Beaver over 3 years ago

  • % Done changed from 100 to 50

#6 Updated by Steve Beaver over 3 years ago

  • Assignee changed from Steve Beaver to Jim Pingle

New addPassword() method and password validation added where appropriate.

I have not addressed the wizard XML pages.

#7 Updated by Chris Buechler over 3 years ago

on services_dyndns_edit.php at least, and probably others, you can no longer edit entries without entering the password again. Otherwise fails with "The field Password is required." Which ideally is a check we want if it's a new entry, but not upon edit of existing.

#8 Updated by Steve Beaver over 3 years ago

services_dyndns_edit.php corrected.

If $_POSTed password == DMYPWD, the old password must be retrieved and re-saved.

This may (or may not) be the case with certain other pages. Checking now.

#9 Updated by Steve Beaver over 3 years ago

  • Status changed from Feedback to Resolved

Also available in: Atom PDF