Skip to content

Commit

Permalink
formats/command: escape also chunks with arbitrary "metacharacters"
Browse files Browse the repository at this point in the history
...unless that chunk consists merely of them.  This is apparently
prone to both false positives and negatives (in the context of use),
but at least fixes more serious issues with these magic characters
demonstrating their power when missing the quotation (or escaping,
which we are not resorting to so far, as it would be another complexity
for already convoluted handling).  For instance, value of an instance
attribute for a resource within CIB can read "fish&chips" (in XML:
fish&chips), which when proceeded with cib2pcscmd command,
would previously result in something like:

  pcs -f tmp-cib.xml \
    resource create res ocf:heartbeat:agent par=fish&chips

which, when actually executed in the shell environment, would only run
"pcs -f tmp-cib.xml resource create res ocf:heartbeat:agent par=fish"
that would be subsequently backgrounded only to likewise unintentionally
(and possibly even harmfully) continue execution with "chips" (and
likely failing).

Now, it will cause no harm thanks to quoting properly:

  pcs -f tmp-cib.xml \
    resource create res ocf:heartbeat:agent 'ipd=fish&chips'

* * *

For the paranoids, there's a universally applicable workaround boiling
down to disabling the cmd-wrap filter, which is actually responsible for
the bells-and-whistles prettified output that only aims to add value
for the humans, for instance:

  clufter cib2pcscmd --noop=cmd-wrap mycib.xml

But technically the raw output should be (unless a corner case like
above is hit) identical, carrying unwrapped lines with over-enquoted
chunks where the necessity to do so is a priori assumed (better to
stay safe) -- actually one example of value added with cmd-wrap
post-processing is getting rid of extraneous quoting :-)

Note that "cmd-wrap" is also an eponymous command on its own, which is
apparently also affected, but for which it doeasn't make sense to apply
that workaround (disabling the only filter enqued) as opposed to
avoiding that command altogether for anything critical...

* * *

The change, in turn, required input/output redirection used
conditionally when a whole-stack-config-to-pcs-commands clufter's
command was about to emit "pcs cluster auth" to use "<> /dev/tty"
rather than "<>/dev/tty" because the latter would end up being
undesirably enquoted with the newly introduced rule in place
(fortunately, pcs uses switches and "X=Y" syntax most of the time).
Also add a new test to have at least '&' case covered in the future.

Thanks to Madkiss (seconded by Lars Ellenberg) at freenode/#clusterlabs
for pointing this issue out.

Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
  • Loading branch information
jnpkrn committed Nov 10, 2017
1 parent 7c705a3 commit dabadf6
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 9 deletions.
2 changes: 1 addition & 1 deletion __root__/run-check
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ EOF
&& echo "TEST: cmd diff OK" \
|| { echo "TEST: cmd diff FAIL"; ret=21; }; }<<EOF
# targeting system: ('linux', 'redhat', '7.1', 'Maipo')
pcs cluster auth rhel6-node1 rhel6-node2 <>/dev/tty
pcs cluster auth rhel6-node1 rhel6-node2 <> /dev/tty
pcs cluster setup --name one rhel6-node1 rhel6-node2 --join 60 \\
--token 10000 --consensus 12000
pcs cluster start --all && sleep 60
Expand Down
2 changes: 1 addition & 1 deletion filters/_2pcscmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def coro2pcscmd(**kwargs):
<xsl:if test="$pcscmd_force">
<xsl:value-of select="' --force'"/>
</xsl:if>
<xsl:value-of select="' &lt;&gt;/dev/tty'"/>
<xsl:value-of select="' &lt;&gt; /dev/tty'"/>
<xsl:value-of select="'%(NL)s'"/>
''' + (
verbose_ec_test
Expand Down
10 changes: 8 additions & 2 deletions formats/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,16 @@ class command(SimpleFormat):
@staticmethod
def _escape(base, qs=("'", '"')):
# rule: last but one item in qs cannot be escaped inside enquotion
# XXX: escaping _META_CHARACTERS
# XXX: shell constructs like "&&" and "<>" need to be blank-separated
# and still there may be a confusion if they are used alone
# (fortunately, pcs uses switches and "X=Y" syntax most of
# the time); real solution would be to implement complete
# context-free(?) grammar of the POSIX shell, which is currently
# a blatant overkill
ret = []
for b in base:
if any(c in b for c in qs + tuple(' \t#$')):
if (b.strip(''.join(_META_CHARACTERS)) and
any(c in b for c in qs + tuple('#$') + _META_CHARACTERS)):
use_q, prologue, epilogue = ('', ) * 3
if not(b.endswith('(') and b[-2:-1] in '$<'
and b[:-2].rstrip(' \t') in ('', '"')
Expand Down
23 changes: 18 additions & 5 deletions tests/filters/cmd_wrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,30 @@ def testCmdWrap60(self):
""")

def testCmdWrapCib2PcsCmd(self):
result = cmd_wrap(string_iter('bytestring', bytes_enc("""\
io_strings = (
('''\
pcs -f tmp-cib.xml resource create RESOURCE-apache-webserver ocf:heartbeat:apache 'options= -Dwebserver' op stop 'id=RESOURCE-apache-webserver-OP-stop' 'name=stop' 'interval=0' 'timeout=122s'
""")), text_width=80)
#print(result.BYTESTRING())
self.assertEqual(str_enc(result.BYTESTRING()), """\
''', '''\
pcs -f tmp-cib.xml \\
resource create RESOURCE-apache-webserver ocf:heartbeat:apache \\
'options= -Dwebserver' \\
op stop id=RESOURCE-apache-webserver-OP-stop name=stop interval=0 \\
timeout=122s
""")
'''), ('''\
pcs -f tmp-cib.xml resource create RESOURCE-apache-webserver ocf:heartbeat:apache 'options= -Dwebserver' 'config_file=/etc/httpd/lean&mean.conf' op stop 'id=RESOURCE-apache-webserver-OP-stop' 'name=stop' 'interval=0' 'timeout=122s'
''', '''\
pcs -f tmp-cib.xml \\
resource create RESOURCE-apache-webserver ocf:heartbeat:apache \\
'options= -Dwebserver' 'config_file=/etc/httpd/lean&mean.conf' \\
op stop id=RESOURCE-apache-webserver-OP-stop name=stop interval=0 \\
timeout=122s
'''),
)
for (in_str, out_str) in io_strings:
out_obj = cmd_wrap(string_iter('bytestring', bytes_enc(in_str)),
text_width=80)
#print(out_obj.BYTESTRING())
self.assertEqual(str_enc(out_obj.BYTESTRING()), out_str)


from os.path import join, dirname; from sys import modules as m # 2/3 compat
Expand Down

0 comments on commit dabadf6

Please sign in to comment.