From 404fe1018e08e546fd14c83741e00b900c1cd208 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Wed, 7 Feb 2024 13:09:54 +0200 Subject: [PATCH] rpcserver: validate Kerberos principal name before running kinit Do minimal validation of the Kerberos principal name when passing it to kinit command line tool. Also pass it as the final argument to prevent option injection. Accepted Kerberos principals are: - user names, using the following regexp (username with optional @realm, no spaces or slashes in the name): "(?!^[0-9]+$)^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*[a-zA-Z0-9_.$-]?@?[a-zA-Z0-9.-]*$" - service names (with slash in the name but no spaces). Validation of the hostname is done. There is no validation of the service name. The regular expression above also covers cases where a principal name starts with '-'. This prevents option injection as well. This fixes CVE-2024-1481 Fixes: https://pagure.io/freeipa/issue/9541 Signed-off-by: Alexander Bokovoy Signed-off-by: Rob Crittenden Reviewed-By: Florence Blanc-Renaud --- ipalib/install/kinit.py | 47 ++++++++++++++++++- ipaserver/rpcserver.py | 9 ++-- ipatests/prci_definitions/gating.yaml | 12 +++++ ipatests/prci_definitions/nightly_latest.yaml | 12 +++++ .../nightly_latest_selinux.yaml | 13 +++++ .../nightly_latest_testing.yaml | 14 ++++++ .../nightly_latest_testing_selinux.yaml | 15 ++++++ .../prci_definitions/nightly_previous.yaml | 12 +++++ .../prci_definitions/nightly_rawhide.yaml | 13 +++++ ipatests/setup.py | 1 + ipatests/test_ipalib_install/__init__.py | 0 ipatests/test_ipalib_install/test_kinit.py | 29 ++++++++++++ tox.ini | 2 +- 13 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 ipatests/test_ipalib_install/__init__.py create mode 100644 ipatests/test_ipalib_install/test_kinit.py diff --git a/ipalib/install/kinit.py b/ipalib/install/kinit.py index cc839ec38..4ad4eaa1c 100644 --- a/ipalib/install/kinit.py +++ b/ipalib/install/kinit.py @@ -6,12 +6,16 @@ from __future__ import absolute_import import logging import os +import re import time import gssapi from ipaplatform.paths import paths from ipapython.ipautil import run +from ipalib.constants import PATTERN_GROUPUSER_NAME +from ipalib.util import validate_hostname +from ipalib import api logger = logging.getLogger(__name__) @@ -21,6 +25,40 @@ KRB5_KDC_UNREACH = 2529639068 # A service is not available that s required to process the request KRB5KDC_ERR_SVC_UNAVAILABLE = 2529638941 +PATTERN_REALM = '@?([a-zA-Z0-9.-]*)$' +PATTERN_PRINCIPAL = '(' + PATTERN_GROUPUSER_NAME[:-1] + ')' + PATTERN_REALM +PATTERN_SERVICE = '([a-zA-Z0-9.-]+)/([a-zA-Z0-9.-]+)' + PATTERN_REALM + +user_pattern = re.compile(PATTERN_PRINCIPAL) +service_pattern = re.compile(PATTERN_SERVICE) + + +def validate_principal(principal): + if not isinstance(principal, str): + raise RuntimeError('Invalid principal: not a string') + if ('/' in principal) and (' ' in principal): + raise RuntimeError('Invalid principal: bad spacing') + else: + realm = None + match = user_pattern.match(principal) + if match is None: + match = service_pattern.match(principal) + if match is None: + raise RuntimeError('Invalid principal: cannot parse') + else: + # service = match[1] + hostname = match[2] + realm = match[3] + try: + validate_hostname(hostname) + except ValueError as e: + raise RuntimeError(str(e)) + else: # user match, validate realm + # username = match[1] + realm = match[2] + if realm and 'realm' in api.env and realm != api.env.realm: + raise RuntimeError('Invalid principal: realm mismatch') + def kinit_keytab(principal, keytab, ccache_name, config=None, attempts=1): """ @@ -29,6 +67,7 @@ def kinit_keytab(principal, keytab, ccache_name, config=None, attempts=1): The optional parameter 'attempts' specifies how many times the credential initialization should be attempted in case of non-responsive KDC. """ + validate_principal(principal) errors_to_retry = {KRB5KDC_ERR_SVC_UNAVAILABLE, KRB5_KDC_UNREACH} logger.debug("Initializing principal %s using keytab %s", @@ -65,6 +104,7 @@ def kinit_keytab(principal, keytab, ccache_name, config=None, attempts=1): return None + def kinit_password(principal, password, ccache_name, config=None, armor_ccache_name=None, canonicalize=False, enterprise=False, lifetime=None): @@ -73,8 +113,9 @@ def kinit_password(principal, password, ccache_name, config=None, web-based authentication, use armor_ccache_path to specify http service ccache. """ + validate_principal(principal) logger.debug("Initializing principal %s using password", principal) - args = [paths.KINIT, principal, '-c', ccache_name] + args = [paths.KINIT, '-c', ccache_name] if armor_ccache_name is not None: logger.debug("Using armor ccache %s for FAST webauth", armor_ccache_name) @@ -91,6 +132,7 @@ def kinit_password(principal, password, ccache_name, config=None, logger.debug("Using enterprise principal") args.append('-E') + args.extend(['--', principal]) env = {'LC_ALL': 'C'} if config is not None: env['KRB5_CONFIG'] = config @@ -154,6 +196,7 @@ def kinit_pkinit( :raises: CalledProcessError if PKINIT fails """ + validate_principal(principal) logger.debug( "Initializing principal %s using PKINIT %s", principal, user_identity ) @@ -168,7 +211,7 @@ def kinit_pkinit( assert pkinit_anchor.startswith(("FILE:", "DIR:", "ENV:")) args.extend(["-X", f"X509_anchors={pkinit_anchor}"]) args.extend(["-X", f"X509_user_identity={user_identity}"]) - args.append(principal) + args.extend(['--', principal]) # this workaround enables us to capture stderr and put it # into the raised exception in case of unsuccessful authentication diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py index 198fc9e7d..4f65b7e05 100644 --- a/ipaserver/rpcserver.py +++ b/ipaserver/rpcserver.py @@ -1135,10 +1135,6 @@ class login_password(Backend, KerberosSession): canonicalize=True, lifetime=self.api.env.kinit_lifetime) - if armor_path: - logger.debug('Cleanup the armor ccache') - ipautil.run([paths.KDESTROY, '-A', '-c', armor_path], - env={'KRB5CCNAME': armor_path}, raiseonerr=False) except RuntimeError as e: if ('kinit: Cannot read password while ' 'getting initial credentials') in str(e): @@ -1156,6 +1152,11 @@ class login_password(Backend, KerberosSession): raise KrbPrincipalWrongFAST(principal=principal) raise InvalidSessionPassword(principal=principal, message=unicode(e)) + finally: + if armor_path: + logger.debug('Cleanup the armor ccache') + ipautil.run([paths.KDESTROY, '-A', '-c', armor_path], + env={'KRB5CCNAME': armor_path}, raiseonerr=False) class change_password(Backend, HTTP_Status): diff --git a/ipatests/prci_definitions/gating.yaml b/ipatests/prci_definitions/gating.yaml index f208eb8cc..5921c8c3f 100644 --- a/ipatests/prci_definitions/gating.yaml +++ b/ipatests/prci_definitions/gating.yaml @@ -310,3 +310,15 @@ jobs: template: *ci-master-latest timeout: 3600 topology: *master_1repl_1client + + fedora-latest/test_ipalib_install: + requires: [fedora-latest/build] + priority: 100 + job: + class: RunPytest + args: + build_url: '{fedora-latest/build_url}' + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-latest + timeout: 600 + topology: *master_1repl diff --git a/ipatests/prci_definitions/nightly_latest.yaml b/ipatests/prci_definitions/nightly_latest.yaml index 189203783..8c39b451c 100644 --- a/ipatests/prci_definitions/nightly_latest.yaml +++ b/ipatests/prci_definitions/nightly_latest.yaml @@ -1897,3 +1897,15 @@ jobs: template: *ci-master-latest timeout: 10800 topology: *master_1repl + + fedora-latest/test_ipalib_install: + requires: [fedora-latest/build] + priority: 50 + job: + class: RunPytest + args: + build_url: '{fedora-latest/build_url}' + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-latest + timeout: 600 + topology: *master_1repl diff --git a/ipatests/prci_definitions/nightly_latest_selinux.yaml b/ipatests/prci_definitions/nightly_latest_selinux.yaml index 887a5d95c..cbbdeb8af 100644 --- a/ipatests/prci_definitions/nightly_latest_selinux.yaml +++ b/ipatests/prci_definitions/nightly_latest_selinux.yaml @@ -2048,3 +2048,16 @@ jobs: template: *ci-master-latest timeout: 10800 topology: *master_1repl + + fedora-latest/test_ipalib_install: + requires: [fedora-latest/build] + priority: 50 + job: + class: RunPytest + args: + build_url: '{fedora-latest/build_url}' + selinux_enforcing: True + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-latest + timeout: 600 + topology: *master_1repl diff --git a/ipatests/prci_definitions/nightly_latest_testing.yaml b/ipatests/prci_definitions/nightly_latest_testing.yaml index c525a3580..25805b047 100644 --- a/ipatests/prci_definitions/nightly_latest_testing.yaml +++ b/ipatests/prci_definitions/nightly_latest_testing.yaml @@ -2200,3 +2200,17 @@ jobs: template: *ci-master-latest timeout: 10800 topology: *master_1repl + + testing-fedora/test_ipalib_install: + requires: [testing-fedora/build] + priority: 50 + job: + class: RunPytest + args: + build_url: '{testing-fedora/build_url}' + update_packages: True + enable_testing_repo: True + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-latest + timeout: 600 + topology: *master_1repl diff --git a/ipatests/prci_definitions/nightly_latest_testing_selinux.yaml b/ipatests/prci_definitions/nightly_latest_testing_selinux.yaml index 40150b2bb..e4cf61d23 100644 --- a/ipatests/prci_definitions/nightly_latest_testing_selinux.yaml +++ b/ipatests/prci_definitions/nightly_latest_testing_selinux.yaml @@ -2351,3 +2351,18 @@ jobs: template: *ci-master-latest timeout: 10800 topology: *master_1repl + + testing-fedora/test_ipalib_install: + requires: [testing-fedora/build] + priority: 50 + job: + class: RunPytest + args: + build_url: '{testing-fedora/build_url}' + update_packages: True + selinux_enforcing: True + enable_testing_repo: True + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-latest + timeout: 600 + topology: *master_1repl diff --git a/ipatests/prci_definitions/nightly_previous.yaml b/ipatests/prci_definitions/nightly_previous.yaml index 0971888d6..d955c3600 100644 --- a/ipatests/prci_definitions/nightly_previous.yaml +++ b/ipatests/prci_definitions/nightly_previous.yaml @@ -1897,3 +1897,15 @@ jobs: template: *ci-master-previous timeout: 10800 topology: *master_1repl + + fedora-previous/test_ipalib_install: + requires: [fedora-previous/build] + priority: 50 + job: + class: RunPytest + args: + build_url: '{fedora-previous/build_url}' + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-previous + timeout: 600 + topology: *master_1repl diff --git a/ipatests/prci_definitions/nightly_rawhide.yaml b/ipatests/prci_definitions/nightly_rawhide.yaml index dca5abb40..3dbf312b5 100644 --- a/ipatests/prci_definitions/nightly_rawhide.yaml +++ b/ipatests/prci_definitions/nightly_rawhide.yaml @@ -2048,3 +2048,16 @@ jobs: template: *ci-master-frawhide timeout: 10800 topology: *master_1repl + + fedora-rawhide/test_ipalib_install: + requires: [fedora-rawhide/build] + priority: 50 + job: + class: RunPytest + args: + build_url: '{fedora-rawhide/build_url}' + update_packages: True + test_suite: test_ipalib_install/test_kinit.py + template: *ci-master-frawhide + timeout: 600 + topology: *master_1repl diff --git a/ipatests/setup.py b/ipatests/setup.py index 6217a1ba5..0aec4a70d 100644 --- a/ipatests/setup.py +++ b/ipatests/setup.py @@ -41,6 +41,7 @@ if __name__ == '__main__': "ipatests.test_integration", "ipatests.test_ipaclient", "ipatests.test_ipalib", + "ipatests.test_ipalib_install", "ipatests.test_ipaplatform", "ipatests.test_ipapython", "ipatests.test_ipaserver", diff --git a/ipatests/test_ipalib_install/__init__.py b/ipatests/test_ipalib_install/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/ipatests/test_ipalib_install/test_kinit.py b/ipatests/test_ipalib_install/test_kinit.py new file mode 100644 index 000000000..f89ea17d7 --- /dev/null +++ b/ipatests/test_ipalib_install/test_kinit.py @@ -0,0 +1,29 @@ +# +# Copyright (C) 2024 FreeIPA Contributors see COPYING for license +# +"""Tests for ipalib.install.kinit module +""" + +import pytest + +from ipalib.install.kinit import validate_principal + + +# None means no exception is expected +@pytest.mark.parametrize('principal, exception', [ + ('testuser', None), + ('testuser@EXAMPLE.TEST', None), + ('test/ipa.example.test', None), + ('test/ipa.example.test@EXAMPLE.TEST', None), + ('test/ipa@EXAMPLE.TEST', RuntimeError), + ('test/-ipa.example.test@EXAMPLE.TEST', RuntimeError), + ('test/ipa.1example.test@EXAMPLE.TEST', RuntimeError), + ('test /ipa.example,test', RuntimeError), + ('testuser@OTHER.TEST', RuntimeError), + ('test/ipa.example.test@OTHER.TEST', RuntimeError), +]) +def test_validate_principal(principal, exception): + try: + validate_principal(principal) + except Exception as e: + assert e.__class__ == exception diff --git a/tox.ini b/tox.ini index 274d352e8..607f96244 100644 --- a/tox.ini +++ b/tox.ini @@ -19,7 +19,7 @@ deps= ipatests commands= {envbindir}/ipa --help - {envbindir}/ipa-run-tests --junitxml={envdir}/junit-{envname}.xml {posargs:--ipaclient-unittests} + {envbindir}/ipa-run-tests --junitxml={envdir}/junit-{envname}.xml {posargs:--ipaclient-unittests} --ignore test_ipalib_install [testenv:pylint3] basepython=python3