Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cf-agent --bootstrap fails on Debian systems. #1050

Merged
merged 1 commit into from
Oct 29, 2013

Conversation

basvandervlies
Copy link
Contributor

Because sys.workdir/inputs is a symlink to /etc/cfengine3. The bootstrap process creates failsafe.cf in /etc/cfengine3, but cf-agent complains that this sys.workdir/inputs is a symlink and aborts the bootstrap process. Both must fail/complain or both must accept the symbolic link to the directory as destination.

This patch enable the last option. It will determine if the destination is a symlink to a directory and copy_from attribute type_check : false is set. If both options are true the copy will be honored.

…uts is a symlink to /etc/cfengine3. The bootstrap creates 'failsafe.cf' in /etc/cfengine3, but cf-agent complains that this sys.workdir/inputs is a symlink. Both must fail/complain or we accept symbolic link to a

directory.

This patch enable the last option. It will determine if the destination is a symlink to a directory and ```copy_from``` attribute ```type_check : false``` is set.  If both options are true the copy will be honoured.
@tzz
Copy link
Contributor

tzz commented Oct 28, 2013

I think this is right, IMO there's no reason to refuse the inputs directory if it's a symlink.

kacf added a commit that referenced this pull request Oct 29, 2013
cf-agent --bootstrap fails on Debian systems.
@kacf kacf merged commit 20465de into cfengine:master Oct 29, 2013
@kacf
Copy link
Contributor

kacf commented Oct 29, 2013

Agreed. And merged! :-)

@basvandervlies
Copy link
Contributor Author

Thanks a lot!!

@basvandervlies
Copy link
Contributor Author

I want to comment on this. Just find out that test:

  • ./testall ./10_files/02_maintain/010.cf

We have to add: type_check => "true" to the copy_from sync_cp body.

@kacf
Copy link
Contributor

kacf commented Oct 29, 2013

Indeed. This test also fails: ./10_files/templating/mustache_abuse.cf.

@basvandervlies
Copy link
Contributor Author

@kacfengine The test you mentioned has nothing to do with the patch. The patch is only for the body copy_from or is the assumption wrong?

@kacf
Copy link
Contributor

kacf commented Oct 29, 2013

You're probably right. I haven't checked the details myself. I'll look into it.

@tzz
Copy link
Contributor

tzz commented Oct 29, 2013

The mustache test failure was from an unrelated commit, fixed in #1052 in any case.

kacf pushed a commit to kacf/core that referenced this pull request Oct 31, 2013
kacf added a commit to kacf/core that referenced this pull request Oct 31, 2013
@tzz
Copy link
Contributor

tzz commented Nov 12, 2013

@basvandervlies @vohi @kacfengine can this fix be backported to 3.5.x? (also asked in Webhuis/Cfengine-debian#41)

@vohi
Copy link
Contributor

vohi commented Nov 13, 2013

Hm, what would be a reason for cf-agent refusing an inputs directory that's a symlink, and what's the implication of permitting this? Would like to understand this before we accept this change and back port it to a drop-in replacement maintenance release.

@markburgess
Copy link

On 13/11/13 15:59, Volker Hilsheimer wrote:

Hm, what would be a reason for cf-agent refusing an inputs directory
that's a symlink, and what's the implication of permitting this? Would
like to understand this before we accept this change and back port it
to a drop-in replacement maintenance release.


Reply to this email directly or view it on GitHub
#1050 (comment).

Two reasons:

  1. Granting access to symlinks is considered a huge security hole in
    client-server systems as they are very hard to validate. Local users
    could easily subvert policy to point to their own policy.
  2. Putting policy anywhere than /var/cfengine could lead to files
    disappearing on network mounted filesystems. CFEngine needs a safe,
    private and reliable cache of its data. This cannot be guaranteed with
    symbolic links, as symbolic links can point to nothing at all.

I strongly advise to never allow symbolic links of directories, and also
not files without checking that the files exist and pointed to are 1.
owned by root, and 2. not writable by any non.root user, 3) in a
directory that is not writable by any non-root user.

M

Founder and CTO at CFEngine
Author of In Search of Certainty: The Science of our Information
Infrastructure http://markburgess.org/certainty.html

@neilhwatson
Copy link
Contributor

People do run CFEngine as none root in production. I've seen application teams do this. So being able to deploy and bootstrap as a non root user is a good feature. So rather than root, file should not be group or world writable.

I do agree that symlinks are risky. If they are allowed then there must be check on the destination, as described above. Similarly there could be a check that directories are not network file systems.

@basvandervlies
Copy link
Contributor Author

On 13 nov. 2013, at 15:59, Volker Hilsheimer notifications@github.com wrote:

Hm, what would be a reason for cf-agent refusing an inputs directory that's a symlink, and what's the implication of permitting this? Would like to understand this before we accept this change and back port it to a drop-in replacement maintenance release.

As for bootstrap. cf-agent creates a ‘failsafe.cf’ file in sys.workdir/inputs. In debian this a symlink to /etc/cfengine. Cfengine does not complain and creates the bootstrap ‘failsafe.cf’ file. Now cf-agent executes the failsafe,cf and suddenly it fails dus the fact sys.workdir/inputs is a symlink to a directory.

There are 2 scenarios:

  1. The bootstrap code must check if sys.workdir/inputs is a directory. If not the issue a warning/error or something else.
  2. symlink to a directory is allowed. This patch implements this feature and checks if the symlink points to a directory and honors the copy_from attributes (type_check:false).

SURFsara has a new telephone number: +31 20 800 1300.

Bas van der Vlies
| Operations, Support & Development | SURFsara | Science Park 140 | 1098 XG Amsterdam
| T +31 (0) 20 800 1300 | bas.vandervlies@surfsara.nl | www.surfsara.nl |

@tzz
Copy link
Contributor

tzz commented Nov 13, 2013

I personally see no problem with symlinks on the CFEngine server. We don't use them by default in our installation so they are a conscious policy decision by the sysadmin. If someone obtains write access to /var/cfengine, they could modify any file under /var/cfengine/masterfiles anyhow, so specifically protecting against symlinks is not going to help. If the concern is that the target of the symlink should disappear, that's probably a cf-serverd issue that's equally problematic if the contents of /var/cfengine/masterfiles should disappear. (See below for why this is edited out)

I think, however, that the fundamental issue here may be different. The problem is that the client doesn't need to know that /var/cfengine/masterfiles lives in that specific path. Today the client expects it. But the server could simply remap /var/cfengine/masterfiles to /etc/cfengine/masterfiles internally, which wouldn't matter to the client if it was presented the same way.

Another less intrusive approach is for the server to expose the masterdir location in a scalar variable. The client would then connect, read that variable, and use it to construct the source path.

@tzz
Copy link
Contributor

tzz commented Nov 13, 2013

After direct discussion with @markburgess and looking at the possible issues from symlinks in the masterfiles dir path, I see there are a few possible problems we haven't considered. There are race conditions that can be exploited, for instance. So please consider the first paragraph above retracted and (in case it wasn't clear) note that anything I state is my humble opinion and not "speaking ex cathedra for CFEngine."

The second and third paragraphs, regarding how the server can present an abstract location to the clients, are for a "logically consistent" solution implemented on the server side. Something like rsync modules, where you can say rsync server::module and the server maps module to /whatever/path/is/configured. But this creates fragility by breaking a simple file copy into a two-step operation. I see how that can be surprising and even break clients if either step fails. I'm still not convinced either way.

@basvandervlies what do you think about, on the server side, making a special Debian policy (executed by the default cf-agent policy on the hub) to synchronize /etc/cfengine/masterfiles to /var/cfengine/masterfiles periodically? That would avoid the need for a symlink and avoid the possibility of race conditions and security compromises due to symlinks.

@basvandervlies
Copy link
Contributor Author

@tzz just a quick response. It is all about client side for me. I never used the boostrap before:

  • cf-agent -B policy_server

This will fail on the client. cf-agent will create '''/etc/cfengine/failsafe.cf''' and does not care that sys.workdir is a symlink to '''/etc/cfengine'''. After this '''cf-agent -f failsafe.cf''' will run and this fails and complains about it. It is only for cf-agent client configuration files to my knowledge and as nothing to do with the cf-serverd.

we have '''move_obstruction''' option for '''body copy_from'''. This will remove the offending file if it is not a directory. Maybe the generated failsafe.cf should include this option then it does not fail. Then it is still strange that the bootstrap process can create a file in a symlinked directory.

I just tested this with my old cfengine server 3.3.2-1. I made the following change:

  • cd /var/lib/cfengine3
  • rm masterfiles; ln -s /data/cfengine/config masterfiles

I can just copy the cfengine configuration files. maybe something is patched between versions to prevent this. I have to test it with a new cf-serverd version.

''/etc/cfengine/masterfiles''' is not the right direction. You also do not place '''masterfiles''' in ''''sys.workdir/inputs'''.
Why not make this configurable via ''''---masterfiles-dir''' or something similar.

This was not a quick response ;-)

@kacf
Copy link
Contributor

kacf commented Nov 14, 2013

There are race conditions that can be exploited, for instance.

Can you elaborate on how? This is already in master, so if there are security implications we should be aware of them.

@markburgess
Copy link

Google symlink security issues.

Generally, when something is excluded explicitly in the code or done in a none-obvious way, its a sign there is a reason. Worth asking up front.

Symlinks are hard to validate and can enable local root exploits when trusted by root.

Kristian Amlie notifications@github.com wrote:

There are race conditions that can be exploited, for instance.

Can you elaborate on how? This is already in master, so if there are
security implications we should be aware of them.


Reply to this email directly or view it on GitHub:
#1050 (comment)

@basvandervlies
Copy link
Contributor Author

Ok just did a test with the 3.5.X branch and cf-serverd. My first question is how can i debug a child connection nothing seems to be logged when we encounter a problem in child thread: cf-serverd -dF (only initial connection).

Now for cf-serverd. symlinks are alllowed for masterfiles directory if there are on the same filesystem. In previous version 3.3.2 there was not that restriction. It is fine for mer

I like to emphasis that the this pull request is for '''cf-agent''' and not ''cf-serverd'''

@cdituri
Copy link
Contributor

cdituri commented Nov 14, 2013

@basvandervlies
Fire up gdb, set a break point in the child, and 'set follow-fork-mode child' before starting the execution run.

(I can't believe there's no way to enter a backtick from an iPhone o_O)

@kacf
Copy link
Contributor

kacf commented Nov 14, 2013

Googling symlink security issues does not turn up any useful results for me. It's all either old security holes that have been fixed, or misunderstandings about the security implications of symlinks.

If anyone is aware of any security issues, they need to post the details here, how the feature can be exploited. There is no need for discretion, since this version hasn't been released yet.

There are many ways to shoot yourself in the foot with CFEngine, I don't see how symlinks are any different. An administrator can of course make an insecure symlink to a user owned directory, but then he has noone to blame but himself.

@vohi
Copy link
Contributor

vohi commented Nov 14, 2013

Quoting Mark's respective paragraph from the email discussion:

If one allows symlinks in access control, one opens up the possibility of man-in-the-middle attacks by local users. It might not be practical in this specific case, but it will work in others -- so we should not open up a trap for users to fall into. Because links are just unvalidated strings, someone can introduce a symlink in the middle of a path using local disk access as a non-privilieged user and divert an entire path to a different location. e.g. when CFEngine is searching through directories where non-root users have write access, links can be inserted as a race condition that can be used to obtain root privilege to any file on the system. To prevent that, CFEngine uses kernel state to validate paths when recursing and it disallows path verification by name. It validates the security of the /var/cfengine files and aborts if there is tampering. Part of this came out of one of the two root exploits published against CFEngine over the past 20 years. I would never have thought of these things myself, without the help of penetration testers. It affects both server and client side code.

I recommend we revert that change in master until everybody has understood the reason for current behaviour.

@basvandervlies
Copy link
Contributor Author

@markburgess @vohi Please revert the change. It is a bad one. I completely agree. Just did a scenario:

  • user bas creates a symbolic link in /tmp: ln -fs /root/bas_root_dir /root/bas

''''
lrwxrwxrwx 1 bas bas 9 Nov 14 14:06 /tmp/bas_root_dir -> /root/bas
drwx------ 16 root root 4096 Nov 14 14:34 /root/bas/
'''

It wil just copy the data to '''/root/bas''' and that is a symlink attack. Thanks for noticing this!!!

The "old" version reports:

  • Path /tmp//bas_root_dir is a symlink. Unable to move it aside without move_obstructions is set
  • 2013-11-14T14:49:15+0000 info: Path '/tmp//bas_root_dir' is a symlink. Unable to move it aside without move_obstructions is set

this is the example
''''
bundle agent test
{
vars:
any::
"exclude_dirs" slist => { ".svn" };

files:
any::
"/tmp//bas_root_dir"
copy_from => purge_copy("/etc/cfengine3"),
depth_search => select_recurse("inf", @(exclude_dirs)),
action => update_immediate;
}

body copy_from purge_copy(from)
{
source => "$(from)";
compare => "hash";
copy_backup => "false";
purge => "true";
trustkey => "true";
preserve => "true";
}

body depth_search select_recurse(d,exclude_list)
{
depth => "$(d)";
xdev => "false";
exclude_dirs => { @(exclude_list) };
}

body action update_immediate
{
ifelapsed => "0";
}
''''

@kacf
Copy link
Contributor

kacf commented Nov 14, 2013

@markburgess, @vohi, @basvandervlies: Thanks for explaining. There is indeed a security hole there, since this function also deals with untrusted directories. I will revert it tomorrow.

@awsiv
Copy link
Contributor

awsiv commented Nov 14, 2013

+1.. Great discussion indeed! Thanks @basvandervlies for verifying it

@basvandervlies
Copy link
Contributor Author

I learned a lot ;-). I will report the issue that '''bootstrap.c''' must report that '''sys.workdir/inputs''' is not directory and maybe remove it and recreate it.

Now for cf-serverd. symlinks are alllowed for masterfiles directory if there are on the same filesystem. Is this ok?

@kacf
Copy link
Contributor

kacf commented Nov 15, 2013

@basvandervlies: Me too!

I have reverted the commit in #1087.

@kacf
Copy link
Contributor

kacf commented Nov 15, 2013

@basvandervlies: I think the key element is whether the symlink itself is trusted. If it resides in /var/cfengine, and is owned by root, it should be safe to assume that it is a secure link. The same is not true for symlinks that could reside in user directories.

@basvandervlies
Copy link
Contributor Author

@kacfengine that is correct. We have to check if the owner of the link is the same as the directory were it points to.

@basvandervlies
Copy link
Contributor Author

@kacfengine i just added a check if the owners matches and if not display a message.
''''
11:49 r7n16.lisa.surfsara.nl:/var/tmp/bas/root/Cfengine-debian
root# cf-agent/cf-agent -KI -f /var/tmp/copy.cf
2013-11-15T11:49:31+0100 error: /test/files/'/tmp//bas_root_dir': Security viollation symlink owner is different
''''
i don not know if this accpetable?

@kacf
Copy link
Contributor

kacf commented Nov 18, 2013

Yes, I think a fix like that is what we need. I'll be discussing this issue a bit in-house first, to try to minimize the chance of ending up in this situation again.

@basvandervlies
Copy link
Contributor Author

just reopen the call so it is not off the radar. we can always close it if it is not accepted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants