From 2da78b515ad199ede438b0569810511958855535 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Sat, 2 Nov 2013 20:52:18 +0000 Subject: [PATCH 01/10] Use bcrypt for login form. Add tests to check that salts and hashes are random, and check handling of invalid passwords or salts. --- modules/installed/lib/auth.py | 5 ++- tests/auth_test.py | 70 +++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 tests/auth_test.py diff --git a/modules/installed/lib/auth.py b/modules/installed/lib/auth.py index 7e6384d98..3dfd995c1 100644 --- a/modules/installed/lib/auth.py +++ b/modules/installed/lib/auth.py @@ -7,7 +7,8 @@ # on 1 February 2011. import cherrypy -import urllib, hashlib +import urllib +from passlib.hash import bcrypt import cfg import random import time @@ -31,7 +32,7 @@ def check_credentials(username, passphrase): u = None # hash the password whether the user exists, to foil timing # side-channel attacks - pass_hash = hashlib.md5(passphrase).hexdigest() + pass_hash = bcrypt.encrypt(passphrase, salt=u['salt']) if u is None or u['passphrase'] != pass_hash: error = "Bad user-name or password." diff --git a/tests/auth_test.py b/tests/auth_test.py new file mode 100644 index 000000000..861a51d59 --- /dev/null +++ b/tests/auth_test.py @@ -0,0 +1,70 @@ +#! /usr/bin/env python +# -*- mode: python; mode: auto-fill; fill-column: 80 -*- + +import user_store, auth +from passlib.hash import bcrypt +from logger import Logger +import cfg +import unittest +import cherrypy +import plugin_mount +import os +from model import User +cfg.log = Logger() + +cherrypy.log.access_file = None + +class Auth(unittest.TestCase): + """Test check_credentials function of auth to confirm it works as expected""" + + def setUp(self): + cfg.user_db = os.path.join(cfg.file_root, "tests/testdata/users"); + cfg.users = plugin_mount.UserStoreModule.get_plugins()[0] + + def tearDown(self): + for user in cfg.users.get_all(): + cfg.users.remove(user[0]) + cfg.users.close() + + def test_password_check(self): + self.add_user("test_user", "password") + + # check_credentials returns None if there is no error, or returns error string + self.assertIsNone(auth.check_credentials("test_user", "password")) + self.assertIsNotNone(auth.check_credentials("test_user", "wrong")) + + def test_salt_check(self): + test_user = self.add_user("test_user", "password", "abcdefghijklmnopqrstuv") + self.assertIsNotNone(auth.check_credentials("test_user", "password")) + + def test_salt_is_random(self): + test_user1 = self.add_user("test_user1", "password") + test_user2 = self.add_user("test_user2", "password") + self.assertNotEqual(test_user1["salt"], test_user2["salt"]) + + def test_hash_is_random(self): + test_user1 = self.add_user("test_user1", "password") + test_user2 = self.add_user("test_user2", "password") + self.assertNotEqual(test_user1["passphrase"], test_user2["passphrase"]) + + # set fake_salt if you want to store an invalid salt + def add_user(self, test_username, passphrase='', fake_salt=None): + test_user = self.create_user(test_username, passphrase, fake_salt) + cfg.users.set(test_username,test_user) + return test_user + + def create_user(self, username, passphrase='', fake_salt=None): + test_user = User() + test_user["username"] = username + pass_hash = bcrypt.encrypt(passphrase) + test_user["passphrase"] = pass_hash + if fake_salt: + test_user["salt"] = fake_salt + else: + # for bcrypt, the salt is output as part of the hash + test_user["salt"] = pass_hash[7:29] + + return test_user + +if __name__ == "__main__": + unittest.main() From 3425d265c32d33c189710bcffd1d0df62ce27b3a Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Sat, 2 Nov 2013 21:03:29 +0000 Subject: [PATCH 02/10] update model --- model.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/model.py b/model.py index e0b3cd20d..6ad424e23 100644 --- a/model.py +++ b/model.py @@ -1,9 +1,9 @@ class User(dict): """ Every user must have keys for a username, name, passphrase (this - is a md5 hash of the password), groups, and an email address. They can be - blank or None, but the keys must exist. """ + is a bcrypt hash of the password), salt, groups, and an email address. + They can be blank or None, but the keys must exist. """ def __init__(self, dict=None): - for key in ['username', 'name', 'passphrase', 'email']: + for key in ['username', 'name', 'passphrase', 'salt', 'email']: self[key] = '' for key in ['groups']: self[key] = [] From 198cea5b587b6ed843c3cb09ec847bb26dfd2d16 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Sun, 3 Nov 2013 21:55:06 +0000 Subject: [PATCH 03/10] Use bcrypt to hash passwords for new users in firstboot and user_add forms. Removed references to md5 hashing which was already non-functional. --- modules/installed/first_boot.py | 10 ++++++---- modules/installed/system/users.py | 16 +++++++++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/modules/installed/first_boot.py b/modules/installed/first_boot.py index 6104d1a94..6f7d9bcd7 100644 --- a/modules/installed/first_boot.py +++ b/modules/installed/first_boot.py @@ -9,6 +9,7 @@ from withsqlite.withsqlite import sqlite_db import cfg import config from model import User +from passlib.hash import bcrypt class FirstBoot(PagePlugin): def __init__(self, *args, **kwargs): @@ -26,7 +27,7 @@ class FirstBoot(PagePlugin): return "fake key" @cherrypy.expose - def state0(self, message="", hostname="", box_key="", submitted=False, username="", md5_password="", **kwargs): + def state0(self, message="", hostname="", box_key="", submitted=False, username="", password="", **kwargs): """ In this state, we do time config over HTTP, name the box and server key selection. @@ -63,13 +64,15 @@ class FirstBoot(PagePlugin): elif submitted and not box_key: box_key = self.generate_box_key() db['box_key'] = box_key - if username and md5_password: + if username and password: + pass_hash = bcrypt.encrypt(password) di = { 'username':username, 'name':'First user - please change', 'expert':'on', "groups": ["expert"], - 'passphrase':md5_password, + 'passphrase':pass_hash, + 'salt':pass_hash[7:29], # for bcrypt } new_user = User(di) cfg.users.set(username,new_user) @@ -93,7 +96,6 @@ class FirstBoot(PagePlugin): form.html("

Initial user and password. Access to this web interface is protected by knowing a username and password. Provide one here to register the initial privileged user. The password can be changed and other users added later.

") form.text_input('Username:', id="username", value=username) form.text_input('Password:', id="password", type='password') - form.text_input(name="md5_password", type="hidden") form.html("

%(box_name)s uses cryptographic keys so it can prove its identity when talking to you. %(box_name)s can make a key for itself, but if one already exists (from a prior FreedomBox, for example), you can paste it below. This key should not be the same as your key because you are not your FreedomBox!

" % {'box_name':cfg.box_name}) form.text_box("If you want, paste your box's key here.", id="box_key", value=box_key) form.hidden(name="submitted", value="True") diff --git a/modules/installed/system/users.py b/modules/installed/system/users.py index 1d358e4fa..129f34cf9 100644 --- a/modules/installed/system/users.py +++ b/modules/installed/system/users.py @@ -6,6 +6,7 @@ import cfg from forms import Form from util import * from model import User +from passlib.hash import bcrypt class users(PagePlugin): order = 20 # order of running init in PagePlugins @@ -34,28 +35,33 @@ class add(FormPlugin, PagePlugin): def main(self, username='', name='', email='', message=None, *args, **kwargs): form = Form(title="Add User", action="/sys/users/add/index", - onsubmit="return md5ify('add_user_form', 'password')", name="add_user_form", message=message) form.text_input(_("Username"), name="username", value=username) form.text_input(_("Full name"), name="name", value=name) form.text_input(_("Email"), name="email", value=email) form.text_input(_("Password"), name="password", type="password") - form.text_input(name="md5_password", type="hidden") form.submit(label=_("Create User"), name="create") return form.render() - def process_form(self, username=None, name=None, email=None, md5_password=None, **kwargs): + def process_form(self, username=None, name=None, email=None, password=None, **kwargs): msg = Message() if not username: msg.add = _("Must specify a username!") - if not md5_password: msg.add = _("Must specify a password!") + if not password: msg.add = _("Must specify a password!") if username in cfg.users.get_all(): msg.add = _("User already exists!") else: try: - di = {'username':username, 'name':name, 'email':email, 'passphrase':md5_password} + pass_hash = bcrypt.encrypt(password) + di = { + 'username':username, + 'name':name, + 'email':email, + 'passphrase':pass_hash, + 'salt': pass_hash[7:29], # for bcrypt + } new_user = User(di) cfg.users.set(username,new_user) except: From dbeb31dfa1a5b2f2cff11692d4ee355548b43b0d Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Sun, 3 Nov 2013 23:39:16 +0000 Subject: [PATCH 04/10] Add add_user function to auth module. --- modules/installed/lib/auth.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/modules/installed/lib/auth.py b/modules/installed/lib/auth.py index 3dfd995c1..0c62a7822 100644 --- a/modules/installed/lib/auth.py +++ b/modules/installed/lib/auth.py @@ -15,6 +15,35 @@ import time cfg.session_key = '_cp_username' +def add_user(username, passphrase, name='', email='', expert=False): + error = None + if not username: error = "Must specify a username!" + if not password: error = "Must specify a password!" + + if error is None: + # hash the password whether the user exists, to foil timing + # side-channel attacks + pass_hash = bcrypt.encrypt(password) + + if username in cfg.users.get_all(): + error = "User already exists!" + else: + di = { + 'username':username, + 'name':name, + 'email':email, + 'expert':'on' if expert else 'off', + 'groups':['expert'] if expert else [], + 'passphrase':pass_hash, + 'salt':pass_hash[7:29], # for bcrypt + } + new_user = User(di) + cfg.users.set(username,newuser) + + if error: + cfg.log(error) + return error + def check_credentials(username, passphrase): """Verifies credentials for username and passphrase. Returns None on success or a string describing the error on failure""" From 01ac7e164ee268c33180c659b22d89ee847ff009 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Mon, 4 Nov 2013 00:30:21 +0000 Subject: [PATCH 05/10] Update tests for auth module, and fix some bugs discovered in auth module. --- modules/installed/lib/auth.py | 30 +++++++++++++----- tests/auth_test.py | 58 +++++++++++++++++------------------ 2 files changed, 50 insertions(+), 38 deletions(-) diff --git a/modules/installed/lib/auth.py b/modules/installed/lib/auth.py index 0c62a7822..441ee11b8 100644 --- a/modules/installed/lib/auth.py +++ b/modules/installed/lib/auth.py @@ -9,22 +9,30 @@ import cherrypy import urllib from passlib.hash import bcrypt +from passlib.exc import PasswordSizeError import cfg import random import time +from model import User cfg.session_key = '_cp_username' def add_user(username, passphrase, name='', email='', expert=False): + """Add a new user with specified username and passphrase. + """ error = None if not username: error = "Must specify a username!" - if not password: error = "Must specify a password!" + if not passphrase: error = "Must specify a passphrase!" if error is None: # hash the password whether the user exists, to foil timing # side-channel attacks - pass_hash = bcrypt.encrypt(password) + try: + pass_hash = bcrypt.encrypt(passphrase) + except PasswordSizeError: + error = "Password is too long." + if error is None: if username in cfg.users.get_all(): error = "User already exists!" else: @@ -38,7 +46,7 @@ def add_user(username, passphrase, name='', email='', expert=False): 'salt':pass_hash[7:29], # for bcrypt } new_user = User(di) - cfg.users.set(username,newuser) + cfg.users.set(username,new_user) if error: cfg.log(error) @@ -55,13 +63,19 @@ def check_credentials(username, passphrase): cfg.log(error) return error - if username in cfg.users: - u = cfg.users[username] - else: - u = None # hash the password whether the user exists, to foil timing # side-channel attacks - pass_hash = bcrypt.encrypt(passphrase, salt=u['salt']) + try: + if username in cfg.users: + u = cfg.users[username] + pass_hash = bcrypt.encrypt(passphrase, salt=u['salt']) + else: + u = None + pass_hash = bcrypt.encrypt(passphrase) + except PasswordSizeError: + error = "Password is too long." + cfg.log(error) + return error if u is None or u['passphrase'] != pass_hash: error = "Bad user-name or password." diff --git a/tests/auth_test.py b/tests/auth_test.py index 861a51d59..589025987 100644 --- a/tests/auth_test.py +++ b/tests/auth_test.py @@ -26,45 +26,43 @@ class Auth(unittest.TestCase): cfg.users.remove(user[0]) cfg.users.close() - def test_password_check(self): - self.add_user("test_user", "password") + def test_add_user(self): + self.assertIsNone(auth.add_user("test_user", "password")) + self.assertIsNotNone(auth.add_user(None, "password")) + self.assertIsNotNone(auth.add_user("test_user", None)) + self.assertIsNotNone(auth.add_user("test_user", "password")) - # check_credentials returns None if there is no error, or returns error string + def test_password_check(self): + auth.add_user("test_user", "password") + + # check_credentials returns None if there is no error, + # or returns error string self.assertIsNone(auth.check_credentials("test_user", "password")) self.assertIsNotNone(auth.check_credentials("test_user", "wrong")) - def test_salt_check(self): - test_user = self.add_user("test_user", "password", "abcdefghijklmnopqrstuv") + def test_nonexistent_user(self): self.assertIsNotNone(auth.check_credentials("test_user", "password")) + def test_password_too_long(self): + password = "x" * 4097 + self.assertIsNotNone(auth.add_user("test_user", password)) + self.assertIsNotNone(auth.check_credentials("test_user", password)) + def test_salt_is_random(self): - test_user1 = self.add_user("test_user1", "password") - test_user2 = self.add_user("test_user2", "password") - self.assertNotEqual(test_user1["salt"], test_user2["salt"]) + auth.add_user("test_user1", "password") + auth.add_user("test_user2", "password") + self.assertNotEqual( + cfg.users["test_user1"]["salt"], + cfg.users["test_user2"]["salt"] + ) def test_hash_is_random(self): - test_user1 = self.add_user("test_user1", "password") - test_user2 = self.add_user("test_user2", "password") - self.assertNotEqual(test_user1["passphrase"], test_user2["passphrase"]) - - # set fake_salt if you want to store an invalid salt - def add_user(self, test_username, passphrase='', fake_salt=None): - test_user = self.create_user(test_username, passphrase, fake_salt) - cfg.users.set(test_username,test_user) - return test_user - - def create_user(self, username, passphrase='', fake_salt=None): - test_user = User() - test_user["username"] = username - pass_hash = bcrypt.encrypt(passphrase) - test_user["passphrase"] = pass_hash - if fake_salt: - test_user["salt"] = fake_salt - else: - # for bcrypt, the salt is output as part of the hash - test_user["salt"] = pass_hash[7:29] - - return test_user + auth.add_user("test_user1", "password") + auth.add_user("test_user2", "password") + self.assertNotEqual( + cfg.users["test_user1"]["passphrase"], + cfg.users["test_user2"]["passphrase"] + ) if __name__ == "__main__": unittest.main() From 9238aea8fec41eafbb88a8e6c8123b485fe5f7aa Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Mon, 4 Nov 2013 01:14:17 +0000 Subject: [PATCH 06/10] Fix check for already existing username in add_user. Add documentation of process for storing and validating hashed passwords. --- doc/Makefile | 2 +- doc/security.mdwn | 30 ++++++++++++++++++++++++++++++ modules/installed/lib/auth.py | 2 +- 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 doc/security.mdwn diff --git a/doc/Makefile b/doc/Makefile index 6b37461f3..adaf5e2f1 100644 --- a/doc/Makefile +++ b/doc/Makefile @@ -6,7 +6,7 @@ PDFLATEX=pdflatex # List text files in the order in which you want them to appear in the # complete manual: -SOURCES=README.mdwn INSTALL.mdwn themes.mdwn hacking.mdwn TODO.mdwn modules.mdwn scripts.mdwn faq.mdwn COPYING.mdwn colophon.mdwn +SOURCES=README.mdwn INSTALL.mdwn themes.mdwn hacking.mdwn TODO.mdwn modules.mdwn scripts.mdwn security.mdwn faq.mdwn COPYING.mdwn colophon.mdwn OTHER= TODO_SOURCES=$(patsubst TODO.mdwn,,$(SOURCES)) MAN_SOURCES=$(patsubst COPYING.mdwn,copyright_notice00,$(SOURCES)) diff --git a/doc/security.mdwn b/doc/security.mdwn new file mode 100644 index 000000000..febf609b0 --- /dev/null +++ b/doc/security.mdwn @@ -0,0 +1,30 @@ +# Security + +## Password Storage + +Here is an overview of how user passwords are currently being stored in Plinth. + +### Storing a password (add_user function in auth module): + +1. We check if the username or password is empty. If so, return an error message. + +2. Use bcrypt (from passlib) to encrypt the password and generate a random salt. This step is performed regardless of whether the user already exists. + +3. If the password length is over 4096, bcrypt raises an exception. We catch this exception and return an error message. + +4. Check if the username exists in user store. If so, return an error message. + +5. If no error has occurred so far, create the new user. The username, hashed password, and salt are stored in the user store databaes. The salt is a substring of the hash output by bcrypt. + +### Checking password at login (check_credentials function in auth module): + +1. We check if the username or password is empty. If so, return an error message. + +2. Use bcrypt to encrypt the supplied password. This step is performed regardless of whether the user already exists. If the user exists, use the salt value stored for that user in the database. Otherwise, don't specify a salt (bcrypt will generate a random one). + +3. If the password length is over 4096, bcrypt raises an exception. We catch this exception and return an error message. + +4. Check if the user doesn't exist, or if the hashed password doesn't match the stored hash. Return an error message "Bad user-name or password" if either of these conditions are true. + +5. If no error has occurred so far, return None to indicate that the supplied credentials are valid. + diff --git a/modules/installed/lib/auth.py b/modules/installed/lib/auth.py index 441ee11b8..fec48b80a 100644 --- a/modules/installed/lib/auth.py +++ b/modules/installed/lib/auth.py @@ -33,7 +33,7 @@ def add_user(username, passphrase, name='', email='', expert=False): error = "Password is too long." if error is None: - if username in cfg.users.get_all(): + if username in map(lambda x: x[0], cfg.users.get_all()): error = "User already exists!" else: di = { From dccd1deae13d1838ec3a76c2644e2649f3d54656 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Mon, 4 Nov 2013 02:00:40 +0000 Subject: [PATCH 07/10] Modify firstboot and user_add forms to use add_user function. --- modules/installed/first_boot.py | 21 +++++++-------------- modules/installed/system/users.py | 27 +++++---------------------- tests/auth_test.py | 1 - 3 files changed, 12 insertions(+), 37 deletions(-) diff --git a/modules/installed/first_boot.py b/modules/installed/first_boot.py index 6f7d9bcd7..8bb8e4d68 100644 --- a/modules/installed/first_boot.py +++ b/modules/installed/first_boot.py @@ -2,14 +2,13 @@ from urlparse import urlparse import os, cherrypy, re from gettext import gettext as _ from plugin_mount import PagePlugin, PluginMount, FormPlugin -from modules.auth import require +from modules.auth import require, add_user from forms import Form import util as u from withsqlite.withsqlite import sqlite_db import cfg import config from model import User -from passlib.hash import bcrypt class FirstBoot(PagePlugin): def __init__(self, *args, **kwargs): @@ -65,18 +64,12 @@ class FirstBoot(PagePlugin): box_key = self.generate_box_key() db['box_key'] = box_key if username and password: - pass_hash = bcrypt.encrypt(password) - di = { - 'username':username, - 'name':'First user - please change', - 'expert':'on', - "groups": ["expert"], - 'passphrase':pass_hash, - 'salt':pass_hash[7:29], # for bcrypt - } - new_user = User(di) - cfg.users.set(username,new_user) - validuser = True + error = add_user(username, password, 'First user - please change', '', True) + if error: + message += _("User account creation failed: %s") % error + validuser = False + else: + validuser = True else: validuser = False diff --git a/modules/installed/system/users.py b/modules/installed/system/users.py index 129f34cf9..c4bb79495 100644 --- a/modules/installed/system/users.py +++ b/modules/installed/system/users.py @@ -1,12 +1,11 @@ import os, cherrypy from gettext import gettext as _ -from auth import require +from auth import require, add_user from plugin_mount import PagePlugin, FormPlugin import cfg from forms import Form from util import * from model import User -from passlib.hash import bcrypt class users(PagePlugin): order = 20 # order of running init in PagePlugins @@ -47,28 +46,12 @@ class add(FormPlugin, PagePlugin): def process_form(self, username=None, name=None, email=None, password=None, **kwargs): msg = Message() - if not username: msg.add = _("Must specify a username!") - if not password: msg.add = _("Must specify a password!") - - if username in cfg.users.get_all(): - msg.add = _("User already exists!") + error = add_user(username, password, name, email, False) + if error: + msg.text = error else: - try: - pass_hash = bcrypt.encrypt(password) - di = { - 'username':username, - 'name':name, - 'email':email, - 'passphrase':pass_hash, - 'salt': pass_hash[7:29], # for bcrypt - } - new_user = User(di) - cfg.users.set(username,new_user) - except: - msg.add = _("Error storing user!") - - if not msg: msg.add = _("%s saved." % username) + cfg.log(msg.text) main = self.main(username, name, email, msg=msg.text) return self.fill_template(title="Manage Users and Groups", main=main, sidebar_left=self.sidebar_left, sidebar_right=self.sidebar_right) diff --git a/tests/auth_test.py b/tests/auth_test.py index 589025987..6ea5d0252 100644 --- a/tests/auth_test.py +++ b/tests/auth_test.py @@ -2,7 +2,6 @@ # -*- mode: python; mode: auto-fill; fill-column: 80 -*- import user_store, auth -from passlib.hash import bcrypt from logger import Logger import cfg import unittest From f55ba4155132437ed82595a0b77919b8bbe286d0 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Mon, 4 Nov 2013 02:13:27 +0000 Subject: [PATCH 08/10] Use POST instead of GET for forms. It seems like it's working now. --- modules/installed/lib/forms.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/modules/installed/lib/forms.py b/modules/installed/lib/forms.py index 827e11048..4980efd37 100644 --- a/modules/installed/lib/forms.py +++ b/modules/installed/lib/forms.py @@ -19,14 +19,7 @@ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more details. """ class Form(): - def __init__(self, action=None, cls='form', title=None, onsubmit=None, name=None, message='', method="get"): - """Note that there appears to be a bug in cherrypy whereby - forms submitted via post don't have their fields included in - kwargs for the default index method. So we use get by - default, though it's not as neat. - - TODO: file bug on this w/ CherryPy project - """ + def __init__(self, action=None, cls='form', title=None, onsubmit=None, name=None, message='', method="post"): action = self.get_form_attrib_text('action', action) onsubmit = self.get_form_attrib_text('onsubmit', onsubmit) From 978db68137bf664914a13e4fad0b152cb2f3b76f Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Mon, 4 Nov 2013 02:23:40 +0000 Subject: [PATCH 09/10] Remove time.clock line in auth module. It wasn't being used. --- modules/installed/lib/auth.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/modules/installed/lib/auth.py b/modules/installed/lib/auth.py index fec48b80a..4ceff878d 100644 --- a/modules/installed/lib/auth.py +++ b/modules/installed/lib/auth.py @@ -12,7 +12,6 @@ from passlib.hash import bcrypt from passlib.exc import PasswordSizeError import cfg import random -import time from model import User cfg.session_key = '_cp_username' @@ -56,8 +55,6 @@ def check_credentials(username, passphrase): """Verifies credentials for username and passphrase. Returns None on success or a string describing the error on failure""" - start = time.clock() - if not username or not passphrase: error = "No username or password." cfg.log(error) From 91cdbc4ef830c12dfbf8ed3dbad941109ee028b1 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Mon, 4 Nov 2013 02:37:12 +0000 Subject: [PATCH 10/10] No need to have avoid timing side-channel attack in user_add. We're just going to tell you if the user already exists anyway. --- doc/security.mdwn | 2 +- modules/installed/lib/auth.py | 36 +++++++++++++++++------------------ 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/doc/security.mdwn b/doc/security.mdwn index febf609b0..286f65934 100644 --- a/doc/security.mdwn +++ b/doc/security.mdwn @@ -8,7 +8,7 @@ Here is an overview of how user passwords are currently being stored in Plinth. 1. We check if the username or password is empty. If so, return an error message. -2. Use bcrypt (from passlib) to encrypt the password and generate a random salt. This step is performed regardless of whether the user already exists. +2. Use bcrypt (from passlib) to encrypt the password and generate a random salt. 3. If the password length is over 4096, bcrypt raises an exception. We catch this exception and return an error message. diff --git a/modules/installed/lib/auth.py b/modules/installed/lib/auth.py index 4ceff878d..aae470444 100644 --- a/modules/installed/lib/auth.py +++ b/modules/installed/lib/auth.py @@ -23,29 +23,27 @@ def add_user(username, passphrase, name='', email='', expert=False): if not username: error = "Must specify a username!" if not passphrase: error = "Must specify a passphrase!" - if error is None: - # hash the password whether the user exists, to foil timing - # side-channel attacks - try: - pass_hash = bcrypt.encrypt(passphrase) - except PasswordSizeError: - error = "Password is too long." - if error is None: if username in map(lambda x: x[0], cfg.users.get_all()): error = "User already exists!" else: - di = { - 'username':username, - 'name':name, - 'email':email, - 'expert':'on' if expert else 'off', - 'groups':['expert'] if expert else [], - 'passphrase':pass_hash, - 'salt':pass_hash[7:29], # for bcrypt - } - new_user = User(di) - cfg.users.set(username,new_user) + try: + pass_hash = bcrypt.encrypt(passphrase) + except PasswordSizeError: + error = "Password is too long." + + if error is None: + di = { + 'username':username, + 'name':name, + 'email':email, + 'expert':'on' if expert else 'off', + 'groups':['expert'] if expert else [], + 'passphrase':pass_hash, + 'salt':pass_hash[7:29], # for bcrypt + } + new_user = User(di) + cfg.users.set(username,new_user) if error: cfg.log(error)