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

Bug #6955

Risk of malicious code execution

Added by Jacek Masiulaniec over 3 years ago. Updated over 1 year ago.

Status:ClosedStart date:04/04/2011
Priority:UrgentDue date:
Assignee:Jeff Weiss% Done:

0%

Category:library
Target version:1.7.0
Keywords: Affected Facter version:
Branch:https://github.com/puppetlabs/facter/pull/203

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

Fact search path includes current working directory:

[jacekm@localhost ~]$ ls facter
ls: facter: No such file or directory
[jacekm@localhost ~]$ facter >/dev/null
[jacekm@localhost ~]$ mkdir facter
[jacekm@localhost ~]$ echo 'STDERR.puts "evil code"' > facter/evil.rb
[jacekm@localhost ~]$ facter >/dev/null
evil code
[jacekm@localhost ~]$ 

This is harmful in multi-user environments: starting facter in specially crafted directory can result in malicious code execution.


Related issues

Related to Facter - Bug #20381: API Break in Facter 1.7.0 Closed
Duplicated by Facter - Bug #13668: undefined method `watch' for #<Object:0x10fb852a8 @operat... Duplicate 04/06/2012

History

#1 Updated by Nigel Kersten over 3 years ago

  • Status changed from Unreviewed to Accepted
  • Priority changed from Normal to Urgent

#2 Updated by Paul Nasrat over 3 years ago

I’m not sure this is urgent, and a desperate fix may well break things in current environments. This has been true for all releases of facter afaik. Changing it quickly may also break development.

We should ideally move away from $: + ‘facter’ to something else, but if you’re trying to squeeze in a security fix may not want to. Maybe just if running as root do extra checks.

#3 Updated by Paul Nasrat over 3 years ago

Also generally for security vulnerabilities you should report via security@ and not via the bug tracker I’d assume. The Authorization vulnerability email lists that address but it’s not very discoverable from the main puppetlabs site for reporting security vulnerabilities ala http://www.apache.org/security/ or https://access.redhat.com/security/team/contact/

#4 Updated by Nigel Kersten over 3 years ago

Paul Nasrat wrote:

I’m not sure this is urgent, and a desperate fix may well break things in current environments. This has been true for all releases of facter afaik. Changing it quickly may also break development.

I disagree. It is urgent that we work out how we’re going to address this, even if we discover that we need to save the actual implementation for 1.6.0 or some other release where it’s more practical to break backwards compability.

#5 Updated by Nigel Kersten over 3 years ago

It’s also causing issues right now in automated acceptance testing FWIW.

#6 Updated by Nigel Kersten over 3 years ago

Can you characterize why Facter does this Paul?

#7 Updated by Paul Nasrat over 3 years ago

The implementation predates my involvement with the project

If it’s the automated testing issue see the feature request I raised to add -I and -x to include/exclude paths Issue #4551, that’s probably doable for 1.5.x. If anyone ever works on the 2.0 stuff we should break the fact loading path to clear out the crappy structure of lib/facter/util to make the code first class lib/facter citizen and facts from elsewhere.

#8 Updated by Josh Cooper about 3 years ago

The root cause of this problem is that ruby 1.8.x – 1.9.1 includes the current working directory in the $LOAD_PATH ($:). This behavior was changed in ruby 1.9.2:

$: no longer includes the current directory, use require_relative

Facter does not use require to load facts, instead it looks in the facter directory relative to every directory in the LOAD_PATH, and calls Kernel.load on every *.rb file. And since “.” is in the LOAD_PATH, we get this behavior. Assuming <somedir>/facter/lib is in the LOAD_PATH, then it should be safe to exclude the current working directory during fact loading.

Interestingly, puppet’s autoloader would have a similar problem when generating its list of module directories, except that it rejects directories in the LOAD_PATH that start with “.”. Though this change was made due to #2574, when calling Dir.entries returns . and ..

Thread.current[:env_module_directories][real_env] ||= real_env.modulepath.collect do |dir|
  Dir.entries(dir).reject { |f| f =~ /^\./ }.collect { |f| File.join(dir, f) }
end.flatten.collect { |d| [File.join(d, "plugins"), File.join(d, "lib")] }.flatten.find_all do |d|
  FileTest.directory?(d)
end

#9 Updated by James Turnbull about 3 years ago

  • Category set to library

#10 Updated by Ken Barber almost 3 years ago

  • Target version set to 144

#11 Updated by Jeff Weiss over 2 years ago

  • Assignee set to Anonymous
  • Target version changed from 144 to 2.0.0

#12 Updated by Jeff Weiss over 2 years ago

Looking at this and thinking that perhaps instead of excluding . and anything beginning with .. (my first thought), but rather excluding anything that isn’t explicitly an absolute directory. We’ll need to make certain that we don’t screw up windows when we do that.

#13 Updated by Jeff Weiss over 2 years ago

  • Status changed from Accepted to In Topic Branch Pending Review
  • Assignee changed from Anonymous to Jeff Weiss
  • Branch set to https://github.com/puppetlabs/facter/pull/203

#14 Updated by Anonymous over 2 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

This has been merged, and will be in Facter 2.0.0; we believe it resolves the problem entirely.

#15 Updated by Matthaus Owens over 2 years ago

  • Status changed from Merged - Pending Release to Closed

released in Facter 2.0.0rc1

#16 Updated by Josh Cooper over 1 year ago

  • Target version changed from 2.0.0 to 1.7.0

This was actually released in 1.7.0, but wasn’t apparent, likely due to the facter 2.x branch issue https://groups.google.com/forum/?fromgroups=#!topic/puppet-dev/eHmSzwHoFAk

Also available in: Atom PDF