Skip to content

Commit

Permalink
Implemented safe_open() in order to avoid symlink race conditions.
Browse files Browse the repository at this point in the history
It makes sure that we do not follow unsafe symlinks, that is a link
that belongs to a user, but has a target owned by a different user.
It does this in a way that avoids race conditions (between testing if
a file is a link, and opening that file, there is a small window of
opportunity).

Also implements safe_chdir(), to also safeguard chdir() operations.

Fixes issue #3693.
  • Loading branch information
kacf committed Nov 26, 2013
1 parent f0bc83e commit 995b9d1
Show file tree
Hide file tree
Showing 32 changed files with 1,225 additions and 49 deletions.
5 changes: 3 additions & 2 deletions cf-agent/files_editline.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include <locks.h>
#include <string_lib.h>
#include <misc_lib.h>
#include <file_lib.h>
#include <rlist.h>
#include <policy.h>
#include <ornaments.h>
Expand Down Expand Up @@ -152,7 +153,7 @@ int ScheduleEditLineOperations(EvalContext *ctx, Bundle *bp, Attributes a, const
Bundle *MakeTemporaryBundleFromTemplate(EvalContext *ctx, Policy *policy, Attributes a, const Promise *pp, PromiseResult *result)
{
FILE *fp = NULL;
if ((fp = fopen(a.edit_template, "r" )) == NULL)
if ((fp = safe_fopen(a.edit_template, "r" )) == NULL)
{
cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "Unable to open template file '%s' to make '%s'", a.edit_template, pp->promiser);
*result = PromiseResultUpdate(*result, PROMISE_RESULT_INTERRUPTED);
Expand Down Expand Up @@ -1238,7 +1239,7 @@ static int InsertFileAtLocation(EvalContext *ctx, Item **start, Item *begin_ptr,
Item *loc = NULL;
int preserve_block = a.sourcetype && strcmp(a.sourcetype, "file_preserve_block") == 0;

if ((fin = fopen(pp->promiser, "r")) == NULL)
if ((fin = safe_fopen(pp->promiser, "r")) == NULL)
{
cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "Could not read file '%s'. (fopen: %s)", pp->promiser, GetErrorStr());
*result = PromiseResultUpdate(*result, PROMISE_RESULT_INTERRUPTED);
Expand Down
2 changes: 1 addition & 1 deletion cf-agent/files_operators.c
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ static bool SaveItemListCallback(const char *dest_filename, void *param)
FILE *fp;

//saving list to file
if ((fp = fopen(dest_filename, "w")) == NULL)
if ((fp = safe_fopen(dest_filename, "w")) == NULL)
{
Log(LOG_LEVEL_ERR, "Unable to open destination file '%s' for writing. (fopen: %s)",
dest_filename, GetErrorStr());
Expand Down
4 changes: 2 additions & 2 deletions cf-agent/verify_files.c
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ static PromiseResult VerifyFilePromise(EvalContext *ctx, char *path, Promise *pp
}

ChopLastNode(basedir);
if (chdir(basedir))
if (safe_chdir(basedir))
{
Log(LOG_LEVEL_ERR, "Failed to chdir into '%s'", basedir);
}
Expand Down Expand Up @@ -619,7 +619,7 @@ PromiseResult ScheduleEditOperation(EvalContext *ctx, char *filename, Attributes

Writer *ouput_writer = NULL;
{
FILE *output_file = fopen(pp->promiser, "w");
FILE *output_file = safe_fopen(pp->promiser, "w");
if (!output_file)
{
cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, "Output file '%s' could not be opened for writing", pp->promiser);
Expand Down
6 changes: 3 additions & 3 deletions cf-agent/verify_files_hashes.c
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ int CompareBinaryFiles(const char *file1, const char *file2, struct stat *sstat,

if ((fc.servers == NULL) || (strcmp(RlistScalarValue(fc.servers), "localhost") == 0))
{
fd1 = open(file1, O_RDONLY | O_BINARY, 0400);
fd2 = open(file2, O_RDONLY | O_BINARY, 0400);
fd1 = safe_open(file1, O_RDONLY | O_BINARY, 0400);
fd2 = safe_open(file2, O_RDONLY | O_BINARY, 0400);

do
{
Expand Down Expand Up @@ -395,7 +395,7 @@ void LogHashChange(const char *file, FileState status, char *msg, Promise *pp)
}
#endif /* !__MINGW32__ */

if ((fp = fopen(fname, "a")) == NULL)
if ((fp = safe_fopen(fname, "a")) == NULL)
{
Log(LOG_LEVEL_ERR, "Could not write to the hash change log. (fopen: %s)", GetErrorStr());
return;
Expand Down
14 changes: 7 additions & 7 deletions cf-agent/verify_files_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ static PromiseResult PurgeLocalFiles(EvalContext *ctx, Item *filelist, const cha

/* chdir to minimize the risk of race exploits during copy (which is inherently dangerous) */

if (chdir(localdir) == -1)
if (safe_chdir(localdir) == -1)
{
Log(LOG_LEVEL_VERBOSE, "Can't chdir to local directory '%s'. (chdir: %s)", localdir, GetErrorStr());
return PROMISE_RESULT_NOOP;
Expand Down Expand Up @@ -1319,8 +1319,8 @@ bool CopyRegularFile(EvalContext *ctx, const char *source, const char *dest, str
#ifdef __APPLE__
if (rsrcfork)
{ /* Can't just "mv" the resource fork, unfortunately */
rsrcrd = open(new, O_RDONLY | O_BINARY);
rsrcwd = open(dest, O_WRONLY | O_BINARY | O_CREAT | O_TRUNC, 0600);
rsrcrd = safe_open(new, O_RDONLY | O_BINARY);
rsrcwd = safe_open(dest, O_WRONLY | O_BINARY | O_CREAT | O_TRUNC, 0600);

if (rsrcrd == -1 || rsrcwd == -1)
{
Expand Down Expand Up @@ -2089,7 +2089,7 @@ int DepthSearch(EvalContext *ctx, char *name, struct stat *sb, int rlevel, Attri
Log(LOG_LEVEL_DEBUG, "Direct file reference '%s', no search implied", name);
snprintf(basedir, sizeof(basedir), "%s", name);
ChopLastNode(basedir);
if (chdir(basedir))
if (safe_chdir(basedir))
{
Log(LOG_LEVEL_ERR, "Failed to chdir into '%s'", basedir);
return false;
Expand Down Expand Up @@ -2203,7 +2203,7 @@ int DepthSearch(EvalContext *ctx, char *name, struct stat *sb, int rlevel, Attri

static int PushDirState(EvalContext *ctx, char *name, struct stat *sb)
{
if (chdir(name) == -1)
if (safe_chdir(name) == -1)
{
Log(LOG_LEVEL_INFO, "Could not change to directory '%s', mode '%04jo' in tidy. (chdir: %s)",
name, (uintmax_t)(sb->st_mode & 07777), GetErrorStr());
Expand All @@ -2224,7 +2224,7 @@ static bool PopDirState(int goback, char *name, struct stat *sb, Recursion r)
{
if (goback && (r.travlinks))
{
if (chdir(name) == -1)
if (safe_chdir(name) == -1)
{
Log(LOG_LEVEL_ERR, "Error in backing out of recursive travlink descent securely to '%s'. (chdir: %s)",
name, GetErrorStr());
Expand All @@ -2238,7 +2238,7 @@ static bool PopDirState(int goback, char *name, struct stat *sb, Recursion r)
}
else if (goback)
{
if (chdir("..") == -1)
if (safe_chdir("..") == -1)
{
Log(LOG_LEVEL_ERR, "Error in backing out of recursive descent securely to '%s'. (chdir: %s)",
name, GetErrorStr());
Expand Down
3 changes: 2 additions & 1 deletion cf-agent/verify_packages.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <dir.h>
#include <files_names.h>
#include <files_interfaces.h>
#include <file_lib.h>
#include <vars.h>
#include <conversion.h>
#include <expand.h>
Expand Down Expand Up @@ -125,7 +126,7 @@ PromiseResult VerifyPackagesPromise(EvalContext *ctx, Promise *pp)

// Start by reseting the root directory in case yum tries to glob regexs(!)

if (chdir("/") != 0)
if (safe_chdir("/") != 0)
{
Log(LOG_LEVEL_ERR, "Failed to chdir into '/'");
}
Expand Down
5 changes: 3 additions & 2 deletions cf-execd/cf-execd-runner.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <mutex.h>
#include <exec_tools.h>
#include <misc_lib.h>
#include <file_lib.h>
#include <assert.h>
#include <crypto.h>
#include <known_dirs.h>
Expand Down Expand Up @@ -354,8 +355,8 @@ static int CompareResult(const char *filename, const char *prev_file)

int rtn = 0;

FILE *old_fp = fopen(prev_file, "r");
FILE *new_fp = fopen(filename, "r");
FILE *old_fp = safe_fopen(prev_file, "r");
FILE *new_fp = safe_fopen(filename, "r");
if (old_fp && new_fp)
{
const char *errptr;
Expand Down
7 changes: 4 additions & 3 deletions cf-key/cf-key-functions.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include <communication.h>
#include <env_context.h>
#include <crypto.h>
#include <file_lib.h>

#include <cf-key-functions.h>

Expand All @@ -45,7 +46,7 @@ RSA* LoadPublicKey(const char* filename)
RSA* key;
static char *passphrase = "Cfengine passphrase";

fp = fopen(filename, "r");
fp = safe_fopen(filename, "r");
if (fp == NULL)
{
Log(LOG_LEVEL_ERR, "Cannot open file '%s'. (fopen: %s)", filename, GetErrorStr());
Expand Down Expand Up @@ -252,7 +253,7 @@ void KeepKeyPromises(const char *public_key_file, const char *private_key_file)
return;
}

fd = open(private_key_file, O_WRONLY | O_CREAT | O_TRUNC, 0600);
fd = safe_open(private_key_file, O_WRONLY | O_CREAT | O_TRUNC, 0600);

if (fd < 0)
{
Expand All @@ -278,7 +279,7 @@ void KeepKeyPromises(const char *public_key_file, const char *private_key_file)

fclose(fp);

fd = open(public_key_file, O_WRONLY | O_CREAT | O_TRUNC, 0600);
fd = safe_open(public_key_file, O_WRONLY | O_CREAT | O_TRUNC, 0600);

if (fd < 0)
{
Expand Down
2 changes: 1 addition & 1 deletion cf-serverd/cf-serverd-functions.c
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ static int GenerateAvahiConfig(const char *path)
{
FILE *fout;
Writer *writer = NULL;
fout = fopen(path, "w+");
fout = safe_fopen(path, "w+");
if (fout == NULL)
{
Log(LOG_LEVEL_ERR, "Unable to open '%s'", path);
Expand Down
5 changes: 3 additions & 2 deletions cf-serverd/server_common.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ static const int CF_NOSIZE = -1;
#include <files_names.h>
#include <files_interfaces.h>
#include <files_hashes.h>
#include <file_lib.h>
#include <env_context.h>
#include <dir.h>
#include <conversion.h>
Expand Down Expand Up @@ -1028,7 +1029,7 @@ void CfGetFile(ServerFileGetState *args)

/* File transfer */

if ((fd = open(filename, O_RDONLY)) == -1)
if ((fd = safe_open(filename, O_RDONLY)) == -1)
{
Log(LOG_LEVEL_ERR, "Open error of file '%s'. (open: %s)",
filename, GetErrorStr());
Expand Down Expand Up @@ -1175,7 +1176,7 @@ void CfEncryptGetFile(ServerFileGetState *args)

EVP_CIPHER_CTX_init(&ctx);

if ((fd = open(filename, O_RDONLY)) == -1)
if ((fd = safe_open(filename, O_RDONLY)) == -1)
{
Log(LOG_LEVEL_ERR, "Open error of file '%s'. (open: %s)", filename, GetErrorStr());
FailedTransfer(conn_info);
Expand Down
4 changes: 4 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,10 @@ AC_REPLACE_FUNCS(pthread_attr_setstacksize)
AC_CHECK_DECLS(pthread_sigmask, [], [], [[#include <signal.h>]])
AC_REPLACE_FUNCS(pthread_sigmask)

AC_CHECK_DECLS([openat], [], [], [[#include <fcntl.h>]])
AC_CHECK_DECLS([fstatat], [], [], [[#include <sys/stat.h>]])
AC_REPLACE_FUNCS(openat fstatat)

dnl ######################################################################
dnl Required by cf-upgrade. It cannot be implemented in libcompat because
dnl cf-upgrade does not link to any libraries except libutils and only
Expand Down
4 changes: 2 additions & 2 deletions libcfnet/client_code.c
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ int EncryptCopyRegularFileNet(const char *source, const char *dest, off_t size,

unlink(dest); /* To avoid link attacks */

if ((dd = open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_BINARY, 0600)) == -1)
if ((dd = safe_open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_BINARY, 0600)) == -1)
{
Log(LOG_LEVEL_ERR,
"NetCopy to destination '%s:%s' security - failed attempt to exploit a race? (Not copied). (open: %s)",
Expand Down Expand Up @@ -1026,7 +1026,7 @@ int CopyRegularFileNet(const char *source, const char *dest, off_t size, bool en

unlink(dest); /* To avoid link attacks */

if ((dd = open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_BINARY, 0600)) == -1)
if ((dd = safe_open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_BINARY, 0600)) == -1)
{
Log(LOG_LEVEL_ERR,
"NetCopy to destination '%s:%s' security - failed attempt to exploit a race? (Not copied) (open: %s)",
Expand Down
27 changes: 27 additions & 0 deletions libcompat/chdir_lock.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
Copyright (C) CFEngine AS
This file is part of CFEngine 3 - written and maintained by CFEngine AS.
This program is free software; you can redistribute it and/or modify it
under the terms of the GNU General Public License as published by the
Free Software Foundation; version 3.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA
To the extent this program is licensed as part of the Enterprise
versions of CFEngine, the applicable Commercial Open Source License
(COSL) may apply to this file if you as a licensee so wish it. See
included file COSL.txt.
*/

#include <pthread.h>

extern pthread_mutex_t CHDIR_LOCK;
Loading

0 comments on commit 995b9d1

Please sign in to comment.