From 2ca4f2aaa89bda070ef7be8522b6296aec5289db Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 7 Sep 2022 13:55:23 -0400 Subject: [PATCH] Clarify the usage of sha1 as a "secure" hash. --- lib/galaxy/managers/users.py | 26 +++++++++++++------------- lib/galaxy/model/__init__.py | 4 ++-- lib/galaxy/tool_util/deps/__init__.py | 2 +- lib/galaxy/util/hash_util.py | 22 +++++++++++++++++++--- lib/tool_shed/util/admin_util.py | 6 +++--- lib/tool_shed/webapp/model/__init__.py | 6 +++--- 6 files changed, 41 insertions(+), 25 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index c5783115e1b..f40a85bbd94 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -40,7 +40,7 @@ from galaxy.structured_app import ( BasicSharedApp, MinimalManagerApp, ) -from galaxy.util.hash_util import new_secure_hash +from galaxy.util.hash_util import new_secure_hash_v2 from galaxy.web import url_for log = logging.getLogger(__name__) @@ -197,8 +197,8 @@ class UserManager(base.ModelManager, deletable.PurgableManagerMixin): # to identify if it is needed for some reason. # # Deleting multiple times will re-hash the username/email - email_hash = new_secure_hash(user.email + pseudorandom_value) - uname_hash = new_secure_hash(user.username + pseudorandom_value) + email_hash = new_secure_hash_v2(user.email + pseudorandom_value) + uname_hash = new_secure_hash_v2(user.username + pseudorandom_value) # We must also redact username for role in user.all_roles(): if self.app.config.redact_username_during_deletion: @@ -219,15 +219,15 @@ class UserManager(base.ModelManager, deletable.PurgableManagerMixin): .all() ) for addr in user_addresses: - addr.desc = new_secure_hash(addr.desc + pseudorandom_value) - addr.name = new_secure_hash(addr.name + pseudorandom_value) - addr.institution = new_secure_hash(addr.institution + pseudorandom_value) - addr.address = new_secure_hash(addr.address + pseudorandom_value) - addr.city = new_secure_hash(addr.city + pseudorandom_value) - addr.state = new_secure_hash(addr.state + pseudorandom_value) - addr.postal_code = new_secure_hash(addr.postal_code + pseudorandom_value) - addr.country = new_secure_hash(addr.country + pseudorandom_value) - addr.phone = new_secure_hash(addr.phone + pseudorandom_value) + addr.desc = new_secure_hash_v2(addr.desc + pseudorandom_value) + addr.name = new_secure_hash_v2(addr.name + pseudorandom_value) + addr.institution = new_secure_hash_v2(addr.institution + pseudorandom_value) + addr.address = new_secure_hash_v2(addr.address + pseudorandom_value) + addr.city = new_secure_hash_v2(addr.city + pseudorandom_value) + addr.state = new_secure_hash_v2(addr.state + pseudorandom_value) + addr.postal_code = new_secure_hash_v2(addr.postal_code + pseudorandom_value) + addr.country = new_secure_hash_v2(addr.country + pseudorandom_value) + addr.phone = new_secure_hash_v2(addr.phone + pseudorandom_value) self.session().add(addr) # Purge the user super().purge(user, flush=flush) @@ -563,7 +563,7 @@ class UserManager(base.ModelManager, deletable.PurgableManagerMixin): user = trans.sa_session.query(self.app.model.User).filter(self.app.model.User.table.c.email == email).first() activation_token = user.activation_token if activation_token is None: - activation_token = util.hash_util.new_secure_hash(str(random.getrandbits(256))) + activation_token = util.hash_util.new_secure_hash_v2(str(random.getrandbits(256))) user.activation_token = activation_token trans.sa_session.add(user) trans.sa_session.flush() diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index e1c3edcc4ad..f3e6fbbbde3 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -146,7 +146,7 @@ from galaxy.util.form_builder import ( WorkflowField, WorkflowMappingField, ) -from galaxy.util.hash_util import new_secure_hash +from galaxy.util.hash_util import new_insecure_hash from galaxy.util.json import safe_loads from galaxy.util.sanitize_html import sanitize_html @@ -638,7 +638,7 @@ class User(Base, Dictifiable, RepresentById): if User.use_pbkdf2: self.password = galaxy.security.passwords.hash_password(cleartext) else: - self.password = new_secure_hash(text_type=cleartext) + self.password = new_insecure_hash(text_type=cleartext) self.last_password_change = now() def set_random_password(self, length=16): diff --git a/lib/galaxy/tool_util/deps/__init__.py b/lib/galaxy/tool_util/deps/__init__.py index c321ecfb220..abddec1ffb2 100644 --- a/lib/galaxy/tool_util/deps/__init__.py +++ b/lib/galaxy/tool_util/deps/__init__.py @@ -431,7 +431,7 @@ class CachedDependencyManager(DependencyManager): (dep.name, dep.version, dep.exact, dep.dependency_type) for dep in resolved_dependencies ] hash_str = json.dumps(sorted(resolved_dependencies)) - return hash_util.new_secure_hash(hash_str)[:8] # short hash + return hash_util.new_insecure_hash(hash_str)[:8] # short hash def get_hashed_dependencies_path(self, resolved_dependencies): """ diff --git a/lib/galaxy/util/hash_util.py b/lib/galaxy/util/hash_util.py index 2413dd1c4c5..53fe69a8aa0 100644 --- a/lib/galaxy/util/hash_util.py +++ b/lib/galaxy/util/hash_util.py @@ -90,9 +90,25 @@ def md5_hash_file(path: Union[str, os.PathLike]) -> Optional[str]: return None -def new_secure_hash(text_type: Union[bytes, str]) -> str: +def new_secure_hash_v2(text_type: Union[bytes, str]) -> str: + """More modern version of new_secure_hash. + + Certain passwords are set via new_insecure_hash (previously new_secure_hash), + so that needs to remain for legacy purposes. """ - Returns the hexdigest of the sha1 hash of the argument `text_type`. + assert text_type is not None + return sha512(smart_str(text_type)).hexdigest() + + +def new_insecure_hash(text_type: Union[bytes, str]) -> str: + """Returns the hexdigest of the sha1 hash of the argument `text_type`. + + Previously called new_secure_hash, but this should not be considered + secure - SHA1 is no longer considered a secure hash and has been broken + since the early 2000s. + + use_pbkdf2 should be set by default and galaxy.security.passwords should + be the default used for passwords in Galaxy. """ assert text_type is not None return sha1(smart_str(text_type)).hexdigest() @@ -110,4 +126,4 @@ def is_hashable(value: Any) -> bool: return True -__all__ = ("md5", "hashlib", "sha1", "sha", "new_secure_hash", "hmac_new", "is_hashable") +__all__ = ("md5", "hashlib", "sha1", "sha", "new_insecure_hash", "new_secure_hash_v2", "hmac_new", "is_hashable") diff --git a/lib/tool_shed/util/admin_util.py b/lib/tool_shed/util/admin_util.py index 3e8eca90341..184b6bc393e 100644 --- a/lib/tool_shed/util/admin_util.py +++ b/lib/tool_shed/util/admin_util.py @@ -13,7 +13,7 @@ from galaxy import ( ) from galaxy.security.validate_user_input import validate_password from galaxy.util import inflector -from galaxy.util.hash_util import new_secure_hash +from galaxy.util.hash_util import new_secure_hash_v2 from galaxy.web.form_builder import CheckboxField from galaxy.web.legacy_framework.grids import ( Grid, @@ -773,8 +773,8 @@ class Admin: compliance_log.info(f"delete-user-event: {user_id}") # See lib/galaxy/webapps/tool_shed/controllers/admin.py pseudorandom_value = str(int(time.time())) - email_hash = new_secure_hash(user.email + pseudorandom_value) - uname_hash = new_secure_hash(user.username + pseudorandom_value) + email_hash = new_secure_hash_v2(user.email + pseudorandom_value) + uname_hash = new_secure_hash_v2(user.username + pseudorandom_value) for role in user.all_roles(): print( role, self.app.config.redact_username_during_deletion, self.app.config.redact_email_during_deletion diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index e0989af283c..810665b404b 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -48,7 +48,7 @@ from galaxy.security.validate_user_input import validate_password_str from galaxy.util import unique_id from galaxy.util.bunch import Bunch from galaxy.util.dictifiable import Dictifiable -from galaxy.util.hash_util import new_secure_hash +from galaxy.util.hash_util import new_insecure_hash from tool_shed.dependencies.repository import relation_builder from tool_shed.util import ( hg_util, @@ -152,7 +152,7 @@ class User(Base, Dictifiable, _HasTable): def check_password(self, cleartext): """Check if 'cleartext' matches 'self.password' when hashed.""" - return self.password == new_secure_hash(text_type=cleartext) + return self.password == new_insecure_hash(text_type=cleartext) def get_disk_usage(self, nice_size=False): return 0 @@ -171,7 +171,7 @@ class User(Base, Dictifiable, _HasTable): if message: raise Exception(f"Invalid password: {message}") # Set 'self.password' to the digest of 'cleartext'. - self.password = new_secure_hash(text_type=cleartext) + self.password = new_insecure_hash(text_type=cleartext) class PasswordResetToken(Base, _HasTable):