#2652 web: input validation
Merged 3 months ago by tkopecek. Opened 3 months ago by tkopecek.
tkopecek/koji xss  into  master

file modified
+73 -44
@@ -42,6 +42,34 @@ 

      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 @@ 

      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 @@ 

          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 @@ 

      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 @@ 

      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 @@ 

      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 @@ 

      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 @@ 

      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 @@ 

  

      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 @@ 

  

      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 @@ 

      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 @@ 

      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 @@ 

      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 @@ 

      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 @@ 

      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 @@ 

  

      maxTasks = 1

  

+     hostArch = _validate_arch(hostArch)

      hostArchFilter = hostArch

      if hostArchFilter == 'ix86':

          hostArchFilter = ['i386', 'i486', 'i586', 'i686']
@@ -2273,6 +2299,7 @@ 

  

  

  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 @@ 

  

      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 @@ 

               '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 @@ 

  

  

  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 @@ 

          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 @@ 

                  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 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 @@ 

      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

file modified
+10
@@ -22,6 +22,7 @@ 

  import datetime

  import hashlib

  import os

+ import re

  import ssl

  import stat

  import xmlrpc.client
@@ -49,6 +50,9 @@ 

  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 @@ 

      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 @@ 

          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 @@ 

          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,

Commit 2be8600 fixes this pull-request

Pull-Request has been merged by tkopecek

3 months ago