Quote handling for CMD_CYCLE_OPEN

[xenithorb]

Version: 2.6.9

I am trying to do the following for a very specific edge case:

CMD_CYCLE_OPEN: /bin/firewall-cmd --zone=public --timeout=30 --add-rich-rule='rule family=ipv4 source address=$PKT_SRC accept'
CMD_CYCLE_CLOSE: NONE

but no matter what I try, all quotes are scrubbed and the command errors out because the parts after --add-rich-rule= need to be passed as a single string (I know that's dumb)

strace shows the following:

[X.X.X.X] (stanza #1) Running CMD_CYCLE_OPEN command: /bin/firewall-cmd --zone=public --timeout=30 --add-rich-rule='rule family=ipv4 source address=X.X.X.X accept'
read(5, "usage: see firewall-cmd man page\nfirewall-cmd: error: unrecognized arguments: family=ipv4 source address=X.X.X.X accept'\n", 4096) = 128

This means that what is getting actually executed is:

/bin/firewall-cmd --zone=public --timeout=30 --add-rich-rule=rule family=ipv4 source address=X.X.X.X accept

(missing single quotes)

Disclaimer: I understand the ramifications and security implications of what I'm doing here, but a secondary hypervisor firewall is protecting the host, and this is my choice for not injecting many, many rules.

I would really like to see the quoting work as intended, and possibly with double-quotes inside, too. Originally I had all of the arguments in the '' quoted as well, but compromised to try and solve the problem.

Additionally, I've tried every quote escaping combination I know, "'", ''', ", """, et al. And could not get fwknopd to not dump quotes on execution. I suspect that while it's parsing for your own internal variables, it's word-splitting on spaces which is not good in this particular instance (bad for data integrity)

[jp-bennett]

We'll dig into this. I agree that some sort of quote escaping should be possible for these use cases. Ideally we want the command cycle functionality to be as flexible as possible, so this ability makes sense.

[xenithorb]

Thanks! I believe the points of interest for this are:

strtoargv(): https://github.com/mrash/fwknop/blob/master/common/fko_util.c#L772 and
if(strtoargv : https://github.com/mrash/fwknop/blob/17dd9de06f13b3ba8936dd785cbb8c1f3d0996c7/server/extcmd.c#L150
execvpe(): https://github.com/mrash/fwknop/blob/17dd9de06f13b3ba8936dd785cbb8c1f3d0996c7/server/extcmd.c#L206

Tried looking for some obvious errors, but I don't do C. Hopefully that highlights something for anyone that wants to take a quick look.

[mrash]

One way to get things working is to compile fwknop with the --disable-execvpe argument to the configure script. This will force fwknopd to just execute commands via system(), and therefore it doesn't manually build argv from the CMD_CYCLE_OPEN or CMD_CYCLE_CLOSE strings. fwknop defaults to using execvpe() so that it doesn't use the shell environment, but this comes at a price of having to build up argv from the command strings - and this implies the need for some level of simple parsing. This is why the strtoargv() function was written, and it is perhaps too conservative in what it considers to be an argument. This thread is interesting, and would provide a better solution probably:

http://stackoverflow.com/questions/9659697/parse-string-into-array-based-on-spaces-or-double-quotes-strings

In the meantime, I think --disable-execvpe should do the trick and worked for me in a simple test.