From 9fab54d43c052321c10436adb7bf810215d18b13 Mon Sep 17 00:00:00 2001 From: Tomasz bla Fortuna Date: Sun, 3 Jan 2010 15:01:12 +0100 Subject: [PATCH] Moved parts of state.c to db_file. Lock/temporary file locating functions. This should make state file simplier, but complicated a bit db_file.c. Creating init/fini functions of db backend could solve thing nicely. --- ChangeLog | 4 + libotp/db_file.c | 225 +++++++++++++++++++++++++++++-------- libotp/ppp.c | 7 +- libotp/ppp_common.h | 31 ++++- libotp/state.c | 126 +-------------------- libotp/state.h | 13 +-- utility/otpasswd_actions.c | 2 +- 7 files changed, 225 insertions(+), 183 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0a01b72..fe1ece2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -48,6 +48,10 @@ Trying to sort tasks according to their priority. * [+] Because of signals - redo permissions. (SUID required) * [+] The key/counter length is not checked when read from file. * [+] Big thing - Move state files to /etc + SUID. + * [?] Any possibility to change directory from /etc/otpasswd? + There's config so it can't be defined there. + * [?] Global DB should work with users which aren't in passwd. + * [?] fsync before rename/unlock (see ext4 problem) * [?] Check if lock files are links. if so. fail. Or rather always unlink before overwritting. * [?] Logging into syslog from utility if SUID; Also deny -v. diff --git a/libotp/db_file.c b/libotp/db_file.c index 555d916..dcbac5e 100644 --- a/libotp/db_file.c +++ b/libotp/db_file.c @@ -25,6 +25,7 @@ #include /* usleep, open, close, unlink, getuid */ #include #include /* stat */ +#include /* getpwnam */ #include #include "print.h" @@ -56,7 +57,7 @@ static char *_strtok(char *input, const char *delim) /* Check if db exists, and enforce permissions. * - enforce it's correct permissions */ -static int _db_file_permissions(const state *s) +static int _db_file_permissions(const char *db_path) { const uid_t uid = getuid(); struct stat st; @@ -121,7 +122,7 @@ static int _db_file_permissions(const state *s) */ /* 1) Check if state file exists and read it's parameters */ - if (stat(s->db_path, &st) != 0) { + if (stat(db_path, &st) != 0) { /* Does not exists */ if (cfg->db == CONFIG_DB_USER) return STATE_NON_EXISTENT; @@ -139,7 +140,7 @@ static int _db_file_permissions(const state *s) /* 3) Others/Group */ if (st.st_mode & (S_IRWXG | S_IRWXO)) { - if (chmod(s->db_path, S_IRUSR|S_IWUSR) != 0) { + if (chmod(db_path, S_IRUSR|S_IWUSR) != 0) { print_perror(PRINT_NOTICE, "Unable to enforce state file permissions.\n"); return STATE_IO_ERROR; @@ -172,6 +173,89 @@ static int _db_file_permissions(const state *s) return 0; } +/* Returns a name of current state + lock + temp file */ +static int _db_path(const char *username, char **db, char **lck, char **tmp) +{ + int retval = 1; + static struct passwd *pwdata = NULL; + cfg_t *cfg = cfg_get(); + + + assert(username); + assert(cfg); + assert(!*db && !*lck && !*tmp); /* Must be NULL */ + + /* Determine db location at first */ + switch (cfg->db) { + case CONFIG_DB_USER: + { + char *home = NULL; + int length; + + /* Get home */ + pwdata = getpwnam(username); + if (pwdata && pwdata->pw_dir) + home = pwdata->pw_dir; + else { + return STATE_NO_SUCH_USER; + } + + /* Append a filename */ + length = strlen(home); + length += strlen(cfg->user_db_path); + length += 2; + + *db = malloc(length); + if (!*db) + return STATE_NOMEM; + + int ret = snprintf(*db, length, "%s/%s", home, cfg->user_db_path); + + assert( ret == length - 1 ); + + break; + } + case CONFIG_DB_GLOBAL: + *db = strdup(cfg->global_db_path); + if (!*db) { + return STATE_NOMEM; + } + break; + + case CONFIG_DB_MYSQL: + case CONFIG_DB_LDAP: + default: + /* We should not be called with this options never */ + assert(0); + } + + const int db_len = strlen(*db); + /* Create lock filename; normal file + .lck */ + + retval = STATE_NOMEM; + *lck = malloc(db_len + 5 + 1); + *tmp = malloc(db_len + 5 + 1); + + if (!*lck || !*tmp) { + goto error; + } + + retval = sprintf(*lck, "%s.lck", *db); + assert(retval > 0); + + retval = sprintf(*tmp, "%s.tmp", *db); + assert(retval > 0); + + /* All ok */ + return 0; + +error: + free(*db), *db = NULL; + free(*tmp), *tmp = NULL; + free(*lck), *lck = NULL; + return retval; +} + /* State files constants */ static const int _version = 6; static const char *_delim = ":"; @@ -338,24 +422,32 @@ int db_file_load(state *s) /* Value returned. */ int retval; - ret = _db_file_permissions(s); + /* Files: database, lock and temporary */ + char *db = NULL, *lck = NULL, *tmp = NULL; + ret = _db_path(s->username, &db, &lck, &tmp); if (ret != 0) { - print(PRINT_NOTICE, "Unable to load state file.\n"); return ret; } + retval = _db_file_permissions(db); + if (retval != 0) { + print(PRINT_NOTICE, "Unable to load state file.\n"); + goto cleanup1; + } + /* DB file should always be locked before changing. * Locking can only be omitted when we want to discard * any changes or that we don't bother if somebody changes * them at the same time. * Here we just detect that it's not locked and lock it then */ - if (s->lock_fd <= 0) { + if (s->lock <= 0) { print(PRINT_NOTICE, "State file not locked while reading from it\n"); - if (db_file_lock(s) != 0) { + retval = db_file_lock(s); + if (retval != 0) { print(PRINT_ERROR, "Unable to lock file for reading!\n"); - return STATE_LOCK_ERROR; + goto cleanup1; } /* Locked locally, unlock locally later */ @@ -364,11 +456,11 @@ int db_file_load(state *s) locked = 0; } - f = fopen(s->db_path, "r"); + f = fopen(db, "r"); if (!f) { print_perror(PRINT_ERROR, "Unable to open %s for reading.", - s->db_path); + db); retval = STATE_IO_ERROR; goto cleanup; } @@ -498,13 +590,13 @@ int db_file_load(state *s) if (s->code_length < 2 || s->code_length > 16) { print(PRINT_ERROR, "Illegal passcode length. %s is invalid\n", - s->db_path); + db); goto cleanup; } if (s->flags > (FLAG_SHOW|FLAG_SALTED)) { print(PRINT_ERROR, "Unsupported set of flags. %s is invalid\n", - s->db_path); + db); goto cleanup; } @@ -522,6 +614,11 @@ int db_file_load(state *s) } if (f) fclose(f); + +cleanup1: + free(db); + free(tmp); + free(lck); return retval; } @@ -595,22 +692,25 @@ int db_file_store(state *s) char user_entry_buff[STATE_ENTRY_SIZE]; - int tmp; - - assert(s->db_path != NULL); - assert(s->username != NULL); + /* Files: database, lock and temporary */ + char *db = NULL, *lck = NULL, *tmp = NULL; + ret = _db_path(s->username, &db, &lck, &tmp); + if (ret != 0) { + return ret; + } - if (s->lock_fd <= 0) { + if (s->lock <= 0) { print(PRINT_NOTICE, "State file not locked while writing to it\n"); - if (db_file_lock(s) != 0) { + ret = db_file_lock(s); + if (ret != 0) { print(PRINT_ERROR, "Unable to lock file for writing!\n"); - return STATE_LOCK_ERROR; + goto cleanup1; } locked = 1; } - in = fopen(s->db_path, "r"); + in = fopen(db, "r"); if (!in) { /* User=db and file doesn't exist - ok. */ @@ -619,18 +719,18 @@ int db_file_store(state *s) /* FIXME, better rewrite to use stat */ print_perror(PRINT_ERROR, "Unable to open %s for reading", - s->db_path); + db); ret = STATE_IO_ERROR; goto cleanup; } } - out = fopen(s->db_tmp_path, "w"); + out = fopen(tmp, "w"); if (!out) { print_perror(PRINT_ERROR, "Unable to open %s for writing", - s->db_tmp_path); + tmp); ret = STATE_IO_ERROR; goto cleanup; @@ -641,8 +741,8 @@ int db_file_store(state *s) * owner of original file if we are root! */ if (geteuid() == 0) { struct stat st; - stat(s->db_path, &st); - if (chown(s->db_tmp_path, st.st_uid, st.st_gid) != 0) { + stat(db, &st); + if (chown(tmp, st.st_uid, st.st_gid) != 0) { print_perror(PRINT_ERROR, "Unable to ensure owner/group of temporary file\n"); ret = STATE_IO_ERROR; goto cleanup; @@ -689,10 +789,10 @@ int db_file_store(state *s) } /* 4) Flush, save... then rename in cleanup part */ - tmp = fflush(out); - tmp += fclose(out); + ret = fflush(out); + ret += fclose(out); out = NULL; - if (tmp != 0) { + if (ret != 0) { print_perror(PRINT_ERROR, "Error while flushing/closing state file"); ret = STATE_IO_ERROR; goto cleanup; @@ -703,12 +803,14 @@ int db_file_store(state *s) cleanup: if (in) fclose(in); - if (out) + if (out) { + fflush(out); fclose(out); + } if (ret == 0) { /* If everything went fine, rename tmp to normal file */ - if (rename(s->db_tmp_path, s->db_path) != 0) { + if (rename(tmp, db) != 0) { print_perror(PRINT_WARN, "Unable to rename temporary state " "file and save state."); @@ -717,22 +819,27 @@ int db_file_store(state *s) /* It might fail, but shouldn't * Also we just want to ensure others * can't read this file */ - if (_db_file_permissions(s) != 0) { + if (_db_file_permissions(db) != 0) { print(PRINT_WARN, "Unable to set state file permissions. " "Key might be world-readable!\n"); } print(PRINT_NOTICE, "State file written correctly\n"); } - } else if (unlink(s->db_tmp_path) != 0) { + + } else if (unlink(tmp) != 0) { print_perror(PRINT_WARN, "Unable to unlink temporary state file %s", - s->db_tmp_path); + tmp); } if (locked && db_file_unlock(s) != 0) { print(PRINT_ERROR, "Error while unlocking state file!\n"); } +cleanup1: + free(db); + free(lck); + free(tmp); return ret; } @@ -743,20 +850,25 @@ int db_file_lock(state *s) int cnt; int fd; - assert(s->db_lck_path); - assert(s->db_path); - assert(s->lock_fd == -1); + assert(s->lock == -1); + + /* Files: database, lock and temporary */ + char *db = NULL, *lck = NULL, *tmp = NULL; + ret = _db_path(s->username, &db, &lck, &tmp); + if (ret != 0) { + return ret; + } fl.l_type = F_WRLCK; fl.l_whence = SEEK_SET; fl.l_start = fl.l_len = 0; /* Open/create lock file */ - fd = open(s->db_lck_path, O_WRONLY|O_CREAT, S_IWUSR|S_IRUSR); + fd = open(lck, O_WRONLY|O_CREAT, S_IWUSR|S_IRUSR); if (fd == -1) { /* Unable to create file, therefore unable to obtain lock */ - print_perror(PRINT_NOTICE, "Unable to create %s lock file", s->db_lck_path); + print_perror(PRINT_NOTICE, "Unable to create %s lock file", lck); goto error; } @@ -785,11 +897,16 @@ int db_file_lock(state *s) goto error; } - s->lock_fd = fd; + s->lock = fd; print(PRINT_NOTICE, "Got lock on state file\n"); return 0; /* Got lock */ + error: + free(db); + free(lck); + free(tmp); + return STATE_LOCK_ERROR; } @@ -798,30 +915,44 @@ int db_file_lock(state *s) int db_file_unlock(state *s) { struct flock fl; + int retval = STATE_LOCK_ERROR; + + /* Files: database, lock and temporary */ + char *db = NULL, *lck = NULL, *tmp = NULL; + retval = _db_path(s->username, &db, &lck, &tmp); + if (retval != 0) { + return retval; + } - if (s->lock_fd < 0) { + + if (s->lock < 0) { print(PRINT_NOTICE, "No lock to release!\n"); - return STATE_LOCK_ERROR; /* No lock to release */ + goto error; } fl.l_type = F_UNLCK; fl.l_whence = SEEK_SET; fl.l_start = fl.l_len = 0; - int ret = fcntl(s->lock_fd, F_SETLK, &fl); + int ret = fcntl(s->lock, F_SETLK, &fl); - close(s->lock_fd); - s->lock_fd = -1; + close(s->lock); + s->lock = -1; - unlink(s->db_lck_path); + unlink(lck); if (ret != 0) { print(PRINT_NOTICE, "Strange error while releasing lock\n"); /* Strange error while releasing the lock */ - return STATE_LOCK_ERROR; + goto error; } - return 0; + retval = 0; +error: + free(db); + free(lck); + free(tmp); + return retval; } diff --git a/libotp/ppp.c b/libotp/ppp.c index da4aac3..18fb281 100644 --- a/libotp/ppp.c +++ b/libotp/ppp.c @@ -532,7 +532,7 @@ const char *ppp_get_error_desc(int error) switch (error) { case 0: return "No error"; - case PPP_NOMEM: + case STATE_NOMEM: return "Out of memory while reading state."; case STATE_LOCK_ERROR: @@ -553,6 +553,9 @@ const char *ppp_get_error_desc(int error) case STATE_NO_USER_ENTRY: return "No user entry. Have you created key with --key option?"; + case STATE_NO_SUCH_USER: + return "No such Unix user in passwd database. Unable to locate home.\n"; + default: return "Error occured while reading state. Use -v to determine which."; } @@ -566,7 +569,7 @@ int ppp_init(state **s, const char *user) int ret; *s = malloc(sizeof(**s)); if (!*s) - return PPP_NOMEM; + return STATE_NOMEM; ret = state_init(*s, user); if (ret == 0) diff --git a/libotp/ppp_common.h b/libotp/ppp_common.h index ff5b1ca..6b5e8f4 100644 --- a/libotp/ppp_common.h +++ b/libotp/ppp_common.h @@ -1,3 +1,21 @@ +/********************************************************************** + * otpasswd -- One-time password manager and PAM module. + * Copyright (C) 2009 by Tomasz bla Fortuna + * + * 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, either version 3 of the License, or + * (at your option) any later version. + * + * 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 otpasswd. If not, see . + **********************************************************************/ + #ifndef _PPP_COMMON_H_ #define _PPP_COMMON_H_ @@ -17,12 +35,12 @@ /* We must distinguish between locking problems (critical) * and non-existant state file (usually not critical). - * + * * Depending on db used and enforce option sometimes we * should ignore OTP login and sometimes we should hard-fail. */ enum errors { - PPP_NOMEM = 40, + STATE_NOMEM = 40, /*** ALWAYS FAIL ***/ /* Error while locking (existing) state file */ @@ -32,24 +50,27 @@ enum errors { STATE_PARSE_ERROR = 51, /* Counter too big. Key should be regenerated */ - STATE_NUMSPACE = 52, + STATE_NUMSPACE = 52, /* File exists, but we're unable to open/read/write * state file (not a file, permissions might be wrong). */ STATE_IO_ERROR = 53, + /* User doesn't exists in Unix database + * but was required because of home directory */ + STATE_NO_SUCH_USER = 54, /*** NOT ALWAYS FATAL */ /* State doesn't exist. * If enforce = 0 - ignore OTP. */ - STATE_NON_EXISTENT = 54, + STATE_NON_EXISTENT = 55, /* State exists, is readable, but doesn't have * user entry. Always causes ignore if enforce=0 */ - STATE_NO_USER_ENTRY = 55, + STATE_NO_USER_ENTRY = 56, }; diff --git a/libotp/state.c b/libotp/state.c index 4ab2d33..e5ece1c 100644 --- a/libotp/state.c +++ b/libotp/state.c @@ -38,9 +38,9 @@ int state_validate_str(const char *str) int i; /* Spaces are ok, \n and \r not. * alpha, digits, +, -, @, _ are ok - * These characters should be LaTeX safe, so no {} - * Also they can be passed to some external script, - * so they must obey restrictions. + * These characters should be Shell & LaTeX safe, so no {} + * and other strange characters which aren't necessary + * for labels and contacts. */ for (i=0; ipw_dir) - home = pwdata->pw_dir; - else - return NULL; - - /* Append a filename */ - length = strlen(home); - length += strlen(cfg->user_db_path); - length += 2; - - name = malloc(length); - if (!name) - goto error; - - int ret = snprintf(name, length, "%s/%s", home, cfg->user_db_path); - - assert( ret == length - 1 ); - - if (ret != length - 1) { - goto error; - } - - return name; - -error: - free(name); - return NULL; -} - -static int _state_lck_tmp_path(const char *base, char **lck, char **tmp) -{ - int ret; - - /* Create lock filename; normal file + .lck */ - *lck = malloc(strlen(base) + 5 + 1); - - if (!*lck) { - return PPP_NOMEM; - } - - *tmp = malloc(strlen(base) + 5 + 1); - if (!*tmp) { - free(*lck); - *lck = NULL; - return PPP_NOMEM; - } - - ret = sprintf(*lck, "%s.lck", base); - assert(ret > 0); - - ret = sprintf(*tmp, "%s.tmp", base); - assert(ret > 0); - - return 0; -} - /****************************************** * Functions for managing state information @@ -159,11 +90,9 @@ int state_init(state *s, const char *username) s->failures = 0; s->recent_failures = 0; s->spass_set = 0; - s->lock_fd = -1; + s->lock = -1; s->prompt = NULL; - s->db_path = NULL; - s->db_lck_path = NULL; memset(s->label, 0x00, sizeof(s->label)); memset(s->contact, 0x00, sizeof(s->contact)); @@ -181,45 +110,6 @@ int state_init(state *s, const char *username) s->codes_on_card = s->codes_in_row = s->current_row = s->current_column = 0; - /** Determine state file position */ - switch (cfg->db) { - case CONFIG_DB_USER: - s->db_path = _state_user_db_file(username); - - if (s->db_path == NULL) { - print(PRINT_ERROR, "Unable to determine home directory of user\n"); - return 4; - } - - /* Create lock filename; normal file + .lck */ - if (_state_lck_tmp_path(s->db_path, - &s->db_lck_path, - &s->db_tmp_path) != 0) { - free(s->db_path), s->db_path = NULL; - return PPP_NOMEM; - } - break; - - case CONFIG_DB_GLOBAL: - s->db_path = strdup(cfg->global_db_path); - if (!s->db_path) { - return PPP_NOMEM; - } - - if (_state_lck_tmp_path(s->db_path, - &s->db_lck_path, - &s->db_tmp_path) != 0) { - free(s->db_path), s->db_path = NULL; - return PPP_NOMEM; - } - break; - - case CONFIG_DB_MYSQL: - case CONFIG_DB_LDAP: - print(PRINT_ERROR, "Database type not yet implemented.\n"); - return 1; - } - /* Save user name in state */ s->username = strdup(username); @@ -240,13 +130,12 @@ int state_init(state *s, const char *username) ret = mpz_init_set_str(s->code_mask, code_mask, 16); assert(ret == 0); - return 0; } void state_fini(state *s) { - if (s->lock_fd > 0) + if (s->lock > 0) state_unlock(s); mpz_clear(s->counter); @@ -267,9 +156,6 @@ void state_fini(state *s) s->prompt = NULL; } - free(s->db_path); - free(s->db_lck_path); - free(s->db_tmp_path); free(s->username); /* Clear the rest of memory */ @@ -349,8 +235,6 @@ int state_lock(state *s) cfg_t *cfg = cfg_get(); assert(cfg); - assert(s->db_path != NULL); - switch (cfg->db) { case CONFIG_DB_USER: case CONFIG_DB_GLOBAL: diff --git a/libotp/state.h b/libotp/state.h index ac2036f..ee38d01 100644 --- a/libotp/state.h +++ b/libotp/state.h @@ -31,8 +31,6 @@ /*** State ***/ typedef struct { - /*** State stored in STATE_FILENAME ***/ - /* 128 bit counter pointing at the next passcode * which will be used for authentication */ mpz_t counter; @@ -102,11 +100,12 @@ typedef struct { * state information. */ char *username; /* user who called utility or does auth */ - char *db_path; /* Path to state file */ - char *db_lck_path; /* Name of lock filename */ - char *db_tmp_path; /* Temporary state file location */ - int lock_fd; /* Descriptor of state lock - * will be < 0 if file not locked */ + + /* DB lock status.-1 value means state is not locked + * while all other values mean that it's locked + * and can have some other db_ related information + * (like descriptor of opened lock file) */ + int lock; } state; diff --git a/utility/otpasswd_actions.c b/utility/otpasswd_actions.c index 9cffcfe..254f91e 100644 --- a/utility/otpasswd_actions.c +++ b/utility/otpasswd_actions.c @@ -561,7 +561,7 @@ int action_key(options_t *options, const cfg_t *cfg) ret = state_store(&s); /* This should auto lock */ if (ret != 0) { - print(PRINT_ERROR, "Unable to save state to %s file\n", s.db_path); + print(PRINT_ERROR, "Unable to save state.\n"); print(PRINT_NOTICE, "(%s)\n", ppp_get_error_desc(ret)); goto cleanup; }