From 6d4831a6dc3ccfee668145c893ff6a25d188035f Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Jan 18 2021 10:06:35 +0000
Subject: web: input validation


Fix for CVE-2020-15856

Fixes: https://pagure.io/koji/issue/2645

---

diff --git a/www/kojiweb/index.py b/www/kojiweb/index.py
index efb403d..a85f18c 100644
--- a/www/kojiweb/index.py
+++ b/www/kojiweb/index.py
@@ -42,6 +42,34 @@ def _sortbyname(x):
     return x['name']
 
 
+# regexps for input checking
+_VALID_SEARCH_CHARS = r"""a-zA-Z0-9"""
+_VALID_SEARCH_SYMS = r""" @.,_/\()%+-~*?|[]^$"""
+_VALID_SEARCH_RE = re.compile('^[' + _VALID_SEARCH_CHARS + re.escape(_VALID_SEARCH_SYMS) + ']+$')
+
+_VALID_ARCH_RE = re.compile(r'^[\w-]+$', re.ASCII)
+
+
+def _validate_arch(arch):
+    # archs (ASCII alnum + _ + -)
+    if not arch:
+        return None
+    elif _VALID_ARCH_RE.match(arch):
+        return arch
+    else:
+        raise koji.GenericError("Invalid arch: %r" % arch)
+
+
+def _validate_name_or_id(value):
+    # integer ID or label, it is unicode alnum + search symbols (reasonable expectation?)
+    if value.isdigit():
+        return int(value)
+    elif _VALID_SEARCH_RE.match(value):
+        return value
+    else:
+        raise koji.GenericError("Invalid int/label value: %r" % value)
+
+
 # loggers
 authlogger = logging.getLogger('koji.auth')
 
@@ -496,10 +524,12 @@ def tasks(environ, owner=None, state='active', view='tree', method='all', hostID
     values = _initValues(environ, 'Tasks', 'tasks')
     server = _getServer(environ)
 
+    if view not in ('tree', 'toplevel', 'flat'):
+        raise koji.GenericError("Invalid value for view: %r" % view)
+
     opts = {'decode': True}
     if owner:
-        if owner.isdigit():
-            owner = int(owner)
+        owner = _validate_name_or_id(owner)
         ownerObj = server.getUser(owner, strict=True)
         opts['owner'] = ownerObj['id']
         values['owner'] = ownerObj['name']
@@ -567,10 +597,7 @@ def tasks(environ, owner=None, state='active', view='tree', method='all', hostID
         values['hostID'] = None
 
     if channelID:
-        try:
-            channelID = int(channelID)
-        except ValueError:
-            pass
+        channelID = _validate_name_or_id(channelID)
         channel = server.getChannel(channelID, strict=True)
         opts['channel_id'] = channel['id']
         values['channel'] = channel
@@ -857,7 +884,10 @@ def tags(environ, start=None, order=None, childID=None):
     else:
         values['perms'] = []
 
-    values['childID'] = childID
+    if childID is None:
+        values['childID'] = None
+    else:
+        values['childID'] = int(childID)
 
     return _genHTML(environ, 'tags.chtml')
 
@@ -871,15 +901,13 @@ def packages(environ, tagID=None, userID=None, order='package_name', start=None,
     server = _getServer(environ)
     tag = None
     if tagID is not None:
-        if tagID.isdigit():
-            tagID = int(tagID)
+        tagID = _validate_name_or_id(tagID)
         tag = server.getTag(tagID, strict=True)
     values['tagID'] = tagID
     values['tag'] = tag
     user = None
     if userID is not None:
-        if userID.isdigit():
-            userID = int(userID)
+        userID = _validate_name_or_id(userID)
         user = server.getUser(userID, strict=True)
     values['userID'] = userID
     values['user'] = user
@@ -909,8 +937,7 @@ def packageinfo(environ, packageID, tagOrder='name', tagStart=None, buildOrder='
     values = _initValues(environ, 'Package Info', 'packages')
     server = _getServer(environ)
 
-    if packageID.isdigit():
-        packageID = int(packageID)
+    packageID = _validate_name_or_id(packageID)
     package = server.getPackage(packageID)
     if package is None:
         raise koji.GenericError('invalid package ID: %s' % packageID)
@@ -934,8 +961,7 @@ def taginfo(environ, tagID, all='0', packageOrder='package_name', packageStart=N
     values = _initValues(environ, 'Tag Info', 'tags')
     server = _getServer(environ)
 
-    if tagID.isdigit():
-        tagID = int(tagID)
+    tagID = _validate_name_or_id(tagID)
     tag = server.getTag(tagID, strict=True)
 
     values['title'] = tag['name'] + ' | Tag Info'
@@ -1149,8 +1175,7 @@ def externalrepoinfo(environ, extrepoID):
     values = _initValues(environ, 'External Repo Info', 'tags')
     server = _getServer(environ)
 
-    if extrepoID.isdigit():
-        extrepoID = int(extrepoID)
+    extrepoID = _validate_name_or_id(extrepoID)
     extRepo = server.getExternalRepo(extrepoID, strict=True)
     repoTags = server.getTagExternalRepos(repo_info=extRepo['id'])
 
@@ -1301,8 +1326,7 @@ def builds(environ, userID=None, tagID=None, packageID=None, state=None, order='
 
     user = None
     if userID:
-        if userID.isdigit():
-            userID = int(userID)
+        userID = _validate_name_or_id(userID)
         user = server.getUser(userID, strict=True)
     values['userID'] = userID
     values['user'] = user
@@ -1314,16 +1338,14 @@ def builds(environ, userID=None, tagID=None, packageID=None, state=None, order='
 
     tag = None
     if tagID:
-        if tagID.isdigit():
-            tagID = int(tagID)
+        tagID = _validate_name_or_id(tagID)
         tag = server.getTag(tagID, strict=True)
     values['tagID'] = tagID
     values['tag'] = tag
 
     package = None
     if packageID:
-        if packageID.isdigit():
-            packageID = int(packageID)
+        packageID = _validate_name_or_id(packageID)
         package = server.getPackage(packageID, strict=True)
     values['packageID'] = packageID
     values['package'] = package
@@ -1409,8 +1431,7 @@ def userinfo(environ, userID, packageOrder='package_name', packageStart=None,
     values = _initValues(environ, 'User Info', 'users')
     server = _getServer(environ)
 
-    if userID.isdigit():
-        userID = int(userID)
+    userID = _validate_name_or_id(userID)
     user = server.getUser(userID, strict=True)
 
     values['title'] = user['name'] + ' | User Info'
@@ -1438,7 +1459,10 @@ def rpminfo(environ, rpmID, fileOrder='name', fileStart=None, buildrootOrder='-i
     server = _getServer(environ)
 
     rpmID = int(rpmID)
-    rpm = server.getRPM(rpmID)
+    try:
+        rpm = server.getRPM(rpmID, strict=True)
+    except koji.GenericError:
+        raise koji.GenericError('invalid RPM ID: %i' % rpmID)
 
     values['title'] = '%(name)s-%%s%(version)s-%(release)s.%(arch)s.rpm' % rpm + ' | RPM Info'
     epochStr = ''
@@ -1621,8 +1645,7 @@ def hostinfo(environ, hostID=None, userID=None):
     server = _getServer(environ)
 
     if hostID:
-        if hostID.isdigit():
-            hostID = int(hostID)
+        hostID = _validate_name_or_id(hostID)
         host = server.getHost(hostID)
         if host is None:
             raise koji.GenericError('invalid host ID: %s' % hostID)
@@ -1757,8 +1780,7 @@ def channelinfo(environ, channelID):
     return _genHTML(environ, 'channelinfo.chtml')
 
 
-def buildrootinfo(environ, buildrootID, builtStart=None, builtOrder=None, componentStart=None,
-                  componentOrder=None):
+def buildrootinfo(environ, buildrootID):
     values = _initValues(environ, 'Buildroot Info', 'hosts')
     server = _getServer(environ)
 
@@ -2052,6 +2074,9 @@ def rpmsbyhost(environ, start=None, order=None, hostArch=None, rpmArch=None):
     values = _initValues(environ, 'RPMs by Host', 'reports')
     server = _getServer(environ)
 
+    hostArch = _validate_arch(hostArch)
+    rpmArch = _validate_arch(rpmArch)
+
     maxRPMs = 1
     hostArchFilter = hostArch
     if hostArchFilter == 'ix86':
@@ -2126,6 +2151,7 @@ def tasksbyhost(environ, start=None, order='-tasks', hostArch=None):
 
     maxTasks = 1
 
+    hostArch = _validate_arch(hostArch)
     hostArchFilter = hostArch
     if hostArchFilter == 'ix86':
         hostArchFilter = ['i386', 'i486', 'i586', 'i686']
@@ -2273,6 +2299,7 @@ def _filter_hosts_by_arch(hosts, arch):
 
 
 def clusterhealth(environ, arch='__all__'):
+    arch = _validate_arch(arch)
     values = _initValues(environ, 'Cluster health', 'reports')
     server = _getServer(environ)
     channels = server.listChannels()
@@ -2329,21 +2356,18 @@ def recentbuilds(environ, user=None, tag=None, package=None):
 
     tagObj = None
     if tag is not None:
-        if tag.isdigit():
-            tag = int(tag)
-        tagObj = server.getTag(tag)
+        tag = _validate_name_or_id(tag)
+        tagObj = server.getTag(tag, strict=True)
 
     userObj = None
     if user is not None:
-        if user.isdigit():
-            user = int(user)
-        userObj = server.getUser(user)
+        user = _validate_name_or_id(user)
+        userObj = server.getUser(user, strict=True)
 
     packageObj = None
     if package:
-        if package.isdigit():
-            package = int(package)
-        packageObj = server.getPackage(package)
+        package = _validate_name_or_id(package)
+        packageObj = server.getPackage(package, strict=True)
 
     if tagObj is not None:
         builds = server.listTagged(tagObj['id'], inherit=True,
@@ -2404,9 +2428,6 @@ _infoURLs = {'package': 'packageinfo?packageID=%(id)i',
              'maven': 'archiveinfo?archiveID=%(id)i',
              'win': 'archiveinfo?archiveID=%(id)i'}
 
-_VALID_SEARCH_CHARS = r"""a-zA-Z0-9"""
-_VALID_SEARCH_SYMS = r""" @.,_/\()%+-~*?|[]^$"""
-_VALID_SEARCH_RE = re.compile('^[' + _VALID_SEARCH_CHARS + re.escape(_VALID_SEARCH_SYMS) + ']+$')
 _DEFAULT_SEARCH_ORDER = {
     # For searches against large tables, use '-id' to show most recent first
     'build': '-id',
@@ -2422,6 +2443,8 @@ _DEFAULT_SEARCH_ORDER = {
 
 
 def search(environ, start=None, order=None):
+    if start is not None:
+        start = int(start)
     values = _initValues(environ, 'Search', 'search')
     server = _getServer(environ)
     values['error'] = None
@@ -2436,10 +2459,14 @@ def search(environ, start=None, order=None):
         values['type'] = type
         values['match'] = match
 
+        if match not in ('glob', 'regexp', 'exact'):
+            raise koji.GenericError("Invalid match type: %r" % match)
+
         if not _VALID_SEARCH_RE.match(terms):
             values['error'] = 'Invalid search terms<br/>' + \
                 'Search terms may contain only these characters: ' + \
                 _VALID_SEARCH_CHARS + _VALID_SEARCH_SYMS
+            values['terms'] = ''
             return _genHTML(environ, 'search.chtml')
 
         if match == 'regexp':
@@ -2447,6 +2474,7 @@ def search(environ, start=None, order=None):
                 re.compile(terms)
             except Exception:
                 values['error'] = 'Invalid regular expression'
+                values['terms'] = ''
                 return _genHTML(environ, 'search.chtml')
 
         infoURL = _infoURLs.get(type)
@@ -2492,9 +2520,9 @@ def api(environ):
 def watchlogs(environ, taskID):
     values = _initValues(environ)
     if isinstance(taskID, list):
-        values['tasks'] = ', '.join(taskID)
+        values['tasks'] = ', '.join([int(x) for x in taskID])
     else:
-        values['tasks'] = taskID
+        values['tasks'] = int(taskID)
 
     html = """
 <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
@@ -2518,6 +2546,7 @@ def repoinfo(environ, repoID):
     values = _initValues(environ, 'Repo Info', 'tags')
     server = _getServer(environ)
 
+    repoID = _validate_name_or_id(repoID)
     values['repo_id'] = repoID
     repo_info = server.repoInfo(repoID, strict=False)
     values['repo'] = repo_info
diff --git a/www/lib/kojiweb/util.py b/www/lib/kojiweb/util.py
index 548f23a..301fd92 100644
--- a/www/lib/kojiweb/util.py
+++ b/www/lib/kojiweb/util.py
@@ -22,6 +22,7 @@
 import datetime
 import hashlib
 import os
+import re
 import ssl
 import stat
 import xmlrpc.client
@@ -49,6 +50,9 @@ except Exception:
 themeInfo = {}
 themeCache = {}
 
+# allowed values for SQL ordering (e.g. -id, package_name, etc.)
+RE_ORDER = re.compile(r'^-?\w+$')
+
 
 def _initValues(environ, title='Build System Info', pageID='summary'):
     global themeInfo
@@ -290,6 +294,8 @@ def paginateList(values, data, start, dataName, prefix=None, order=None, noneGre
     be added to the value map.
     """
     if order is not None:
+        if not RE_ORDER.match(order):
+            raise ValueError("Ordering is not alphanumeric: %r" % order)
         if order.startswith('-'):
             order = order[1:]
             reverse = True
@@ -327,6 +333,8 @@ def paginateMethod(server, values, methodName, args=None, kw=None,
         start = 0
     if not dataName:
         raise Exception('dataName must be specified')
+    if not RE_ORDER.match(order):
+        raise ValueError("Ordering is not alphanumeric: %r" % order)
 
     kw['queryOpts'] = {'countOnly': True}
     totalRows = getattr(server, methodName)(*args, **kw)
@@ -358,6 +366,8 @@ def paginateResults(server, values, methodName, args=None, kw=None,
         start = 0
     if not dataName:
         raise Exception('dataName must be specified')
+    if not RE_ORDER.match(order):
+        raise ValueError("Ordering is not alphanumeric: %r" % order)
 
     kw['filterOpts'] = {'order': order,
                         'offset': start,

