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

fix: Attempt to create log file if it does not exist. #1322

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
41 changes: 21 additions & 20 deletions freshclam/freshclam.c
Original file line number Diff line number Diff line change
Expand Up @@ -811,26 +811,6 @@ static fc_error_t initialize(struct optstruct *opts)
#endif
}

#ifdef HAVE_PWD_H
/* Drop database privileges here if we are not planning on daemonizing. If
* we are, we should wait until after we create the PidFile to drop
* privileges. That way, it is owned by root (or whoever started freshclam),
* and no one can change it. */
if (!optget(opts, "daemon")->enabled) {
/*
* freshclam shouldn't work with root privileges.
* Drop privileges to the DatabaseOwner user, if specified.
* Pass NULL for the log file name, because it hasn't been created yet.
*/
ret = drop_privileges(optget(opts, "DatabaseOwner")->strarg, NULL);
if (ret) {
logg(LOGG_ERROR, "Failed to switch to %s user.\n", optget(opts, "DatabaseOwner")->strarg);
status = FC_ECONFIG;
goto done;
}
}
#endif /* HAVE_PWD_H */

/*
* Initialize libclamav.
*/
Expand Down Expand Up @@ -982,6 +962,7 @@ static fc_error_t initialize(struct optstruct *opts)
fcConfig.requestTimeout = optget(opts, "ReceiveTimeout")->numarg;

fcConfig.bCompressLocalDatabase = optget(opts, "CompressLocalDatabase")->enabled;
fcConfig.dbOwner = optget(opts, "DatabaseOwner")->strarg;

/*
* Initialize libfreshclam.
Expand All @@ -992,6 +973,26 @@ static fc_error_t initialize(struct optstruct *opts)
goto done;
}

#ifdef HAVE_PWD_H
/* Drop database privileges here (cause of log file creation in /var/log) if we are not planning on daemonizing. If
* we are, we should wait until after we create the PidFile to drop
* privileges. That way, it is owned by root (or whoever started freshclam),
* and no one can change it. */
if (!optget(opts, "daemon")->enabled) {
/*
* freshclam shouldn't work with root privileges.
* Drop privileges to the DatabaseOwner user, if specified.
* Pass NULL for the log file name, because it hasn't been created yet.
*/
ret = drop_privileges(optget(opts, "DatabaseOwner")->strarg, NULL);
if (ret) {
logg(LOGG_ERROR, "Failed to switch to %s user.\n", optget(opts, "DatabaseOwner")->strarg);
status = FC_ECONFIG;
goto done;
}
}
#endif /* HAVE_PWD_H */

/*
* Set libfreshclam callback functions.
*/
Expand Down
67 changes: 67 additions & 0 deletions libfreshclam/libfreshclam.c
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,71 @@ const char *fc_strerror(fc_error_t fcerror)
}
}

int fc_upsert_logg_file(fc_config *fcConfig)
{
if (fcConfig->logFile == NULL) {
return 0;
}
int ret = 0, field_no = 1;
char *current_path, *file_path = cli_safer_strdup(fcConfig->logFile), *token;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These pointers should be initialized to NULL.

The strdup call should also be on a separate line and should have error handling.

FILE *logg_fp = NULL;
token = cli_strtok(file_path, field_no++, PATHSEP);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be easier to follow if this line were moved down to just above the while() loop.

On the topic of readability, Some blank lines between unrelated lines of code would be nice on the eyes.

struct passwd *current_user, *db_owner;
#ifndef _WIN32
current_user = getpwuid(getuid());
db_owner = getpwnam(fcConfig->dbOwner);
#endif /* _WIN32 */
current_path = (char *)malloc(2);
strcpy(current_path, PATHSEP);
STATBUF sb;
while (token != NULL) {
current_path = (char *)realloc(current_path, strlen(current_path) + strlen(token) + 2);
strcat(current_path, token);
free(token);
token = cli_strtok(file_path, field_no++, PATHSEP);
if (token == NULL) {
break;
}
if (LSTAT(current_path, &sb) == -1) {
if (mkdir(current_path, 0755) == -1) {
printf("ERROR: Failed to create required directory %s. Will continue without writing in %s.\n", current_path, fcConfig->logFile);
ret = -1;
goto cleanup;
}
#ifndef _WIN32
if (current_user->pw_uid != db_owner->pw_uid) {
if (lchown(current_path, db_owner->pw_uid, db_owner->pw_gid) == -1) {
printf("ERROR: Failed to change owner of %s to %s. Will continue without writing in %s.\n", current_path, fcConfig->dbOwner, fcConfig->logFile);
ret = -1;
goto cleanup;
}
}
#endif /* _WIN32 */
}
strcat(current_path, PATHSEP);
}
if ((logg_fp = fopen(fcConfig->logFile, "at")) == NULL) {
printf("ERROR: Can't open %s in append mode (check permissions!).\n", fcConfig->logFile);
ret = -1;
goto cleanup;
}
#ifndef _WIN32
lchown(fcConfig->logFile, db_owner->pw_uid, db_owner->pw_gid);
#endif /* _WIN32 */
Comment on lines +140 to +176
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some inline comments on this process would be nice.

My understanding is it basically tries to do a mkdir -p on the logfile directory path and change ownership to the database owner as needed. Once all the directories are confirmed to exist and be owned by the right user, it will open a log file and change ownership as needed.

The logic seems good to me for linux/unix systems. I'll have to try it out.

I see an effort to compile on Windows but I don't think it'll work on Windows. Paths on windows start with a drive letter, unless it's a relative path. Since this is called for all systems, I expect it to fail to run on Windows, or create some extra directories somewhere (maybe in the current directory?).

This change is also going to require that the freshclam.conf UpdateLogFile option is always an absolute path. I suspect it works okay as a relative path right now so this could possibly break someone's configuration.


cleanup:
if (token != NULL) {
free(token);
}
free(current_path);
free(file_path);
if (logg_fp != NULL) {
fclose(logg_fp);
}

return ret;
}

fc_error_t fc_initialize(fc_config *fcConfig)
{
fc_error_t status = FC_EARG;
Expand Down Expand Up @@ -157,6 +222,8 @@ fc_error_t fc_initialize(fc_config *fcConfig)
logg_rotate = (fcConfig->logFlags & FC_CONFIG_LOG_ROTATE) ? 1 : 0;
logg_size = fcConfig->maxLogSize;
/* Set a log file if requested, and is not already set */
fc_upsert_logg_file(fcConfig);

if ((NULL == logg_file) && (NULL != fcConfig->logFile)) {
logg_file = cli_safer_strdup(fcConfig->logFile);
if (0 != logg(LOGG_INFO_NF, "--------------------------------------\n")) {
Expand Down
1 change: 1 addition & 0 deletions libfreshclam/libfreshclam.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ typedef struct fc_config_ {
const char *proxyPassword; /**< (optional) Password for proxy server authentication. */
const char *databaseDirectory; /**< Filepath of database directory. */
const char *tempDirectory; /**< Filepath to store temp files. */
const char *dbOwner; /**< Owner of DB, used when creating log file/folder. */
} fc_config;

typedef enum fc_error_tag {
Expand Down
Loading