Skip to content
Browse files

Allow users to customize the sanity-checking regexes

This doesn't add security, but at least allows users to open up the
regexes where it's safe to do so.  Fixes #985.

Tested by hand.
  • Loading branch information...
1 parent 5ffe6a6 commit cea9873e7fbe793720e55043c1c7fc4aebff0bd9 @djmitche djmitche committed
View
7 master/NEWS
@@ -4,6 +4,13 @@ Major User visible changes in Buildbot. -*- outline -*-
* Next Version
+
+** Customizable validation regexps
+
+The global c['validation'] parameter can be used to adjust the regular
+expressions used to validate branches, revisions, and properties input by the
+user.
+
** Logging for SVNPoller cleaned up
All logging for SVNPoller now starts with "SVNPoller: ". Previously it was
View
1 master/buildbot/config.py
@@ -22,6 +22,7 @@ class MasterConfig(object):
available at C{master.config}.
@ivar changeHorizon: the current change horizon
+ @ivar validation: regexes for preventing invalid inputs
"""
changeHorizon = None
View
18 master/buildbot/master.py
@@ -14,6 +14,7 @@
# Copyright Buildbot Team Members
+import re
import os
import signal
import time
@@ -308,6 +309,22 @@ def do_load(_):
metrics_config = config.get("metrics")
caches_config = config.get("caches", {})
+ # load validation, with defaults, and verify no unrecognized
+ # keys are included.
+ validation_defaults = {
+ 'branch' : re.compile(r'^[\w.+/~-]*$'),
+ 'revision' : re.compile(r'^[ \w\.\-\/]*$'),
+ 'property_name' : re.compile(r'^[\w\.\-\/\~:]*$'),
+ 'property_value' : re.compile(r'^[\w\.\-\/\~:]*$'),
+ }
+ validation_config = validation_defaults.copy()
+ validation_config.update(config.get("validation", {}))
+ v_config_keys = set(validation_config.keys())
+ v_default_keys = set(validation_defaults.keys())
+ if v_config_keys > v_default_keys:
+ raise ValueError("unrecognized validation key(s): %s" %
+ (", ".join(v_config_keys - v_default_keys,)))
+
except KeyError:
log.msg("config dictionary is missing a required parameter")
log.msg("leaving old configuration in place")
@@ -335,6 +352,7 @@ def do_load(_):
raise KeyError("must have a 'slaves' key")
self.config.changeHorizon = changeHorizon
+ self.config.validation = validation_config
change_source = config.get('change_source', [])
if isinstance(change_source, (list, tuple)):
View
17 master/buildbot/status/web/base.py
@@ -62,11 +62,14 @@ class IHTMLLog(Interface):
def getAndCheckProperties(req):
"""
-Fetch custom build properties from the HTTP request of a "Force build" or
-"Resubmit build" HTML form.
-Check the names for valid strings, and return None if a problem is found.
-Return a new Properties object containing each property found in req.
-"""
+ Fetch custom build properties from the HTTP request of a "Force build" or
+ "Resubmit build" HTML form.
+ Check the names for valid strings, and return None if a problem is found.
+ Return a new Properties object containing each property found in req.
+ """
+ master = req.site.buildbot_service.master
+ pname_validate = master.config.validation['property_name']
+ pval_validate = master.config.validation['property_value']
properties = Properties()
i = 1
while True:
@@ -74,8 +77,8 @@ def getAndCheckProperties(req):
pvalue = req.args.get("property%dvalue" % i, [""])[0]
if not pname:
break
- if not re.match(r'^[\w\.\-\/\~:]*$', pname) \
- or not re.match(r'^[\w\.\-\/\~:]*$', pvalue):
+ if not pname_validate.match(pname) \
+ or not pval_validate.match(pvalue):
log.msg("bad property name='%s', value='%s'" % (pname, pvalue))
return None
properties.setProperty(pname, pvalue, "Force Build Form")
View
12 master/buildbot/status/web/builder.py
@@ -16,7 +16,7 @@
from twisted.web import html
from twisted.web.util import Redirect
-import re, urllib, time
+import urllib, time
from twisted.python import log
from twisted.internet import defer
from buildbot import interfaces
@@ -153,12 +153,15 @@ def force(self, req, auth_ok=False):
log.msg("..but not authorized")
return Redirect(path_to_authfail(req))
+ master = self.getBuildmaster(req)
+
# keep weird stuff out of the branch revision, and property strings.
- # TODO: centralize this somewhere.
- if not re.match(r'^[\w.+/~-]*$', branch):
+ branch_validate = master.config.validation['branch']
+ revision_validate = master.config.validation['revision']
+ if not branch_validate.match(branch):
log.msg("bad branch '%s'" % branch)
return Redirect(path_to_builder(req, self.builder_status))
- if not re.match(r'^[ \w\.\-\/]*$', revision):
+ if not revision_validate.match(r'^[ \w\.\-\/]*$', revision):
log.msg("bad revision '%s'" % revision)
return Redirect(path_to_builder(req, self.builder_status))
properties = getAndCheckProperties(req)
@@ -169,7 +172,6 @@ def force(self, req, auth_ok=False):
if not revision:
revision = None
- master = self.getBuildmaster(req)
d = master.db.sourcestamps.addSourceStamp(branch=branch,
revision=revision, project=project, repository=repository)
def make_buildset(ssid):
View
8 master/buildbot/status/words.py
@@ -89,6 +89,7 @@ class Contact(base.StatusReceiver):
def __init__(self, channel):
#StatusReceiver.__init__(self) doesn't exist
self.channel = channel
+ self.master = channel.master
self.notify_events = {}
self.subscribed = 0
self.muted = False
@@ -451,12 +452,13 @@ def command_FORCE(self, args, who):
raise UsageError("you must provide a Builder, " + errReply)
# keep weird stuff out of the branch and revision strings.
- # TODO: centralize this somewhere.
- if branch and not re.match(r'^[\w\.\-\/]*$', branch):
+ branch_validate = self.master.config.validation['branch']
+ revision_validate = self.master.config.validation['revision']
+ if branch and not branch_validate.match(branch):
log.msg("bad branch '%s'" % branch)
self.send("sorry, bad branch '%s'" % branch)
return
- if revision and not re.match(r'^[\w\.\-\/]*$', revision):
+ if revision and not revision_validate.match(revision):
log.msg("bad revision '%s'" % revision)
self.send("sorry, bad revision '%s'" % revision)
return
View
24 master/docs/cfg-global.texinfo
@@ -12,6 +12,7 @@ The keys in this section affect the operations of the buildmaster globally.
* Defining Global Properties::
* Debug Options::
* Metrics Options::
+* Input Validation::
@end menu
@node Database Specification
@@ -572,3 +573,26 @@ data attributes of <Builder ''builder'' at 48963528>
@code{periodic_interval} determines how often various non-event based metrics are collected, such as memory usage, uncollectable garbage, reactor delay. This defaults to 10s. If set to 0 or None, then periodic collection of this data is disabled. This value can also be changed via a reconifg.
Read more about metrics in the @ref{Metrics} section of the documentation.
+
+@node Input Validation
+@subsection Input Validation
+@bcindex c['validation']
+
+@example
+import re
+c['validation'] = @{
+ 'branch' : re.compile(r'^[\w.+/~-]*$'),
+ 'revision' : re.compile(r'^[ \w\.\-\/]*$'),
+ 'property_name' : re.compile(r'^[\w\.\-\/\~:]*$'),
+ 'property_value' : re.compile(r'^[\w\.\-\/\~:]*$'),
+@}
+@end example
+
+This option configures the validation applied to user inputs of various types.
+This validation is important since these values are often included in
+command-line arguments executed on slaves. Allowing arbitrary input from
+untrusted users may raise security concerns.
+
+The keys describe the type of input validated; the values are compiled regular
+expressions against which the input will be matched. The defaults for each
+type of input are those given in the example, above.

0 comments on commit cea9873

Please sign in to comment.
Something went wrong with that request. Please try again.