Skip to content

Commit

Permalink
Store hashed password in cookie to avoid re-hashing at every request
Browse files Browse the repository at this point in the history
The only drawback is that if admins change a user's password from
plaintext to hashed or vice versa that user will have to log in again.
Attackers knowing hashed passwords shouldn't be a problem as the server
only accepts it if it's signed (together with a recent timestamp) with
its secret key.

Fixes cms-dev#1076.
  • Loading branch information
lw authored and stefano-maggiolo committed Jan 7, 2019
1 parent 229550d commit 8cc03cb
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 33 deletions.
54 changes: 28 additions & 26 deletions cms/server/contest/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,35 +44,18 @@
logger = logging.getLogger(__name__)


def safe_validate_password(participation, password):
"""Check that the password is correct for the authentication.
Validate the given password against the participation (using either
the global or the contest-specific password that is stored in the
database), and guard against a misconfiguration.
def get_password(participation):
"""Return the password the participation can log in with.
participation (Participation): a participation.
password (str): a password provided by someone trying to log in
claiming to be the given participation.
return (bool): whether the password matches the expected one.
return (str): the password that is on record for them.
"""
if participation.password is None:
correct_password = participation.user.password
return participation.user.password
else:
correct_password = participation.password

try:
password_valid = validate_password(correct_password, password)
except ValueError as e:
# This is either a programming or a configuration error.
logger.warning(
"Invalid password stored in database for user %s in contest %s: "
"%s", participation.user.username, participation.contest.name, e)
return False

return password_valid
return participation.password


def validate_login(
Expand Down Expand Up @@ -124,7 +107,18 @@ def log_failed_attempt(msg, *args):
log_failed_attempt("user not registered to contest")
return None, None

if not safe_validate_password(participation, password):
correct_password = get_password(participation)

try:
password_valid = validate_password(correct_password, password)
except ValueError as e:
# This is either a programming or a configuration error.
logger.warning(
"Invalid password stored in database for user %s in contest %s: "
"%s", participation.user.username, participation.contest.name, e)
return None, None

if not password_valid:
log_failed_attempt("wrong password")
return None, None

Expand All @@ -141,8 +135,10 @@ def log_failed_attempt(msg, *args):
"contest %s, at %s", ip_address, username, contest.name,
timestamp)

# If hashing is used, the cookie stores the hashed password so that
# the expensive bcrypt call doesn't need to be done at every request.
return (participation,
json.dumps([username, password, make_timestamp(timestamp)])
json.dumps([username, correct_password, make_timestamp(timestamp)])
.encode("utf-8"))


Expand Down Expand Up @@ -341,14 +337,20 @@ def log_failed_attempt(msg, *args):
log_failed_attempt("user not registered to contest")
return None, None

if not safe_validate_password(participation, password):
correct_password = get_password(participation)

# We compare hashed password because it would be too expensive to
# re-hash the user-provided plaintext password at every request.
if password != correct_password:
log_failed_attempt("wrong password")
return None, None

logger.info("Successful cookie authentication as user %r, on contest %s, "
"returning from %s, at %s", username, contest.name, last_update,
timestamp)

# We store the hashed password (if hashing is used) so that the
# expensive bcrypt hashing doesn't need to be done at every request.
return (participation,
json.dumps([username, password, make_timestamp(timestamp)])
json.dumps([username, correct_password, make_timestamp(timestamp)])
.encode("utf-8"))
7 changes: 0 additions & 7 deletions cmstestsuite/unit_tests/server/contest/authentication_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,7 @@ def test_cookie_contains_password(self):
# Cookies are of no use if one cannot login by password.
self.contest.allow_password_authentication = False
self.assertFailure()

# The cookie works with all methods as it holds the plaintext password.
self.contest.allow_password_authentication = True
self.user.password = hash_password("mypass", method="bcrypt")
self.assertSuccessAndCookieRefreshed()

self.user.password = hash_password("mypass", method="plaintext")
self.assertSuccessAndCookieRefreshed()

# Cookies contain the password, which is validated every time.
self.user.password = build_password("newpass")
Expand Down

0 comments on commit 8cc03cb

Please sign in to comment.