The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #4288

Error if exec command starts with shell built-in like "if"

Added by Alan Harder over 4 years ago. Updated over 1 year ago.

Status:ClosedStart date:07/19/2010
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:exec
Target version:-
Affected Puppet version:2.6.1 Branch:
Keywords: customer

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

exec { 'foo':
  command => 'if [ "abc" != "def" ]; then echo "this is a test"; fi',
  logoutput => true
}

With the above test case on 2.6.0rc4 (on Solaris 10 with ruby 1.8.7) I get:

err: /Stage[main]//Node[...]/Exec[foo]/returns: change from notrun to 0 failed: Could not find executable 'no if in /usr/bin /usr/sbin'

A command like this worked in 0.25.5.. is the behavior change intentional, or is this a bug?

Side node: if I add whitespace at the front (command => ‘ if [ …..’) then the error says Could not find command ‘’


Related issues

Related to Puppet - Bug #4299: Exec fails if command is in given path but "which" is not Closed 07/20/2010
Related to Puppet - Feature #4884: An exec provider that executes unfiltered bash code. Closed 09/30/2010
Related to Puppet - Bug #17603: Puppet execute has unexpected semantics Accepted

History

#1 Updated by James Turnbull over 4 years ago

  • Status changed from Unreviewed to Accepted
  • Target version set to 2.6.0

I am pretty sure this is also #4233 raising its ugly head.

#2 Updated by James Turnbull over 4 years ago

After some more debugging I don’t think it’s linked to #4233 – however with that patched I run the following:

exec { 'foo':
  command => 'if [ "abc" != "def" ]; then echo "this is a test"; fi',
  logoutput => true
}

and get:


puppet --trace --debug --verbose /tmp/4288.pp
/usr/lib/ruby/site_ruby/1.8/puppet/util/errors.rb:60:in `fail'
/usr/lib/ruby/site_ruby/1.8/puppet/type/exec.rb:673:in `validatecmd'
/usr/lib/ruby/site_ruby/1.8/puppet/type/exec.rb:481:in `validate'
/usr/lib/ruby/site_ruby/1.8/puppet/type.rb:1726:in `initialize'
/usr/lib/ruby/site_ruby/1.8/puppet/resource.rb:248:in `new'
/usr/lib/ruby/site_ruby/1.8/puppet/resource.rb:248:in `to_ral'
/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:553:in `send'
/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:553:in `to_catalog'
/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:531:in `each'
/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:531:in `to_catalog'
/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:468:in `to_ral'
/usr/lib/ruby/site_ruby/1.8/puppet/application/apply.rb:115:in `main'
/usr/lib/ruby/site_ruby/1.8/puppet/application/apply.rb:35:in `run_command'
/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:301:in `run'
/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:398:in `exit_on_fail'
/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:301:in `run'
/usr/lib/ruby/site_ruby/1.8/puppet/util/command_line.rb:55:in `execute'
/usr/bin/puppet:4
'if [ "abc" != "def" ]; then echo "this is a test"; fi' is both unqualifed and specified no search path at /tmp/4288.pp:5

#3 Updated by Alan Harder over 4 years ago

sorry, forgot about the default path from my site.pp. add path => ‘/usr/bin:/usr/sbin’ into the test case.

#4 Updated by Markus Roberts over 4 years ago

  • Status changed from Accepted to Needs Decision

I’m thinking this isn’t a bug; “if” isn’t a command.

You should probably be making this invoke bash explicitly if you want consistent behaviour.

#5 Updated by James Turnbull over 4 years ago

  • Target version changed from 2.6.0 to 2.6.1

#6 Updated by Alan Harder over 4 years ago

The 1-line patch from #4233 didn’t change the behavior here.

We have written execs basically assuming the “command” text is given to the shell, so we have some that start with “if”, others that redirect input like: “(echo yes; echo ‘’) | some_command” (starting with “(echo” also gets this error). Another one starts with variable assignment, command => “i=expr ...

The workaround I found is to just put “true; ” at the front of these commands.. this matches /usr/bin/true so there is no error reported, and everything after this is given to the shell as before.

James told me: “we’re explictly not allowing yu to call a bash subcmd due to another issue – can you add that to the ticket and we’ll do a design discussion internally about how to address this?”

It was a lot more convenient to put short shell scripts right in the command.. even with this “check” you can put arbitrary script after the first command. I guess you need to decide whether to open this back up (any script) or really lock it down (keep this check and limit the command to a single command..) Thanks!

#7 Updated by Alan Harder over 4 years ago

One idea: lock down “command =>” and make it secure. Add optional “script =>” that may be used instead.. perhaps less secure, but more convenient. This would just pass the content to the shell, as done in 0.25.x

#8 Updated by Alan Harder over 4 years ago

Another case where I hit trouble:

command => './something',
cwd => '/some/path'

To pass this check I guess I need to remove the ./ and put the dir in the path instead. I’m commenting on this here in case it’s another factor in the discussion of this security check for exec commands.. let me know if I should file a separate issue for this particular case (“./”). Thanks.

#9 Updated by Alan Harder over 4 years ago

  • Affected Puppet version changed from 2.6.0rc4 to 2.6.0

#10 Updated by John Bollinger over 4 years ago

We have written execs basically assuming the “command” text is given to the shell, so we have some that start with “if”, others that redirect input like: “(echo yes; echo ‘’) | some_command” (starting with “(echo” also gets this error). Another one starts with variable assignment, command => “i=expr ...

To my knowledge, it has never been documented that the command is handed to the shell for execution. In fact, that caught me by surprise when I found out, though probably it shouldn’t have: the parameter structure of Exec is wrong for supporting a bona fide exec() call. This design is an unfortunate quirk of Puppet. It is surely slower to pass a command to the shell than to directly fork+exec (because the shell itself will fork+exec anyway if the command contains any non-builtins), and the shell can anyway be explicitly run in those cases where its features are desired. Invoking the shell may also expose Puppet to the shell’s startup processing, including possibly making Puppet execute arbitrary commands in the event that one of the shell’s startup scripts is compromised. In short, I’d like to see Puppet not do that.

On the other hand, I recognize that there are many manifests in use that rely on Puppet’s undocumented behavior in this area. I might even have a few myself. It would be nice to have a way forward. Here’s one alternative:

Rename the Exec resource to Shellexec, and create a new Exec resource that executes the given command directly, instead of through the shell. The new Exec resource would take pre-tokenized arguments as an (array) parameter. For better backwards compatibility, it might also offer the option of a (simple-minded) tokenization of the “command” into command + argument list. That’s just the gist, of course; more design work would be needed before that approach could be implemented.

At absolute minimum, the behavior of Exec should be better documented. If it is intended that users not rely on the command being passed to the shell (even though currently it is), then that, too, should be documented. On the other hand, if there is no intention to change that behavior in the forseeable future, then it is silly to insist that users not rely on it, and in that case it should not be laden with surprises such as some subset of valid shell commands not working.

#11 Updated by Anonymous over 4 years ago

On the mailing list, I responded to some discussion of this topic with the following. As requested, I now duplicate it here for y'all to be able to internally discuss:

If this is a “voting” matter, let me put in a vote for passing a simple string to the shell, and passing an array direct to exec, which is consistent with the use of ‘system’ style commands in a whole bunch of sysadmin scripting languages.

Eg, this:

exec { “foo”: command => [‘/bin/ls’, ‘|’, ‘foo’] }

will pass ‘|’ ‘foo’ to the ls command, compared to:

exec { “foo”: command => “/bin/ls | foo” }

…which passes it to the default system shell.

For what little it is worth, this change would also have bitten us had I not seen the bug discussion, since I assumed that puppet had deliberately follow the typical behaviour of the ‘system’ call in the exec type.

#12 Updated by Cody Herriges over 4 years ago

As per the note in puppet-acceptance I wrote an acceptance test.

http://github.com/ody/puppet-acceptance/blob/ticket/4288/spec/ticket_4288_error_if_exec_command_starts_with_shell_built_in_spec.sh

It fails just as the submitter experienced.

#13 Updated by James Turnbull about 4 years ago

  • Target version changed from 2.6.1 to 2.6.2

#14 Updated by Jesse Wolfe about 4 years ago

  • Target version deleted (2.6.2)

#15 Updated by Paul Berry about 4 years ago

  • Status changed from Needs Decision to Accepted
  • Assignee set to Paul Berry

I’m investigating this. At the moment it looks like it was an intentional behavior change—we modified the way exec works so that it double-checks that the command being executed is actually available on the system before trying to execute it. This doesn’t work if the command uses bash constructs (such as “if”).

I’ll do some digging to verify that this was an intentional change, and to see if there’s a better way to do it. In the meantime, here is a workaround that should get you going again: explicitly execute the command using the shell, like this:

exec { 'foo':
  command => 'bash -c "if [ \"abc\" != \"def\" ]; then echo \"this is a test\"; fi"',
  logoutput => true
}

instead of this:

exec { 'foo':
  command => 'if [ "abc" != "def" ]; then echo "this is a test"; fi',
  logoutput => true
}

#16 Updated by Markus Roberts about 4 years ago

It isn’t just that we want to see that the executable exists; we want to avoid the possibility of shell-injection attacks. The array form, which executes the requested executable with the specified arguments is both faster and safer than the “make it all into a string and hope the shell takes it apart the way you intended form.

This was a conscious design decision on Luke’s part, and I happen to agree with it. We shouldn’t fall into the trap of trading security for convenience, especially when users can (as noted above) still explicitly evoke the shell if they want.

#17 Updated by Alan Barrett about 4 years ago

On the topic of how to test whether a command exists: Use “command -v” instead of “which” or “type” or whatever the code is using now. “command -v” does the right thing for shell builtin command like “if” or “while” or even “!”, and for normal commands.

On the topic of how an exec should behave: I like the idea of letting arrays and strings have different meanings. The Ruby Kernel::exec method already handles that in a sensible way, so it would just need some plumbing to let arrays get through from the user’s manifest to the underlying ruby method.

#18 Updated by Alan Barrett about 4 years ago

John Bollinger wrote:

To my knowledge, it has never been documented that the command is handed to the shell for execution.

Maybe it wasn’t documented, but I would have been astonished if it had not been handed to the shell. Almost every unix program that has an “execute this string as a command” feature either explains in detail how it parses the command, or means “ask the shell to execute this string as a command”; in the absence of documentation to the contrary, a user naturally assumes that arbitrary shell commands are acceptable.

#19 Updated by Paul Berry about 4 years ago

  • Status changed from Accepted to Rejected
  • Assignee deleted (Paul Berry)

I’ve been investigating this bug, and I see the same behavior in 2.6.x, 0.25.x, and 0.24.x.

In 2.6.x:

'if [ "abc" != "def" ]; then echo "this is a test"; fi' is both unqualifed and specified no search path at /Users/pberry/puppet_labs/notes/tests/bug4288/init.pp:4

In 0.25.x:

'if [ "abc" != "def" ]; then echo "this is a test"; fi' is both unqualifed and specified no search path at /Users/pberry/puppet_labs/notes/tests/bug4288/init.pp:4

In 0.24.x:

err: Could not create foo: 'if [ "abc" != "def" ]; then echo "this is a test"; fi' is both unqualifed and specified no search path at /Users/pberry/puppet_labs/notes/tests/bug4288/init.pp:4
'if [ "abc" != "def" ]; then echo "this is a test"; fi' is both unqualifed and specified no search path at /Users/pberry/puppet_labs/notes/tests/bug4288/init.pp:4

This is with the manifest:

exec { 'foo':
  command => 'if [ "abc" != "def" ]; then echo "this is a test"; fi',
  logoutput => true
}

Given that (a) the behavior is consistent across three versions of Puppet, (b) there is concern that changing the behavior would create security vulnerabilities, and © there’s an easy workaround, I’m of the opinion that we should leave the behavior as is.

#20 Updated by Alan Harder about 4 years ago

  • Status changed from Rejected to Re-opened
  • Affected Puppet version changed from 2.6.0 to 2.6.1

Hi Paul- You may choose to re-reject this, but I’m reopening because the test you ran is not what this issue is about.

It is true that all those versions have the same check about requiring a search path if the command is not a full path. Retry your test adding path => ‘/usr/bin’ in your exec resource. Now it’ll get past that “unqualified” check and get to the new “which” check that was added in 2.6.x. Thanks!

#21 Updated by Jesse Wolfe about 4 years ago

  • Status changed from Re-opened to Rejected

Ah! This explains why Luke was surprised to hear that it was possible to execute arbitrary strings via exec. “path” is intended to be the search path for the specified executable. Shell built-ins are not meaningfully in a directory – the fact that giving them a “path” bypassed the command validation was unintentional.

I’ve opened #4884 as a place to discuss adding the ability to execute arbitrary bash script as a supported feature.

#22 Updated by John Sellens almost 3 years ago

Another workaround, which I’ll mention just in case anyone else bumps into this problem and this issue ticket, is to start your command line with “true &&” e.g.

command => “true && cd /usr/local && make trouble”

/bin/true exists, so the command should get passed to the shell. This is sometimes more comprehensible than putting the whole command into a bash -c construct.

Hope that’s helpful for someone!

John

#23 Updated by Anonymous almost 3 years ago

  • Status changed from Rejected to Closed

John Sellens wrote:

Another workaround, which I’ll mention just in case anyone else bumps into this problem and this issue ticket, is to start your command line with “true &&” e.g.

Oh! This is totally no longer true or accurate! We added the shell provider to Puppet 2.7 to address exactly this use case; you can now just:

exec { "whatever":
  provider => shell,
  command => "if this; then that; else those; fi"
}

/bin/true exists, so the command should get passed to the shell. This is sometimes more comprehensible than putting the whole command into a bash -c construct.

That won’t work any more; our default provider will directly exec /bin/true and pass everything else – including the shell control characters – to it as arguments. However, picking the right provider totally works. :)

#24 Updated by Charlie Sharpsteen over 1 year ago

  • Keywords set to customer

Also available in: Atom PDF