Skip to content

Commit 62a1295

Browse files
committed
Security fixes:
* Strip lookup calls out of inventory variables and clean unsafe data returned from lookup plugins (CVE-2014-4966) * Make sure vars don't insert extra parameters into module args and prevent duplicate params from superseding previous params (CVE-2014-4967)
1 parent 69d85c2 commit 62a1295

File tree

8 files changed

+178
-65
lines changed

8 files changed

+178
-65
lines changed

Diff for: lib/ansible/module_utils/basic.py

+2
Original file line numberDiff line numberDiff line change
@@ -772,6 +772,8 @@ def _load_params(self):
772772
(k, v) = x.split("=",1)
773773
except Exception, e:
774774
self.fail_json(msg="this module requires key=value arguments (%s)" % (items))
775+
if k in params:
776+
self.fail_json(msg="duplicate parameter: %s (value=%s)" % (k, v))
775777
params[k] = v
776778
params2 = json.loads(MODULE_COMPLEX_ARGS)
777779
params2.update(params)

Diff for: lib/ansible/runner/__init__.py

+46
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import pipes
3232
import jinja2
3333
import subprocess
34+
import shlex
3435
import getpass
3536

3637
import ansible.constants as C
@@ -394,6 +395,35 @@ def _compute_delegate_user(self, host, inject):
394395

395396
return actual_user
396397

398+
def _count_module_args(self, args):
399+
'''
400+
Count the number of k=v pairs in the supplied module args. This is
401+
basically a specialized version of parse_kv() from utils with a few
402+
minor changes.
403+
'''
404+
options = {}
405+
if args is not None:
406+
args = args.encode('utf-8')
407+
try:
408+
lexer = shlex.shlex(args, posix=True)
409+
lexer.whitespace_split = True
410+
lexer.quotes = '"'
411+
lexer.ignore_quotes = "'"
412+
vargs = list(lexer)
413+
except ValueError, ve:
414+
if 'no closing quotation' in str(ve).lower():
415+
raise errors.AnsibleError("error parsing argument string '%s', try quoting the entire line." % args)
416+
else:
417+
raise
418+
vargs = [x.decode('utf-8') for x in vargs]
419+
for x in vargs:
420+
if "=" in x:
421+
k, v = x.split("=",1)
422+
if k in options:
423+
raise errors.AnsibleError("a duplicate parameter was found in the argument string (%s)" % k)
424+
options[k] = v
425+
return len(options)
426+
397427

398428
# *****************************************************
399429

@@ -612,6 +642,9 @@ def _executor_internal(self, host, new_stdin):
612642
items_terms = self.module_vars.get('items_lookup_terms', '')
613643
items_terms = template.template(basedir, items_terms, inject)
614644
items = utils.plugins.lookup_loader.get(items_plugin, runner=self, basedir=basedir).run(items_terms, inject=inject)
645+
# strip out any jinja2 template syntax within
646+
# the data returned by the lookup plugin
647+
items = utils._clean_data_struct(items, from_remote=True)
615648
if type(items) != list:
616649
raise errors.AnsibleError("lookup plugins have to return a list: %r" % items)
617650

@@ -823,7 +856,20 @@ def _executor_internal_inner(self, host, module_name, module_args, inject, port,
823856

824857
# render module_args and complex_args templates
825858
try:
859+
# When templating module_args, we need to be careful to ensure
860+
# that no variables inadvertantly (or maliciously) add params
861+
# to the list of args. We do this by counting the number of k=v
862+
# pairs before and after templating.
863+
num_args_pre = self._count_module_args(module_args)
826864
module_args = template.template(self.basedir, module_args, inject, fail_on_undefined=self.error_on_undefined_vars)
865+
num_args_post = self._count_module_args(module_args)
866+
if num_args_pre != num_args_post:
867+
raise errors.AnsibleError("A variable inserted a new parameter into the module args. " + \
868+
"Be sure to quote variables if they contain equal signs (for example: \"{{var}}\").")
869+
# And we also make sure nothing added in special flags for things
870+
# like the command/shell module (ie. #USE_SHELL)
871+
if '#USE_SHELL' in module_args:
872+
raise errors.AnsibleError("A variable tried to add #USE_SHELL to the module arguments.")
827873
complex_args = template.template(self.basedir, complex_args, inject, fail_on_undefined=self.error_on_undefined_vars)
828874
except jinja2.exceptions.UndefinedError, e:
829875
raise errors.AnsibleUndefinedVariable("One or more undefined variables: %s" % str(e))

Diff for: lib/ansible/runner/action_plugins/assemble.py

+15-4
Original file line numberDiff line numberDiff line change
@@ -122,14 +122,25 @@ def run(self, conn, tmp, module_name, module_args, inject, complex_args=None, **
122122
self.runner._low_level_exec_command(conn, "chmod a+r %s" % xfered, tmp)
123123

124124
# run the copy module
125-
module_args = "%s src=%s dest=%s original_basename=%s" % (module_args, pipes.quote(xfered), pipes.quote(dest), pipes.quote(os.path.basename(src)))
125+
new_module_args = dict(
126+
src=xfered,
127+
dest=dest,
128+
original_basename=os.path.basename(src),
129+
)
130+
module_args_tmp = utils.merge_module_args(module_args, new_module_args)
126131

127132
if self.runner.noop_on_check(inject):
128133
return ReturnData(conn=conn, comm_ok=True, result=dict(changed=True), diff=dict(before_header=dest, after_header=src, after=resultant))
129134
else:
130-
res = self.runner._execute_module(conn, tmp, 'copy', module_args, inject=inject)
135+
res = self.runner._execute_module(conn, tmp, 'copy', module_args_tmp, inject=inject)
131136
res.diff = dict(after=resultant)
132137
return res
133138
else:
134-
module_args = "%s src=%s dest=%s original_basename=%s" % (module_args, pipes.quote(xfered), pipes.quote(dest), pipes.quote(os.path.basename(src)))
135-
return self.runner._execute_module(conn, tmp, 'file', module_args, inject=inject)
139+
new_module_args = dict(
140+
src=xfered,
141+
dest=dest,
142+
original_basename=os.path.basename(src),
143+
)
144+
module_args_tmp = utils.merge_module_args(module_args, new_module_args)
145+
146+
return self.runner._execute_module(conn, tmp, 'file', module_args_tmp, inject=inject)

Diff for: lib/ansible/runner/action_plugins/copy.py

+16-7
Original file line numberDiff line numberDiff line change
@@ -238,11 +238,16 @@ def run(self, conn, tmp_path, module_name, module_args, inject, complex_args=Non
238238

239239
# src and dest here come after original and override them
240240
# we pass dest only to make sure it includes trailing slash in case of recursive copy
241-
module_args_tmp = "%s src=%s dest=%s original_basename=%s" % (module_args,
242-
pipes.quote(tmp_src), pipes.quote(dest), pipes.quote(source_rel))
241+
new_module_args = dict(
242+
src=tmp_src,
243+
dest=dest,
244+
original_basename=source_rel
245+
)
243246

244247
if self.runner.no_log:
245-
module_args_tmp = "%s NO_LOG=True" % module_args_tmp
248+
new_module_args['NO_LOG'] = True
249+
250+
module_args_tmp = utils.merge_module_args(module_args, new_module_args)
246251

247252
module_return = self.runner._execute_module(conn, tmp_path, 'copy', module_args_tmp, inject=inject, complex_args=complex_args, delete_remote_tmp=delete_remote_tmp)
248253
module_executed = True
@@ -260,12 +265,16 @@ def run(self, conn, tmp_path, module_name, module_args, inject, complex_args=Non
260265
tmp_src = tmp_path + source_rel
261266

262267
# Build temporary module_args.
263-
module_args_tmp = "%s src=%s original_basename=%s" % (module_args,
264-
pipes.quote(tmp_src), pipes.quote(source_rel))
268+
new_module_args = dict(
269+
src=tmp_src,
270+
dest=dest,
271+
)
265272
if self.runner.noop_on_check(inject):
266-
module_args_tmp = "%s CHECKMODE=True" % module_args_tmp
273+
new_module_args['CHECKMODE'] = True
267274
if self.runner.no_log:
268-
module_args_tmp = "%s NO_LOG=True" % module_args_tmp
275+
new_module_args['NO_LOG'] = True
276+
277+
module_args_tmp = utils.merge_module_args(module_args, new_module_args)
269278

270279
# Execute the file module.
271280
module_return = self.runner._execute_module(conn, tmp_path, 'file', module_args_tmp, inject=inject, complex_args=complex_args, delete_remote_tmp=delete_remote_tmp)

Diff for: lib/ansible/runner/action_plugins/template.py

+7-2
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,17 @@ def run(self, conn, tmp, module_name, module_args, inject, complex_args=None, **
117117
self.runner._low_level_exec_command(conn, "chmod a+r %s" % xfered, tmp)
118118

119119
# run the copy module
120-
module_args = "%s src=%s dest=%s original_basename=%s" % (module_args, pipes.quote(xfered), pipes.quote(dest), pipes.quote(os.path.basename(source)))
120+
new_module_args = dict(
121+
src=xfered,
122+
dest=dest,
123+
original_basename=os.path.basename(source),
124+
)
125+
module_args_tmp = utils.merge_module_args(module_args, new_module_args)
121126

122127
if self.runner.noop_on_check(inject):
123128
return ReturnData(conn=conn, comm_ok=True, result=dict(changed=True), diff=dict(before_header=dest, after_header=source, before=dest_contents, after=resultant))
124129
else:
125-
res = self.runner._execute_module(conn, tmp, 'copy', module_args, inject=inject, complex_args=complex_args)
130+
res = self.runner._execute_module(conn, tmp, 'copy', module_args_tmp, inject=inject, complex_args=complex_args)
126131
if res.result.get('changed', False):
127132
res.diff = dict(before=dest_contents, after=resultant)
128133
return res

Diff for: lib/ansible/utils/__init__.py

+48-14
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@
5151

5252
MAX_FILE_SIZE_FOR_DIFF=1*1024*1024
5353

54+
# caching the compilation of the regex used
55+
# to check for lookup calls within data
56+
LOOKUP_REGEX=re.compile(r'lookup\s*\(')
57+
5458
try:
5559
import json
5660
except ImportError:
@@ -313,38 +317,44 @@ def json_loads(data):
313317

314318
return json.loads(data)
315319

316-
def _clean_data(orig_data):
320+
def _clean_data(orig_data, from_remote=False, from_inventory=False):
317321
''' remove template tags from a string '''
318322
data = orig_data
319323
if isinstance(orig_data, basestring):
320-
for pattern,replacement in (('{{','{#'), ('}}','#}'), ('{%','{#'), ('%}','#}')):
324+
sub_list = [('{%','{#'), ('%}','#}')]
325+
if from_remote or (from_inventory and '{{' in data and LOOKUP_REGEX.search(data)):
326+
# if from a remote, we completely disable any jinja2 blocks
327+
sub_list.extend([('{{','{#'), ('}}','#}')])
328+
for pattern,replacement in sub_list:
321329
data = data.replace(pattern, replacement)
322330
return data
323331

324-
def _clean_data_struct(orig_data):
332+
def _clean_data_struct(orig_data, from_remote=False, from_inventory=False):
325333
'''
326334
walk a complex data structure, and use _clean_data() to
327335
remove any template tags that may exist
328336
'''
337+
if not from_remote and not from_inventory:
338+
raise errors.AnsibleErrors("when cleaning data, you must specify either from_remote or from_inventory")
329339
if isinstance(orig_data, dict):
330340
data = orig_data.copy()
331341
for key in data:
332-
new_key = _clean_data_struct(key)
333-
new_val = _clean_data_struct(data[key])
342+
new_key = _clean_data_struct(key, from_remote, from_inventory)
343+
new_val = _clean_data_struct(data[key], from_remote, from_inventory)
334344
if key != new_key:
335345
del data[key]
336346
data[new_key] = new_val
337347
elif isinstance(orig_data, list):
338348
data = orig_data[:]
339349
for i in range(0, len(data)):
340-
data[i] = _clean_data_struct(data[i])
350+
data[i] = _clean_data_struct(data[i], from_remote, from_inventory)
341351
elif isinstance(orig_data, basestring):
342-
data = _clean_data(orig_data)
352+
data = _clean_data(orig_data, from_remote, from_inventory)
343353
else:
344354
data = orig_data
345355
return data
346356

347-
def parse_json(raw_data, from_remote=False):
357+
def parse_json(raw_data, from_remote=False, from_inventory=False):
348358
''' this version for module return data only '''
349359

350360
orig_data = raw_data
@@ -379,10 +389,31 @@ def parse_json(raw_data, from_remote=False):
379389
return { "failed" : True, "parsed" : False, "msg" : orig_data }
380390

381391
if from_remote:
382-
results = _clean_data_struct(results)
392+
results = _clean_data_struct(results, from_remote, from_inventory)
383393

384394
return results
385395

396+
def merge_module_args(current_args, new_args):
397+
'''
398+
merges either a dictionary or string of k=v pairs with another string of k=v pairs,
399+
and returns a new k=v string without duplicates.
400+
'''
401+
if not isinstance(current_args, basestring):
402+
raise errors.AnsibleError("expected current_args to be a basestring")
403+
# we use parse_kv to split up the current args into a dictionary
404+
final_args = parse_kv(current_args)
405+
if isinstance(new_args, dict):
406+
final_args.update(new_args)
407+
elif isinstance(new_args, basestring):
408+
new_args_kv = parse_kv(new_args)
409+
final_args.update(new_args_kv)
410+
# then we re-assemble into a string
411+
module_args = ""
412+
for (k,v) in final_args.iteritems():
413+
if isinstance(v, basestring):
414+
module_args = "%s=%s %s" % (k, pipes.quote(v), module_args)
415+
return module_args.strip()
416+
386417
def smush_braces(data):
387418
''' smush Jinaj2 braces so unresolved templates like {{ foo }} don't get parsed weird by key=value code '''
388419
while '{{ ' in data:
@@ -615,7 +646,7 @@ def parse_kv(args):
615646
for x in vargs:
616647
if "=" in x:
617648
k, v = x.split("=",1)
618-
options[k]=v
649+
options[k] = v
619650
return options
620651

621652
def merge_hash(a, b):
@@ -1062,11 +1093,14 @@ def list_intersection(a, b):
10621093

10631094
def safe_eval(expr, locals={}, include_exceptions=False):
10641095
'''
1065-
this is intended for allowing things like:
1096+
This is intended for allowing things like:
10661097
with_items: a_list_variable
1067-
where Jinja2 would return a string
1068-
but we do not want to allow it to call functions (outside of Jinja2, where
1069-
the env is constrained)
1098+
1099+
Where Jinja2 would return a string but we do not want to allow it to
1100+
call functions (outside of Jinja2, where the env is constrained). If
1101+
the input data to this function came from an untrusted (remote) source,
1102+
it should first be run through _clean_data_struct() to ensure the data
1103+
is further sanitized prior to evaluation.
10701104
10711105
Based on:
10721106
http://stackoverflow.com/questions/12523516/using-ast-and-whitelists-to-make-pythons-eval-safe

Diff for: library/commands/command

+34-27
Original file line numberDiff line numberDiff line change
@@ -176,38 +176,45 @@ class CommandModule(AnsibleModule):
176176
''' read the input and return a dictionary and the arguments string '''
177177
args = MODULE_ARGS
178178
params = {}
179-
params['chdir'] = None
180-
params['creates'] = None
181-
params['removes'] = None
182-
params['shell'] = False
179+
params['chdir'] = None
180+
params['creates'] = None
181+
params['removes'] = None
182+
params['shell'] = False
183183
params['executable'] = None
184184
if "#USE_SHELL" in args:
185185
args = args.replace("#USE_SHELL", "")
186186
params['shell'] = True
187187

188-
r = re.compile(r'(^|\s)(creates|removes|chdir|executable|NO_LOG)=(?P<quote>[\'"])?(.*?)(?(quote)(?<!\\)(?P=quote))((?<!\\)(?=\s)|$)')
189-
for m in r.finditer(args):
190-
v = m.group(4).replace("\\", "")
191-
if m.group(2) == "creates":
192-
params['creates'] = v
193-
elif m.group(2) == "removes":
194-
params['removes'] = v
195-
elif m.group(2) == "chdir":
196-
v = os.path.expanduser(v)
197-
v = os.path.abspath(v)
198-
if not (os.path.exists(v) and os.path.isdir(v)):
199-
self.fail_json(rc=258, msg="cannot change to directory '%s': path does not exist" % v)
200-
params['chdir'] = v
201-
elif m.group(2) == "executable":
202-
v = os.path.expanduser(v)
203-
v = os.path.abspath(v)
204-
if not (os.path.exists(v)):
205-
self.fail_json(rc=258, msg="cannot use executable '%s': file does not exist" % v)
206-
params['executable'] = v
207-
elif m.group(2) == "NO_LOG":
208-
params['NO_LOG'] = self.boolean(v)
209-
args = r.sub("", args)
210-
params['args'] = args
188+
# use shlex to split up the args, while being careful to preserve
189+
# single quotes so they're not removed accidentally
190+
lexer = shlex.shlex(args, posix=True)
191+
lexer.whitespace_split = True
192+
lexer.quotes = '"'
193+
lexer.ignore_quotes = "'"
194+
items = list(lexer)
195+
196+
command_args = ''
197+
for x in items:
198+
if '=' in x:
199+
# check to see if this is a special parameter for the command
200+
k, v = x.split('=', 1)
201+
if k in ('creates', 'removes', 'chdir', 'executable', 'NO_LOG'):
202+
if k == "chdir":
203+
v = os.path.abspath(os.path.expanduser(v))
204+
if not (os.path.exists(v) and os.path.isdir(v)):
205+
self.fail_json(rc=258, msg="cannot change to directory '%s': path does not exist" % v)
206+
elif k == "executable":
207+
v = os.path.abspath(os.path.expanduser(v))
208+
if not (os.path.exists(v)):
209+
self.fail_json(rc=258, msg="cannot use executable '%s': file does not exist" % v)
210+
params[k] = v
211+
else:
212+
# this isn't a valid parameter, so just append it back to the list of arguments
213+
command_args = "%s %s" % (command_args, x)
214+
else:
215+
# not a param, so just append it to the list of arguments
216+
command_args = "%s %s" % (command_args, x)
217+
params['args'] = command_args.strip()
211218
return (params, params['args'])
212219

213220
main()

0 commit comments

Comments
 (0)