diff --git a/CREDITS b/CREDITS index ab6edd06..14753ea4 100644 --- a/CREDITS +++ b/CREDITS @@ -53,3 +53,10 @@ Hank Leininger - For iptables firewalls, suggested a check for the 'comment' match to ensure the local environment will properly support fwknopd operations. The result is the new ENABLE_IPT_COMMENT_CHECK functionality. + +Fernando Arnaboldi (IOActive) + - Found important buffer overflow conditions for authenticated SPA clients + in the fwknopd server (pre-2.0.3). These findings enabled fixes to be + developed along with a new fuzzing capability in the test suite. + - Found a condition in which an overly long IP from malicious authenticated + clients is not properly validated by the fwknopd server (pre-2.0.3). diff --git a/ChangeLog b/ChangeLog index 1568a1aa..5e800b03 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,30 @@ +fwknop-2.0.3 (08//2012): + - Fixed RPM builds by including the $(DESTDIR) prefix for uninstall-local + and install-exec-hook stages in Makefile.am. + - [server] Fernando Arnaboldi from IOActive found several DoS/code + execution vulnerabilities for malicious fwknop clients that manage to + get past the authentication stage (so a such a client must be in + possession of a valid access.conf encryption key). These vulnerbilities + manifested themselves in the handling of malformed access requests, and + both the fwknopd server code along with libfko now perform stronger input + validation of access request data. These vulnerabilities affect + pre-2.0.3 fwknop releases. + - [server] Fernando Arnaboldi from IOActive found a condition in which + the server did not properly validate allow IP addresses from malicious + authenticated clients. This has been fixed with stronger allow IP + validation. + - [client+server] Fernando Arnaboldi from IOActive found that strict + filesystem permissions for various fwknop files are not verified. Added + warnings whenever permissions are not strict enough, and ensured that + files created by the fwknop client and server are only set to user + read/write. + - [client] Fernando Arnaboldi from IOActive found a local buffer overflow + in --last processing with a maliciously constructed ~/.fwknop.run file. + This has been fixed with proper validation of .fwknop.run arguments. + - [test suite] Added a new fuzzing capability to ensure proper server-side + input validation. Fuzzing data is constructed with modified fwknop + client code that is designed to emulate malicious behavior. + fwknop-2.0.2 (08/18/2012): - [server] For GPG mode, added a new access.conf variable "GPG_ALLOW_NO_PW" to make it possible to leverage a server-side GPG key diff --git a/Makefile.am b/Makefile.am index 9df6222e..1d83d5db 100644 --- a/Makefile.am +++ b/Makefile.am @@ -13,7 +13,7 @@ SUBDIRS = \ common \ $(CLIENT_DIR) \ $(SERVER_DIR) \ - doc + doc EXTRA_DIST = \ android/COPYING \ @@ -149,6 +149,7 @@ EXTRA_DIST = \ test/conf/require_user_access.conf \ test/conf/subnet_source_match_access.conf \ test/conf/local_nat_fwknopd.conf \ + test/conf/disable_aging_fwknopd.conf \ test/hardening-check \ test/local_spa.key \ test/test-fwknop.pl \ @@ -173,39 +174,38 @@ dist-hook: rm -f $(distdir)/server/fwknopd.8 uninstall-local: - if test -f $(sysconfdir)/fwknop/fwknopd.conf; then \ - rm -f $(sysconfdir)/fwknop/fwknopd.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; \ fi - if test -f $(sysconfdir)/fwknop/fwknopd.conf.inst; then \ - rm -f $(sysconfdir)/fwknop/fwknopd.conf.inst; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst; \ fi - if test -f $(sysconfdir)/fwknop/access.conf; then \ - rm -f $(sysconfdir)/fwknop/access.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; \ fi - if test -f $(sysconfdir)/fwknop/access.conf.inst; then \ - rm -f $(sysconfdir)/fwknop/access.conf.inst; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst; \ fi install-exec-hook: - if test -d $(sysconfdir)/fwknop; then \ - chmod 700 $(sysconfdir)/fwknop; \ + if test -d $(DESTDIR)$(sysconfdir)/fwknop; then \ + chmod 700 $(DESTDIR)$(sysconfdir)/fwknop; \ fi - if test -f $(sysconfdir)/fwknop/fwknopd.conf; then :; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; then :; \ else \ - if test -f $(sysconfdir)/fwknop/fwknopd.conf.inst; then \ - mv $(sysconfdir)/fwknop/fwknopd.conf.inst $(sysconfdir)/fwknop/fwknopd.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst; then \ + mv $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; \ fi \ fi - if test -f $(sysconfdir)/fwknop/access.conf; then :; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; then :; \ else \ - if test -f $(sysconfdir)/fwknop/access.conf.inst; then \ - mv $(sysconfdir)/fwknop/access.conf.inst $(sysconfdir)/fwknop/access.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst; then \ + mv $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst $(DESTDIR)$(sysconfdir)/fwknop/access.conf; \ fi \ fi - if test -f $(sysconfdir)/fwknop/fwknopd.conf; then \ - chmod 600 $(sysconfdir)/fwknop/fwknopd.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; then \ + chmod 600 $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; \ fi - if test -f $(sysconfdir)/fwknop/access.conf; then \ - chmod 600 $(sysconfdir)/fwknop/access.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; then \ + chmod 600 $(DESTDIR)$(sysconfdir)/fwknop/access.conf; \ fi - diff --git a/TODO b/TODO deleted file mode 100644 index d73d2413..00000000 --- a/TODO +++ /dev/null @@ -1,41 +0,0 @@ -To whom it may concern, this is -*- outline -*- mode. - -* The library - libfko: -** C-based test suite. -** Get it out there and get feedback. - -* The fwknop C client: -** Config file support (~/.fwknoprc ?). -** Server Auth support (maybe). - -* The fwknopd server: -** Sniffer support to acquire SPA packet data ala the fwknopd Perl server: -** SPA packet decryption: -*** Add support for ipfw and pf support eventually. -** Test on embedded platforms - especially OpenWRT on a Linksys router. - -* Nice to haves: -** Binary packages: -*** Redhat RPMS -*** Debian .deb -** Linux/Unix platform: -*** A GNOME or KDE GUI app for the client. -** Windows platform: -*** VB and/or C# class wrappers around libfko.dll. -*** A Windows GUI app that uses the dll or wrapper classes. -** Misc: -*** Python module wrapping libfko. -*** Ruby module wrapping libfko. -*** PHP module wrapping libfko. - - -Copyright 2009-2010 - Damien Stuart - -This file is free software; as a special exception the author gives -unlimited permission to copy and/or distribute it, with or without -modifications, as long as this notice is preserved. - -This file is distributed in the hope that it will be useful, but -WITHOUT ANY WARRANTY, to the extent permitted by law; without even the -implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR -PURPOSE. diff --git a/client/config_init.c b/client/config_init.c index 933b7b62..5bd89557 100644 --- a/client/config_init.c +++ b/client/config_init.c @@ -109,6 +109,11 @@ parse_time_offset(const char *offset_str) if (isdigit(offset_str[i])) { offset_digits[j] = offset_str[i]; j++; + if(j >= MAX_TIME_STR_LEN) + { + fprintf(stderr, "Invalid time offset: %s", offset_str); + exit(EXIT_FAILURE); + } } else if (offset_str[i] == 'm' || offset_str[i] == 'M') { offset_type = TIME_OFFSET_MINUTES; break; @@ -145,9 +150,9 @@ parse_time_offset(const char *offset_str) static int create_fwknoprc(const char *rcfile) { - FILE *rc; + FILE *rc = NULL; - fprintf(stderr, "Creating initial rc file: %s.\n", rcfile); + fprintf(stdout, "[*] Creating initial rc file: %s.\n", rcfile); if ((rc = fopen(rcfile, "w")) == NULL) { @@ -209,7 +214,7 @@ create_fwknoprc(const char *rcfile) "# User-provided named stanzas:\n" "\n" "# Example for a destination server of 192.168.1.20 to open access to \n" - "# SSH for an IP that is resoved exteranlly, and one with a NAT request\n" + "# SSH for an IP that is resolved externally, and one with a NAT request\n" "# for a specific source IP that maps port 8088 on the server\n" "# to port 88 on 192.168.1.55 with timeout.\n" "#\n" @@ -231,6 +236,8 @@ create_fwknoprc(const char *rcfile) fclose(rc); + set_file_perms(rcfile); + return(0); } @@ -510,6 +517,13 @@ process_rc(fko_cli_options_t *options) strlcpy(rcfile, options->rc_file, MAX_PATH_LEN); } + /* Check rc file permissions - if anything other than user read/write, + * then don't process it. This change was made to help ensure that the + * client consumes a proper rc file with strict permissions set (thanks + * to Fernando Arnaboldi from IOActive for pointing this out). + */ + verify_file_perms_ownership(rcfile); + /* Open the rc file for reading, if it does not exist, then create * an initial .fwknoprc file with defaults and go on. */ diff --git a/client/fwknop.c b/client/fwknop.c index 0ada65d3..9c13f660 100644 --- a/client/fwknop.c +++ b/client/fwknop.c @@ -51,6 +51,8 @@ int resolve_ip_http(fko_cli_options_t *options); static void clean_exit(fko_ctx_t ctx, fko_cli_options_t *opts, unsigned int exit_status); +#define MAX_CMDLINE_ARGS 50 /* should be way more than enough */ + int main(int argc, char **argv) { @@ -640,6 +642,7 @@ show_last_command(void) args_save_file); exit(EXIT_FAILURE); } + verify_file_perms_ownership(args_save_file); if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL) { printf("Last fwknop client command line: %s", args_str); } else { @@ -665,7 +668,7 @@ run_last_args(fko_cli_options_t *options) char args_save_file[MAX_PATH_LEN] = {0}; char args_str[MAX_LINE_LEN] = {0}; char arg_tmp[MAX_LINE_LEN] = {0}; - char *argv_new[200]; /* should be way more than enough */ + char *argv_new[MAX_CMDLINE_ARGS]; /* should be way more than enough */ #ifdef WIN32 @@ -677,6 +680,8 @@ run_last_args(fko_cli_options_t *options) if (get_save_file(args_save_file)) { + verify_file_perms_ownership(args_save_file); + if ((args_file_ptr = fopen(args_save_file, "r")) == NULL) { fprintf(stderr, "Could not open args file: %s\n", @@ -701,12 +706,17 @@ run_last_args(fko_cli_options_t *options) argv_new[argc_new] = malloc(strlen(arg_tmp)+1); if (argv_new[argc_new] == NULL) { - fprintf(stderr, "malloc failure for cmd line arg.\n"); + fprintf(stderr, "[*] malloc failure for cmd line arg.\n"); exit(EXIT_FAILURE); } strlcpy(argv_new[argc_new], arg_tmp, strlen(arg_tmp)+1); current_arg_ctr = 0; argc_new++; + if(argc_new >= MAX_CMDLINE_ARGS) + { + fprintf(stderr, "[*] max command line args exceeded.\n"); + exit(EXIT_FAILURE); + } } } } @@ -739,7 +749,6 @@ save_args(int argc, char **argv) return; #endif - if (get_save_file(args_save_file)) { if ((args_file_ptr = fopen(args_save_file, "w")) == NULL) { fprintf(stderr, "Could not open args file: %s\n", @@ -758,6 +767,9 @@ save_args(int argc, char **argv) fprintf(args_file_ptr, "%s\n", args_str); fclose(args_file_ptr); } + + set_file_perms(args_save_file); + return; } diff --git a/client/http_resolve_host.c b/client/http_resolve_host.c index 4eaa5393..e2a61f8b 100644 --- a/client/http_resolve_host.c +++ b/client/http_resolve_host.c @@ -46,9 +46,9 @@ struct url { - char port[6]; - char host[256]; - char path[1024]; + char port[MAX_PORT_STR_LEN]; + char host[MAX_URL_HOST_LEN+1]; + char path[MAX_URL_PATH_LEN+1]; }; static int diff --git a/client/spa_comm.c b/client/spa_comm.c index 7d70ceee..53ba1a39 100644 --- a/client/spa_comm.c +++ b/client/spa_comm.c @@ -147,7 +147,7 @@ send_spa_packet_tcp_or_udp(const char *spa_data, const int sd_len, hints.ai_protocol = IPPROTO_TCP; } - sprintf(port_str, "%d", options->spa_dst_port); + snprintf(port_str, MAX_PORT_STR_LEN, "%d", options->spa_dst_port); error = getaddrinfo(options->spa_server_str, port_str, &hints, &result); diff --git a/client/utils.c b/client/utils.c index 61fa9a8e..ef60494d 100644 --- a/client/utils.c +++ b/client/utils.c @@ -89,4 +89,69 @@ is_base64(const unsigned char *buf, const unsigned short int len) return rv; } +int +set_file_perms(const char *file) +{ + int res = 0; + + res = chmod(file, S_IRUSR | S_IWUSR); + + if(res != 0) + { + fprintf(stderr, + "[-] unable to chmod file %s to user read/write (0600, -rw-------): %s\n", + file, + strerror(errno) + ); + } + return res; +} + +int +verify_file_perms_ownership(const char *file) +{ +#if HAVE_STAT + struct stat st; + + /* Every file that the fwknop client deals with should be owned + * by the user and permissions set to 600 (user read/write) + */ + if((stat(file, &st)) != 0) + { + fprintf(stderr, "[-] unable to run stat() against file: %s: %s\n", + file, strerror(errno)); + exit(EXIT_FAILURE); + } + + /* Make sure it is a regular file or symbolic link + */ + if(S_ISREG(st.st_mode) != 1 && S_ISLNK(st.st_mode) != 1) + { + fprintf(stderr, + "[-] file: %s is not a regular file or symbolic link.\n", + file + ); + return 0; + } + + if((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != (S_IRUSR|S_IWUSR)) + { + fprintf(stderr, + "[-] file: %s permissions should only be user read/write (0600, -rw-------)\n", + file + ); + return 0; + } + + if(st.st_uid != getuid()) + { + fprintf(stderr, "[-] file: %s not owned by current effective user id.\n", + file); + return 0; + } +#endif + + return 1; +} + /***EOF***/ diff --git a/client/utils.h b/client/utils.h index 8fc14737..19e8326d 100644 --- a/client/utils.h +++ b/client/utils.h @@ -31,11 +31,24 @@ #ifndef UTILS_H #define UTILS_H +#include +#include +#include +#include +#include +#include + +#if HAVE_CONFIG_H + #include "config.h" +#endif /* Prototypes */ void hex_dump(const unsigned char *data, const int size); int is_base64(const unsigned char *buf, const unsigned short int len); +int set_file_perms(const char *file); +int verify_file_perms_ownership(const char *file); + size_t strlcat(char *dst, const char *src, size_t siz); size_t strlcpy(char *dst, const char *src, size_t siz); diff --git a/common/common.h b/common/common.h index 0c1c26da..9ec43888 100644 --- a/common/common.h +++ b/common/common.h @@ -102,8 +102,6 @@ enum { #define DEFAULT_NAT_PORT 55000 #define MIN_HIGH_PORT 10000 /* sensible minimum for SPA dest port */ #define MAX_PORT 65535 -#define MAX_PORT_STR_LEN 6 -#define MAX_PROTO_STR_LEN 6 #define MAX_SERVER_STR_LEN 50 #define MAX_LINE_LEN 1024 diff --git a/configure.ac b/configure.ac index de9b443f..492a78d7 100644 --- a/configure.ac +++ b/configure.ac @@ -256,7 +256,7 @@ AC_FUNC_MALLOC AC_FUNC_REALLOC AC_FUNC_STAT -AC_CHECK_FUNCS([bzero gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn]) +AC_CHECK_FUNCS([bzero gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn strnlen stat chmod chown]) AC_SEARCH_LIBS([socket], [socket]) AC_SEARCH_LIBS([inet_addr], [nsl]) diff --git a/lib/fko_message.c b/lib/fko_message.c index 061bb738..3228dfad 100644 --- a/lib/fko_message.c +++ b/lib/fko_message.c @@ -200,6 +200,8 @@ validate_access_msg(const char *msg) do { ndx++; res = validate_proto_port_spec(ndx); + if(res != FKO_SUCCESS) + break; } while((ndx = strchr(ndx, ','))); return(res); @@ -208,14 +210,13 @@ validate_access_msg(const char *msg) int validate_proto_port_spec(const char *msg) { - int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE); + int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE), port_str_len = 0; const char *ndx = msg; if(startlen == MAX_SPA_MESSAGE_SIZE) return(FKO_ERROR_INVALID_DATA); - /* Now check for proto/port string. Currenly we only allow protos - * 'tcp', 'udp', and 'icmp'. + /* Now check for proto/port string. */ if(strncmp(ndx, "tcp", 3) && strncmp(ndx, "udp", 3) @@ -224,19 +225,25 @@ validate_proto_port_spec(const char *msg) return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); ndx = strchr(ndx, '/'); - if(ndx == NULL || (1+(ndx - msg)) >= startlen) + if(ndx == NULL || ((1+(ndx - msg)) > MAX_PROTO_STR_LEN)) return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); - /* Skip over the ',' and make sure we only have digits. + /* Skip over the '/' and make sure we only have digits. */ ndx++; - while(*ndx != '\0') + + /* Must have at least one digit for the port number + */ + if(isdigit(*ndx) == 0) + return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); + + while(*ndx != '\0' && *ndx != ',') { - if(isdigit(*ndx) == 0) + port_str_len++; + if((isdigit(*ndx) == 0) || (port_str_len > MAX_PORT_STR_LEN)) return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); ndx++; } - return(FKO_SUCCESS); } @@ -259,23 +266,31 @@ int got_allow_ip(const char *msg) { const char *ndx = msg; - int dot_cnt = 0; + int dot_ctr = 0, char_ctr = 0; int res = FKO_SUCCESS; while(*ndx != ',' && *ndx != '\0') { + char_ctr++; + if(char_ctr >= MAX_IPV4_STR_LEN) + { + res = FKO_ERROR_INVALID_ALLOW_IP; + break; + } if(*ndx == '.') - dot_cnt++; + dot_ctr++; else if(isdigit(*ndx) == 0) { res = FKO_ERROR_INVALID_ALLOW_IP; break; } - ndx++; } - if(dot_cnt != 3) + if (char_ctr < MIN_IPV4_STR_LEN) + res = FKO_ERROR_INVALID_ALLOW_IP; + + if(dot_ctr != 3) res = FKO_ERROR_INVALID_ALLOW_IP; return(res); diff --git a/lib/fko_message.h b/lib/fko_message.h index f56e33ff..538029d0 100644 --- a/lib/fko_message.h +++ b/lib/fko_message.h @@ -32,6 +32,9 @@ #ifndef FKO_MESSAGE_H #define FKO_MESSAGE_H 1 +#define MAX_PROTO_STR_LEN 5 /* tcp, udp, icmp for now */ +#define MAX_PORT_STR_LEN 6 + /* SPA message format validation functions. */ int validate_cmd_msg(const char *msg); diff --git a/server/access.c b/server/access.c index 8af966da..b62fff7b 100644 --- a/server/access.c +++ b/server/access.c @@ -213,7 +213,7 @@ add_acc_force_nat(fko_srv_options_t *opts, acc_stanza_t *curr_acc, const char *v /* Take an IP or Subnet/Mask and convert it to mask for later * comparisons of incoming source IPs against this mask. */ -static void +static int add_source_mask(fko_srv_options_t *opts, acc_stanza_t *acc, const char *ip) { char *ndx; @@ -278,7 +278,11 @@ add_source_mask(fko_srv_options_t *opts, acc_stanza_t *acc, const char *ip) log_msg(LOG_ERR, "Fatal error parsing IP to int for: %s", ip_str ); - clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); + + free(new_sle); + new_sle = NULL; + + return 0; } /* Store our mask converted from CIDR to a 32-bit value. @@ -290,15 +294,17 @@ add_source_mask(fko_srv_options_t *opts, acc_stanza_t *acc, const char *ip) */ new_sle->maddr = ntohl(in.s_addr) & new_sle->mask; } + return 1; } /* Expand the access SOURCE string to a list of masks. */ -void +static int expand_acc_source(fko_srv_options_t *opts, acc_stanza_t *acc) { char *ndx, *start; - char buf[32]; + char buf[ACCESS_BUF_LEN]; + int res = 1; start = acc->source; @@ -311,8 +317,15 @@ expand_acc_source(fko_srv_options_t *opts, acc_stanza_t *acc) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); - add_source_mask(opts, acc, buf); + + res = add_source_mask(opts, acc, buf); + if(res == 0) + return res; + start = ndx+1; } } @@ -322,15 +335,21 @@ expand_acc_source(fko_srv_options_t *opts, acc_stanza_t *acc) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); - add_source_mask(opts, acc, buf); + + res = add_source_mask(opts, acc, buf); + + return res; } static int parse_proto_and_port(char *pstr, int *proto, int *port) { char *ndx; - char proto_str[32]; + char proto_str[ACCESS_BUF_LEN]; /* Parse the string into its components. */ @@ -342,10 +361,24 @@ parse_proto_and_port(char *pstr, int *proto, int *port) return(-1); } - strlcpy(proto_str, pstr, (ndx - pstr)+1); + if(((ndx - pstr)+1) >= ACCESS_BUF_LEN) + { + log_msg(LOG_ERR, + "Parse error on access port entry: %s", pstr); + return(-1); + } + + strlcpy(proto_str, pstr, (ndx - pstr)+1); *port = atoi(ndx+1); + if((*port < 0) || (*port > MAX_PORT)) + { + log_msg(LOG_ERR, + "Invalid port in access request: %s", pstr); + return(-1); + } + if(strcasecmp(proto_str, "tcp") == 0) *proto = PROTO_TCP; else if(strcasecmp(proto_str, "udp") == 0) @@ -354,7 +387,6 @@ parse_proto_and_port(char *pstr, int *proto, int *port) { log_msg(LOG_ERR, "Invalid protocol in access port entry: %s", pstr); - return(-1); } @@ -457,15 +489,15 @@ add_string_list_ent(acc_string_list_t **stlist, const char *str_str) /* Expand a proto/port access string to a list of access proto-port struct. */ -void +int expand_acc_port_list(acc_port_list_t **plist, char *plist_str) { char *ndx, *start; - char buf[32]; + char buf[ACCESS_BUF_LEN]; start = plist_str; - for(ndx = start; *ndx; ndx++) + for(ndx = start; *ndx != '\0'; ndx++) { if(*ndx == ',') { @@ -474,6 +506,9 @@ expand_acc_port_list(acc_port_list_t **plist, char *plist_str) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(plist, buf); start = ndx+1; @@ -485,9 +520,14 @@ expand_acc_port_list(acc_port_list_t **plist, char *plist_str) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(plist, buf); + + return 1; } /* Expand a comma-separated string into a simple acc_string_list. @@ -652,7 +692,11 @@ expand_acc_ent_lists(fko_srv_options_t *opts) { /* Expand the source string to 32-bit integer masks foreach entry. */ - expand_acc_source(opts, acc); + if(expand_acc_source(opts, acc) == 0) + { + acc = acc->next; + continue; + } /* Now expand the open_ports string. */ @@ -782,8 +826,9 @@ set_acc_defaults(fko_srv_options_t *opts) static int acc_data_is_valid(const acc_stanza_t *acc) { - if(acc->key_len < 0 || ((acc->key == NULL && acc->key_base64 == NULL) - && (acc->gpg_decrypt_pw == NULL || !strlen(acc->gpg_decrypt_pw)))) + if(((acc->key == NULL || !strlen(acc->key)) + && (acc->gpg_decrypt_pw == NULL || !strlen(acc->gpg_decrypt_pw))) + || (acc->use_rijndael == 0 && acc->use_gpg == 0 && acc->gpg_allow_no_pw == 0)) { fprintf(stderr, "[*] No keys found for access stanza source: '%s'\n", acc->source @@ -824,6 +869,8 @@ parse_access_file(fko_srv_options_t *opts) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } + verify_file_perms_ownership(opts->config[CONF_ACCESS_FILE]); + if ((file_ptr = fopen(opts->config[CONF_ACCESS_FILE], "r")) == NULL) { fprintf(stderr, "[*] Could not open access file: %s\n", @@ -934,6 +981,7 @@ parse_access_file(fko_srv_options_t *opts) } add_acc_string(&(curr_acc->key), val); curr_acc->key_len = strlen(curr_acc->key); + add_acc_bool(&(curr_acc->use_rijndael), "Y"); } else if(CONF_VAR_IS(var, "KEY_BASE64")) { @@ -954,6 +1002,7 @@ parse_access_file(fko_srv_options_t *opts) add_acc_string(&(curr_acc->key_base64), val); add_acc_b64_string(&(curr_acc->key), &(curr_acc->key_len), curr_acc->key_base64); + add_acc_bool(&(curr_acc->use_rijndael), "Y"); } else if(CONF_VAR_IS(var, "HMAC_KEY_BASE64")) { @@ -1049,13 +1098,18 @@ parse_access_file(fko_srv_options_t *opts) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } add_acc_string(&(curr_acc->gpg_decrypt_pw), val); + add_acc_bool(&(curr_acc->use_gpg), "Y"); } else if(CONF_VAR_IS(var, "GPG_ALLOW_NO_PW")) { - if(curr_acc->gpg_decrypt_pw != NULL && curr_acc->gpg_decrypt_pw[0] != '\0') - free(curr_acc->gpg_decrypt_pw); - - add_acc_string(&(curr_acc->gpg_decrypt_pw), ""); + add_acc_bool(&(curr_acc->gpg_allow_no_pw), val); + if(curr_acc->gpg_allow_no_pw == 1) + { + add_acc_bool(&(curr_acc->use_gpg), "Y"); + if(curr_acc->gpg_decrypt_pw != NULL && curr_acc->gpg_decrypt_pw[0] != '\0') + free(curr_acc->gpg_decrypt_pw); + add_acc_string(&(curr_acc->gpg_decrypt_pw), ""); + } } else if(CONF_VAR_IS(var, "GPG_REQUIRE_SIG")) { @@ -1199,9 +1253,9 @@ compare_port_list(acc_port_list_t *in, acc_port_list_t *ac, const int match_any) int acc_check_port_access(acc_stanza_t *acc, char *port_str) { - int res = 1; + int res = 1, ctr = 0; - char buf[32]; + char buf[ACCESS_BUF_LEN]; char *ndx, *start; acc_port_list_t *o_pl = acc->oport_list; @@ -1214,14 +1268,34 @@ acc_check_port_access(acc_stanza_t *acc, char *port_str) /* Create our own internal port_list from the incoming SPA data * for comparison. */ - for(ndx = start; *ndx; ndx++) + for(ndx = start; *ndx != '\0'; ndx++) { if(*ndx == ',') { + if((ctr >= ACCESS_BUF_LEN) + || (((ndx-start)+1) >= ACCESS_BUF_LEN)) + { + log_msg(LOG_ERR, + "Unable to create acc_port_list from incoming data: %s", + port_str + ); + return(0); + } strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(&in_pl, buf); start = ndx+1; + ctr = 0; } + ctr++; + } + if((ctr >= ACCESS_BUF_LEN) + || (((ndx-start)+1) >= ACCESS_BUF_LEN)) + { + log_msg(LOG_ERR, + "Unable to create acc_port_list from incoming data: %s", + port_str + ); + return(0); } strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(&in_pl, buf); diff --git a/server/access.h b/server/access.h index b6501014..4ecae19d 100644 --- a/server/access.h +++ b/server/access.h @@ -34,6 +34,8 @@ #define PROTO_TCP 6 #define PROTO_UDP 17 +#define ACCESS_BUF_LEN 32 + /* Function Prototypes */ void parse_access_file(fko_srv_options_t *opts); @@ -41,7 +43,7 @@ int compare_addr_list(acc_int_list_t *source_list, const uint32_t ip); int acc_check_port_access(acc_stanza_t *acc, char *port_str); int acc_check_gpg_remote_id(acc_stanza_t *acc, const char *gpg_id); void dump_access_list(const fko_srv_options_t *opts); -void expand_acc_port_list(acc_port_list_t **plist, char *plist_str); +int expand_acc_port_list(acc_port_list_t **plist, char *plist_str); void free_acc_stanzas(fko_srv_options_t *opts); void free_acc_port_list(acc_port_list_t *plist); diff --git a/server/config_init.c b/server/config_init.c index 70c3e7c2..a728cdae 100644 --- a/server/config_init.c +++ b/server/config_init.c @@ -200,6 +200,8 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } + verify_file_perms_ownership(config_file); + if ((cfile_ptr = fopen(config_file, "r")) == NULL) { fprintf(stderr, "[*] Could not open config file: %s\n", diff --git a/server/fw_util_iptables.c b/server/fw_util_iptables.c index ad1648a5..e1b39888 100644 --- a/server/fw_util_iptables.c +++ b/server/fw_util_iptables.c @@ -582,7 +582,8 @@ process_spa_request(const fko_srv_options_t *opts, const acc_stanza_t *acc, spa_ /* Parse and expand our access message. */ - expand_acc_port_list(&port_list, spadat->spa_message_remain); + if(expand_acc_port_list(&port_list, spadat->spa_message_remain) != 1) + return res; /* Start at the top of the proto-port list... */ diff --git a/server/fwknopd.c b/server/fwknopd.c index 5a780b39..f66ef812 100644 --- a/server/fwknopd.c +++ b/server/fwknopd.c @@ -496,10 +496,22 @@ make_dir_path(const char *run_dir) if(stat(tmp_path, &st) != 0) { if(errno == ENOENT) + { res = mkdir(tmp_path, S_IRWXU); + if(res != 0) + return res; - if(res != 0) - return res; + /* run stat() against the component since we just + * created it + */ + if(stat(tmp_path, &st) != 0) + { + log_msg(LOG_ERR, + "Could not create component: %s of %s\n\n", tmp_path, run_dir + ); + return(ENOTDIR); + } + } } if(! S_ISDIR(st.st_mode)) @@ -664,10 +676,12 @@ get_running_pid(const fko_srv_options_t *opts) char buf[PID_BUFLEN] = {0}; pid_t rpid = 0; + op_fd = open(opts->config[CONF_FWKNOP_PID_FILE], O_RDONLY); if(op_fd > 0) { + verify_file_perms_ownership(opts->config[CONF_FWKNOP_PID_FILE]); if (read(op_fd, buf, PID_BUFLEN) > 0) { buf[PID_BUFLEN-1] = '\0'; diff --git a/server/fwknopd_common.h b/server/fwknopd_common.h index cda3b53f..118a971b 100644 --- a/server/fwknopd_common.h +++ b/server/fwknopd_common.h @@ -278,6 +278,7 @@ typedef struct acc_stanza char *hmac_key; int hmac_key_len; char *hmac_key_base64; + unsigned char use_rijndael; int fw_access_timeout; unsigned char enable_cmd_exec; char *cmd_exec_user; @@ -289,6 +290,8 @@ typedef struct acc_stanza char *gpg_decrypt_pw; unsigned char gpg_require_sig; unsigned char gpg_ignore_sig_error; + unsigned char use_gpg; + unsigned char gpg_allow_no_pw; char *gpg_remote_id; acc_string_list_t *gpg_remote_id_list; time_t access_expire_time; diff --git a/server/incoming_spa.c b/server/incoming_spa.c index 66532e34..eaaf54e5 100644 --- a/server/incoming_spa.c +++ b/server/incoming_spa.c @@ -363,7 +363,7 @@ incoming_spa(fko_srv_options_t *opts) */ enc_type = fko_encryption_type((char *)spa_pkt->packet_data); - if(enc_type == FKO_ENCRYPTION_RIJNDAEL) + if(acc->use_rijndael && enc_type == FKO_ENCRYPTION_RIJNDAEL) { if (acc->key == NULL) { @@ -379,12 +379,12 @@ incoming_spa(fko_srv_options_t *opts) acc->key, acc->key_len, acc->encryption_mode, acc->hmac_key, acc->hmac_key_len); } - else if(enc_type == FKO_ENCRYPTION_GPG) + else if(acc->use_gpg && enc_type == FKO_ENCRYPTION_GPG) { /* For GPG we create the new context without decrypting on the fly * so we can set some GPG parameters first. */ - if(acc->gpg_decrypt_pw != NULL) + if(acc->gpg_decrypt_pw != NULL || acc->gpg_allow_no_pw) { res = fko_new_with_data(&ctx, (char *)spa_pkt->packet_data, NULL, 0, acc->encryption_mode, NULL, 0); @@ -443,19 +443,11 @@ incoming_spa(fko_srv_options_t *opts) res = fko_decrypt_spa_data(ctx, acc->gpg_decrypt_pw, 0); } - else - { - log_msg(LOG_ERR, - "(stanza #%d) No GPG_DECRYPT_PW for GPG encrypted messages, set GPG_ALLOW_NO_PW", - stanza_num - ); - acc = acc->next; - continue; - } } else { - log_msg(LOG_ERR, "(stanza #%d) Unable to determing encryption type. Got type=%i.", + log_msg(LOG_ERR, + "(stanza #%d) No stanza encryption mode match for encryption type: %i.", stanza_num, enc_type); acc = acc->next; continue; diff --git a/server/replay_cache.c b/server/replay_cache.c index 2b164e94..227d64c6 100644 --- a/server/replay_cache.c +++ b/server/replay_cache.c @@ -261,10 +261,14 @@ replay_file_cache_init(fko_srv_options_t *opts) fprintf(digest_file_ptr, "#