From 010257c6a21c205df75f206c86a61194cb5f004f Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Sun, 21 Feb 2016 12:13:12 +0100 Subject: [PATCH] Simplified cookie handling This change set eliminates the need for cryptographically authenticated cookies and relies on sec-module providing accurate information on the SID provided by the client. --- doc/design.md | 50 +++++++++-- doc/sample.config | 6 -- src/Makefile.am | 4 +- src/common/common.c | 2 - src/config.c | 5 -- src/cookies.c | 178 ------------------------------------- src/cookies.h | 42 --------- src/ipc.proto | 36 +++----- src/main-auth.c | 69 ++------------ src/main-proc.c | 1 - src/main-sec-mod-cmd.c | 76 +++++++++------- src/main-user.c | 1 - src/main-worker-cmd.c | 1 - src/main.c | 13 --- src/main.h | 13 +-- src/ocserv-args.def | 6 -- src/sec-mod-auth.c | 71 ++++----------- src/sec-mod-sup-config.c | 1 - src/sec-mod.c | 44 +-------- src/sec-mod.h | 10 +-- src/vpn.h | 13 ++- src/worker-auth.c | 21 ++--- src/worker-http-handlers.c | 1 - src/worker-http.c | 17 ++-- src/worker-misc.c | 1 - src/worker-resume.c | 1 - src/worker-vpn.c | 1 - src/worker.h | 4 +- 28 files changed, 161 insertions(+), 527 deletions(-) delete mode 100644 src/cookies.c delete mode 100644 src/cookies.h diff --git a/doc/design.md b/doc/design.md index 98c93eab..c6c60477 100644 --- a/doc/design.md +++ b/doc/design.md @@ -63,12 +63,13 @@ leaked during a fork(). It handles: data to the radius accounting server. See the SM_CMD_CLI_STATS message handling. - * Gatekeeper for new user sessions. When the main process receives a valid cookie - from a worker process, it will notify the security module which keeps the - authentication state. The security module will return any additional user - configuration settings (received via radius or per-user config file) - - See SM_CMD_AUTH_SESSION_OPEN and SM_CMD_AUTH_SESSION_CLOSE message - handling. + * Gatekeeper for new user sessions. The security module assigns a session + ID (SID) to all connecting users. When the main process receives a request + to resume a session with a SID from a worker process, it will notify the + security module which keeps the authentication state. The security module + will return any additional user configuration settings (received via radius + or per-user config file) - See SM_CMD_AUTH_SESSION_OPEN and SM_CMD_AUTH_SESSION_CLOSE + message handling. Currently it seems we require quite an amount of communication between the main process and the security module. That may affect scaling. If that @@ -142,3 +143,40 @@ device and the client. The tasks handled are: ``` +## IPC Communication for SID assignment + +This is the same diagram as above but shows how the session ID (SID) +is assigned and used throughout the server. + +``` + main sec-mod worker + | | | + | | <--SEC_AUTH_INIT--- | + | | -SEC_AUTH_REP (NEW SID)-> | + | | <--SEC_AUTH_CONT (SID)--- | + | | . | + | | . | + | | . | + | | ----SEC_AUTH_REP -------> | + +(note that by that time the client/worker may be disconnected, +and reconnect later and use the cookie -SID- to resume the +already authenticated session). + + | | | + | <----------AUTH_COOKIE_REQ (SID)----------------- | + | | | + | -SESSION_OPEN (SID)-> | | + | <--SESSION_REPLY---- | | #contains additional config for client + | | | + | -----------------AUTH_REP-----------------------> | #forwards the additional config for client + | | | + | <------------SESSION_INFO------------------------ | + | | | + | | <-- CLI_STATS (SID)------- | + | | (disconnect) + | -SESSION_CLOSE(SID)-> | + | <-- CLI_STATS (SID)-- | + +``` + diff --git a/doc/sample.config b/doc/sample.config index aa7f5a37..9eaf970b 100644 --- a/doc/sample.config +++ b/doc/sample.config @@ -308,12 +308,6 @@ ban-reset-time = 300 # between different networks. cookie-timeout = 300 -# Cookie rekey time (in seconds) -# The time after which the key used to encrypt cookies will be -# refreshed. After this time the previous key will also be valid -# for verification until the next rotation cycle. -cookie-rekey-time = 259200 - # If this is enabled (not recommended) the cookies will stay # valid even after a user manually disconnects, and until they # expire. This may improve roaming with some broken clients. diff --git a/src/Makefile.am b/src/Makefile.am index fabb0a7d..f54a41bc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -48,8 +48,8 @@ AUTH_SOURCES=auth/pam.c auth/pam.h auth/plain.c auth/plain.h auth/radius.c auth/ ACCT_SOURCES=acct/radius.c acct/radius.h acct/pam.c acct/pam.h ocserv_SOURCES = main.c main-auth.c worker-vpn.c worker-auth.c tlslib.c \ - cookies.c main-worker-cmd.c ip-lease.c ip-lease.h main-proc.c \ - vpn.h cookies.h tlslib.h log.c tun.c tun.h config-kkdcp.c \ + main-worker-cmd.c ip-lease.c ip-lease.h main-proc.c \ + vpn.h tlslib.h log.c tun.c tun.h config-kkdcp.c \ config.c worker-resume.c worker.h sec-mod-resume.c main.h \ worker-http-handlers.c html.c html.h worker-http.c \ main-user.c worker-misc.c route-add.c route-add.h worker-privs.c \ diff --git a/src/common/common.c b/src/common/common.c index 764773ec..ae04261c 100644 --- a/src/common/common.c +++ b/src/common/common.c @@ -87,8 +87,6 @@ const char *cmd_request_to_str(unsigned _cmd) return "sm: ban IP"; case SM_CMD_AUTH_BAN_IP_REPLY: return "sm: ban IP reply"; - case SM_CMD_REFRESH_COOKIE_KEY: - return "sm: refresh cookie key"; default: snprintf(tmp, sizeof(tmp), "unknown (%u)", _cmd); return tmp; diff --git a/src/config.c b/src/config.c index 42c83c93..e2062eb2 100644 --- a/src/config.c +++ b/src/config.c @@ -49,7 +49,6 @@ #include #include -#include #include #include #include @@ -910,10 +909,6 @@ size_t urlfw_size = 0; config->cookie_timeout = DEFAULT_COOKIE_RECON_TIMEOUT; READ_TF("persistent-cookies", config->persistent_cookies, 0); - READ_NUMERIC("cookie-rekey-time", config->cookie_rekey_time); - if (config->cookie_rekey_time == 0) - config->cookie_rekey_time = DEFAULT_COOKIE_REKEY_TIME; - READ_NUMERIC("session-timeout", config->session_timeout); READ_NUMERIC("auth-timeout", config->auth_timeout); diff --git a/src/cookies.c b/src/cookies.c deleted file mode 100644 index cef622af..00000000 --- a/src/cookies.c +++ /dev/null @@ -1,178 +0,0 @@ -/* - * Copyright (C) 2013, 2014 Nikos Mavrogiannopoulos - * Copyright (C) 2014 Red Hat - * - * 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 2 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 this program. If not, see . - */ - -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include - -int decrypt_cookie(ProtobufCAllocator *pa, gnutls_datum_t *key, - uint8_t* cookie, unsigned cookie_size, - Cookie **msg) -{ -gnutls_datum_t iv = { (void*)cookie, COOKIE_IV_SIZE }; -int ret; -uint8_t tag[COOKIE_MAC_SIZE]; -gnutls_cipher_hd_t h; -uint8_t *p, *decrypted = NULL; -unsigned p_size; - - if (cookie_size <= COOKIE_IV_SIZE+COOKIE_MAC_SIZE) - return -1; - - ret = gnutls_cipher_init(&h, GNUTLS_CIPHER_AES_128_GCM, key, &iv); - if (ret < 0) - return -1; - - decrypted = talloc_size(pa->allocator_data, cookie_size); - if (decrypted == NULL) { - ret = -1; - goto cleanup; - } - - cookie += COOKIE_IV_SIZE; - cookie_size -= (COOKIE_IV_SIZE + COOKIE_MAC_SIZE); - - ret = gnutls_cipher_decrypt2(h, cookie, cookie_size, decrypted, cookie_size); - if (ret < 0) { - ret = -1; - goto cleanup; - } - - p = decrypted; - p_size = cookie_size; - - ret = gnutls_cipher_tag(h, tag, sizeof(tag)); - if (ret < 0) { - ret = -1; - goto cleanup; - } - - if (memcmp(tag, cookie+cookie_size, COOKIE_MAC_SIZE) != 0) { - ret = -1; - goto cleanup; - } - - /* unpack */ - *msg = cookie__unpack(pa, p_size, p); - if (*msg == NULL) { - ret = -1; - goto cleanup; - } - - ret = 0; - -cleanup: - gnutls_cipher_deinit(h); - talloc_free(decrypted); - - return ret; -} - -int encrypt_cookie(void *pool, gnutls_datum_t *key, const Cookie *msg, - uint8_t** ecookie, unsigned *ecookie_size) -{ -uint8_t _iv[COOKIE_IV_SIZE]; -gnutls_cipher_hd_t h = NULL; -gnutls_datum_t iv = { _iv, sizeof(_iv) }; -int ret; -unsigned packed_size, e_size; -uint8_t *packed = NULL, *e; - - /* pack the cookie */ - packed_size = cookie__get_packed_size(msg); - if (packed_size == 0) - return -1; - - packed = talloc_size(pool, packed_size); - if (packed == NULL) - return -1; - - ret = cookie__pack(msg, packed); - if (ret == 0) { - ret = -1; - goto cleanup; - } - - ret = gnutls_rnd(GNUTLS_RND_NONCE, _iv, sizeof(_iv)); - if (ret < 0) { - ret = -1; - goto cleanup; - } - - ret = gnutls_cipher_init(&h, GNUTLS_CIPHER_AES_128_GCM, key, &iv); - if (ret < 0) { - ret = -1; - goto cleanup; - } - - e_size = packed_size+COOKIE_IV_SIZE+COOKIE_MAC_SIZE; - e = talloc_size(pool, e_size); - if (e == NULL) { - ret = -1; - goto cleanup; - } - - *ecookie = e; - *ecookie_size = e_size; - - memcpy(e, _iv, COOKIE_IV_SIZE); - e += COOKIE_IV_SIZE; - e_size -= COOKIE_IV_SIZE; - - ret = gnutls_cipher_encrypt2(h, packed, packed_size, e, e_size); - if (ret < 0) { - ret = -1; - goto cleanup; - } - - e += packed_size; - - ret = gnutls_cipher_tag(h, e, COOKIE_MAC_SIZE); - if (ret < 0) { - ret = -1; - goto cleanup; - } - - ret = 0; - -cleanup: - talloc_free(packed); - if (h != NULL) - gnutls_cipher_deinit(h); - return ret; - -} - diff --git a/src/cookies.h b/src/cookies.h deleted file mode 100644 index 0d5f2f98..00000000 --- a/src/cookies.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright (C) 2013 Nikos Mavrogiannopoulos - * - * Author: Nikos Mavrogiannopoulos - * - * This file is part of ocserv. - * - * The GnuTLS is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public License - * as published by the Free Software Foundation; either version 2.1 of - * the License, or (at your option) any later version. - * - * This library 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 - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program. If not, see - */ -#ifndef COOKIES_H -#define COOKIES_H - -#include -#include -#include - -#define COOKIE_IV_SIZE 12 /* AES-GCM */ -#define COOKIE_MAC_SIZE 12 /* 96-bits of AES-GCM */ - -/* The time after a disconnection the cookie is valid */ -#define DEFAULT_COOKIE_RECON_TIMEOUT 120 -/* The time after a cookie key is rotated */ -#define DEFAULT_COOKIE_REKEY_TIME 259200 - -int encrypt_cookie(void *pool, gnutls_datum_t *key, const Cookie *msg, - uint8_t** ecookie, unsigned *ecookie_size); -int decrypt_cookie(ProtobufCAllocator *pa, gnutls_datum_t *key, - uint8_t *cookie, unsigned cookie_size, - Cookie **msg); - -#endif diff --git a/src/ipc.proto b/src/ipc.proto index 282b3d65..1665f936 100644 --- a/src/ipc.proto +++ b/src/ipc.proto @@ -220,11 +220,10 @@ message sec_auth_cont_msg message sec_auth_reply_msg { required AUTH_REP reply = 1; - optional bytes cookie = 2; optional string user_name = 3; optional string msg = 4; /* message to display to user */ optional bytes dtls_session_id = 5; - optional bytes sid = 6; + optional bytes sid = 6; /* cookie */ optional uint32 passwd_counter = 8; /* if that's a password prompt indicates the number of password asked */ } @@ -235,19 +234,6 @@ message sec_op_msg required bytes data = 2; } -/* Not a real message, but the cookie */ -message cookie -{ - required string username = 1; - required string groupname = 2; - required string hostname = 3; - required string ip = 4; - required uint32 expiration = 6; - required uint32 ipv4_seed = 7; - required bytes sid = 8; - required bool tls_auth_ok = 9; -} - /* * == Session Termination == * @@ -259,9 +245,8 @@ message cookie /* SEC_SESSION_CLOSE */ message sec_auth_session_msg { - required bytes sid = 1; - /* on open */ - optional bytes cookie = 2; + /* on open/close */ + required bytes sid = 1; /* cookie */ /* on close */ optional uint32 uptime = 3; optional uint64 bytes_in = 4; @@ -270,16 +255,19 @@ message sec_auth_session_msg optional string ipv6 = 7; } - message sec_auth_session_reply_msg { required AUTH_REP reply = 1; - required group_cfg_st config = 2; -} -message sec_refresh_cookie_key -{ - required bytes key = 1; + required group_cfg_st config = 2; + + required string username = 3; + required string groupname = 4; + required string hostname = 5; + required string ip = 6; + required uint32 ipv4_seed = 8; + required bytes sid = 9; + required bool tls_auth_ok = 10; } /* SEC_BAN_IP: sent from sec-mod to main */ diff --git a/src/main-auth.c b/src/main-auth.c index c7ca0877..f24dff53 100644 --- a/src/main-auth.c +++ b/src/main-auth.c @@ -40,7 +40,6 @@ #include "str.h" #include -#include #include #include #include @@ -167,37 +166,9 @@ int handle_auth_cookie_req(main_server_st* s, struct proc_st* proc, const AuthCookieRequestMsg * req) { int ret; -Cookie *cmsg; -gnutls_datum_t key = {s->cookie_key, sizeof(s->cookie_key)}; -char str_ip[MAX_IP_STR+1]; -PROTOBUF_ALLOCATOR(pa, proc); struct proc_st *old_proc; - if (req->cookie.len == 0) { - mslog(s, proc, LOG_INFO, "error in cookie size"); - return -1; - } - - ret = decrypt_cookie(&pa, &key, req->cookie.data, req->cookie.len, &cmsg); - if (ret < 0 && s->prev_cookie_key_active) { - /* try the old key */ - key.data = s->prev_cookie_key; - key.size = sizeof(s->prev_cookie_key); - ret = decrypt_cookie(&pa, &key, req->cookie.data, req->cookie.len, &cmsg); - if (ret == 0) - mslog(s, proc, LOG_INFO, "decrypted cookie with previous key"); - } - - if (ret < 0) { - mslog(s, proc, LOG_INFO, "error decrypting cookie"); - return -1; - } - - if (cmsg->username == NULL) - return -1; - strlcpy(proc->username, cmsg->username, sizeof(proc->username)); - - if (cmsg->sid.len != sizeof(proc->sid)) + if (req->cookie.data == NULL || req->cookie.len != sizeof(proc->sid)) return -1; /* generate a new DTLS session ID for each connection, to allow @@ -207,46 +178,20 @@ struct proc_st *old_proc; return -1; proc->dtls_session_id_size = sizeof(proc->dtls_session_id); - memcpy(proc->sid, cmsg->sid.data, cmsg->sid.len); - - /* override the group name in order to load the correct configuration in - * case his group is specified in the certificate */ - if (cmsg->groupname) - strlcpy(proc->groupname, cmsg->groupname, sizeof(proc->groupname)); - - /* loads sup config */ + /* loads sup config and basic proc info (e.g., username) */ ret = session_open(s, proc, req->cookie.data, req->cookie.len); if (ret < 0) { mslog(s, proc, LOG_INFO, "could not open session"); return -1; } - /* this hints to call session_close() */ - proc->active_sid = 1; /* Put into right cgroup */ if (proc->config->cgroup != NULL) { put_into_cgroup(s, proc->config->cgroup, proc->pid); } - /* check whether the cookie IP matches */ - if (proc->config->deny_roaming != 0) { - if (cmsg->ip == NULL) { - return -1; - } - - if (human_addr2((struct sockaddr *)&proc->remote_addr, proc->remote_addr_len, - str_ip, sizeof(str_ip), 0) == NULL) - return -1; - - if (strcmp(str_ip, cmsg->ip) != 0) { - mslog(s, proc, LOG_INFO, "user '%s' is re-using cookie from different IP (prev: %s, current: %s); rejecting", - cmsg->username, cmsg->ip, str_ip); - return -1; - } - } - /* check for a user with the same sid as in the cookie */ - old_proc = proc_search_sid(s, cmsg->sid.data); + old_proc = proc_search_sid(s, req->cookie.data); if (old_proc != NULL) { mslog(s, old_proc, LOG_DEBUG, "disconnecting previous user session (%u) due to session re-use", (unsigned)old_proc->pid); @@ -267,10 +212,10 @@ struct proc_st *old_proc; mslog(s, proc, LOG_INFO, "new user session"); } - if (cmsg->hostname) - strlcpy(proc->hostname, cmsg->hostname, sizeof(proc->hostname)); - - memcpy(proc->ipv4_seed, &cmsg->ipv4_seed, sizeof(proc->ipv4_seed)); + /* update the SID */ + memcpy(proc->sid, req->cookie.data, req->cookie.len); + /* this also hints to call session_close() */ + proc->active_sid = 1; /* add the links to proc hash */ if (proc_table_add(s, proc) < 0) { diff --git a/src/main-proc.c b/src/main-proc.c index 88051dd8..7fe8bf76 100644 --- a/src/main-proc.c +++ b/src/main-proc.c @@ -55,7 +55,6 @@ #endif #include -#include #include #include #include diff --git a/src/main-sec-mod-cmd.c b/src/main-sec-mod-cmd.c index 796ee13b..fa8cc94b 100644 --- a/src/main-sec-mod-cmd.c +++ b/src/main-sec-mod-cmd.c @@ -65,7 +65,6 @@ int handle_sec_mod_commands(main_server_st * s) void *pool = talloc_new(s); PROTOBUF_ALLOCATOR(pa, pool); BanIpMsg *tmsg = NULL; - SecRefreshCookieKey *rmsg = NULL; if (pool == NULL) return -1; @@ -122,29 +121,6 @@ int handle_sec_mod_commands(main_server_st * s) } switch (cmd) { - case SM_CMD_REFRESH_COOKIE_KEY: - rmsg = sec_refresh_cookie_key__unpack(&pa, raw_len, raw); - if (rmsg == NULL) { - mslog(s, NULL, LOG_ERR, "error unpacking sec-mod data"); - ret = ERR_BAD_COMMAND; - goto cleanup; - } - - if (rmsg->key.len != sizeof(s->cookie_key)) { - mslog(s, NULL, LOG_ERR, "received corrupt cookie key (%u bytes) from sec-mod", (unsigned)rmsg->key.len); - ret = ERR_BAD_COMMAND; - goto cleanup; - } - - memcpy(s->prev_cookie_key, s->cookie_key, sizeof(s->cookie_key)); - s->prev_cookie_key_active = 1; - - memcpy(s->cookie_key, rmsg->key.data, sizeof(s->cookie_key)); - safe_memset(rmsg->key.data, 0, rmsg->key.len); - - mslog(s, NULL, LOG_INFO, "refreshed cookie key"); - break; - case SM_CMD_AUTH_BAN_IP:{ BanIpReplyMsg reply = BAN_IP_REPLY_MSG__INIT; @@ -430,6 +406,10 @@ int session_open(main_server_st * s, struct proc_st *proc, const uint8_t *cookie SecAuthSessionReplyMsg *msg = NULL; char str_ipv4[MAX_IP_STR]; char str_ipv6[MAX_IP_STR]; + char str_ip[MAX_IP_STR]; + + if (cookie == NULL || cookie_size != SID_SIZE) + return -1; ireq.uptime = time(0)-proc->conn_time; ireq.has_uptime = 1; @@ -437,8 +417,8 @@ int session_open(main_server_st * s, struct proc_st *proc, const uint8_t *cookie ireq.has_bytes_in = 1; ireq.bytes_out = proc->bytes_out; ireq.has_bytes_out = 1; - ireq.sid.data = proc->sid; - ireq.sid.len = sizeof(proc->sid); + ireq.sid.data = (void*)cookie; + ireq.sid.len = cookie_size; if (proc->ipv4 && human_addr2((struct sockaddr *)&proc->ipv4->rip, proc->ipv4->rip_len, @@ -452,12 +432,6 @@ int session_open(main_server_st * s, struct proc_st *proc, const uint8_t *cookie ireq.ipv6 = str_ipv6; } - if (cookie) { - ireq.cookie.data = (void*)cookie; - ireq.cookie.len = cookie_size; - ireq.has_cookie = 1; - } - mslog(s, proc, LOG_DEBUG, "sending msg %s to sec-mod", cmd_request_to_str(SM_CMD_AUTH_SESSION_OPEN)); ret = send_msg16(proc, s->sec_mod_fd_sync, SM_CMD_AUTH_SESSION_OPEN, @@ -478,19 +452,53 @@ int session_open(main_server_st * s, struct proc_st *proc, const uint8_t *cookie } if (msg->reply != AUTH__REP__OK) { - mslog(s, proc, LOG_INFO, "could not initiate session for '%s'", proc->username); + mslog(s, proc, LOG_INFO, "could not initiate session"); return -1; } + if (msg->username == NULL) { + mslog(s, proc, LOG_INFO, "no username present in session reply"); + return -1; + } + strlcpy(proc->username, msg->username, sizeof(proc->username)); + + if (msg->hostname) + strlcpy(proc->hostname, msg->hostname, sizeof(proc->hostname)); + + + /* override the group name in order to load the correct configuration in + * case his group is specified in the certificate */ + if (msg->groupname) + strlcpy(proc->groupname, msg->groupname, sizeof(proc->groupname)); + if (msg->config == NULL) { mslog(s, proc, LOG_INFO, "received invalid configuration for '%s'; could not initiate session", proc->username); return -1; } + memcpy(proc->ipv4_seed, &msg->ipv4_seed, sizeof(proc->ipv4_seed)); + proc->config = msg->config; apply_default_config(s, proc, proc->config); + /* check whether the cookie IP matches */ + if (proc->config && proc->config->deny_roaming != 0) { + if (msg->ip == NULL) { + return -1; + } + + if (human_addr2((struct sockaddr *)&proc->remote_addr, proc->remote_addr_len, + str_ip, sizeof(str_ip), 0) == NULL) + return -1; + + if (strcmp(str_ip, msg->ip) != 0) { + mslog(s, proc, LOG_INFO, "user '%s' is re-using cookie from different IP (prev: %s, current: %s); rejecting", + proc->username, msg->ip, str_ip); + return -1; + } + } + return 0; } @@ -593,7 +601,7 @@ int run_sec_mod(main_server_st * s, int *sync_fd) close(sfd[1]); set_cloexec_flag (fd[0], 1); set_cloexec_flag (sfd[0], 1); - sec_mod_server(s->main_pool, s->perm_config, p, s->cookie_key, fd[0], sfd[0]); + sec_mod_server(s->main_pool, s->perm_config, p, fd[0], sfd[0]); exit(0); } else if (pid > 0) { /* parent */ close(fd[0]); diff --git a/src/main-user.c b/src/main-user.c index dec84012..7e304a3f 100644 --- a/src/main-user.c +++ b/src/main-user.c @@ -40,7 +40,6 @@ #include #include -#include #include #include #include diff --git a/src/main-worker-cmd.c b/src/main-worker-cmd.c index c90a2503..b105d9da 100644 --- a/src/main-worker-cmd.c +++ b/src/main-worker-cmd.c @@ -55,7 +55,6 @@ #endif #include -#include #include #include #include diff --git a/src/main.c b/src/main.c index c69f6a69..96bf5cc0 100644 --- a/src/main.c +++ b/src/main.c @@ -57,7 +57,6 @@ #include #include #include -#include #include #include #include @@ -1044,10 +1043,6 @@ static void listen_watcher_cb (EV_P_ ev_io *w, int revents) close(s->sec_mod_fd); close(s->sec_mod_fd_sync); - /* clear the cookie key */ - safe_memset(s->cookie_key, 0, sizeof(s->cookie_key)); - safe_memset(s->prev_cookie_key, 0, sizeof(s->prev_cookie_key)); - setproctitle(PACKAGE_NAME"-worker"); kill_on_parent_kill(SIGTERM); @@ -1207,14 +1202,6 @@ int main(int argc, char** argv) /* Initialize GnuTLS */ tls_global_init(&creds); - /* this is the key used to sign and verify cookies. It is used - * by sec-mod (for signing) and main (for verification). */ - ret = gnutls_rnd(GNUTLS_RND_RANDOM, s->cookie_key, sizeof(s->cookie_key)); - if (ret < 0) { - fprintf(stderr, "Error in cookie key generation\n"); - exit(1); - } - /* load configuration */ ret = cmd_parser(main_pool, argc, argv, &s->perm_config); if (ret < 0) { diff --git a/src/main.h b/src/main.h index 7feebc08..861be31d 100644 --- a/src/main.h +++ b/src/main.h @@ -49,13 +49,7 @@ extern ev_timer maintainance_watcher; #define MAIN_MAINTAINANCE_TIME (900) -extern sigset_t sig_default_set; int cmd_parser (void *pool, int argc, char **argv, struct perm_cfg_st** config); -void reload_cfg_file(void *pool, struct perm_cfg_st* config, unsigned archive); -void clear_old_configs(struct perm_cfg_st* config); -void clear_cfg(struct perm_cfg_st* config); -void write_pid_file(void); -void remove_pid_file(void); struct listener_st { ev_io io; @@ -118,7 +112,7 @@ typedef struct proc_st { struct sockaddr_storage our_addr; /* our address */ socklen_t our_addr_len; - /* The SID present in the cookie. Used for session control only */ + /* The SID which acts as a cookie */ uint8_t sid[SID_SIZE]; unsigned active_sid; @@ -193,11 +187,6 @@ typedef struct main_server_st { tls_st *creds; - uint8_t cookie_key[COOKIE_KEY_SIZE]; - /* when we rotate keys, this is the previous one active for verification */ - uint8_t prev_cookie_key[COOKIE_KEY_SIZE]; - unsigned prev_cookie_key_active; - struct listen_list_st listen_list; struct proc_list_st proc_list; struct script_list_st script_list; diff --git a/src/ocserv-args.def b/src/ocserv-args.def index 3b8c15a5..c57f7fcd 100644 --- a/src/ocserv-args.def +++ b/src/ocserv-args.def @@ -402,12 +402,6 @@ ban-reset-time = 300 # between different networks. cookie-timeout = 300 -# Cookie rekey time (in seconds) -# The time after which the key used to encrypt cookies will be -# refreshed. After this time the previous key will also be valid -# for verification until the next rotation cycle. -cookie-rekey-time = 259200 - # If this is enabled (not recommended) the cookies will stay # valid even after a user manually disconnects, and until they # expire. This may improve roaming with some broken clients. diff --git a/src/sec-mod-auth.c b/src/sec-mod-auth.c index 65d0facd..cc92c7dd 100644 --- a/src/sec-mod-auth.c +++ b/src/sec-mod-auth.c @@ -39,7 +39,6 @@ #include "str.h" #include -#include #include #include #include @@ -109,42 +108,6 @@ void sec_mod_add_score_to_ip(sec_mod_st *sec, client_entry_st *e, const char *ip return; } -static int generate_cookie(sec_mod_st * sec, client_entry_st * entry) -{ - int ret; - Cookie msg = COOKIE__INIT; - - msg.username = entry->acct_info.username; - msg.groupname = entry->acct_info.groupname; - msg.hostname = entry->hostname; - msg.ip = entry->acct_info.remote_ip; - msg.tls_auth_ok = entry->tls_auth_ok; - - /* Fixme: possibly we should allow for completely random seeds */ - if (sec->config->predictable_ips != 0) { - msg.ipv4_seed = hash_any(entry->acct_info.username, strlen(entry->acct_info.username), 0); - } else { - ret = gnutls_rnd(GNUTLS_RND_NONCE, &msg.ipv4_seed, sizeof(msg.ipv4_seed)); - if (ret < 0) - return -1; - } - - msg.sid.data = entry->sid; - msg.sid.len = sizeof(entry->sid); - - /* this is the time when this cookie must be activated (used to authenticate). - * if not activated by that time it expires */ - msg.expiration = time(0) + sec->config->cookie_timeout; - - ret = - encrypt_cookie(entry, &sec->dcookie_key, &msg, &entry->cookie, - &entry->cookie_size); - if (ret < 0) - return -1; - - return 0; -} - static int send_sec_auth_reply(int cfd, sec_mod_st * sec, client_entry_st * entry, AUTHREP r) { @@ -153,16 +116,7 @@ int send_sec_auth_reply(int cfd, sec_mod_st * sec, client_entry_st * entry, AUTH if (r == AUTH__REP__OK) { /* fill message */ - ret = generate_cookie(sec, entry); - if (ret < 0) { - seclog(sec, LOG_INFO, "cannot generate cookie"); - return ret; - } - msg.reply = AUTH__REP__OK; - msg.has_cookie = 1; - msg.cookie.data = entry->cookie; - msg.cookie.len = entry->cookie_size; msg.user_name = entry->acct_info.username; @@ -437,13 +391,6 @@ int handle_sec_auth_session_open(sec_mod_st *sec, int fd, const SecAuthSessionMs return send_failed_session_open_reply(sec, fd); } - if (req->has_cookie == 0 || (req->cookie.len != e->cookie_size) || - memcmp(req->cookie.data, e->cookie, e->cookie_size) != 0) { - seclog(sec, LOG_ERR, "cookie error; denied session for user '%s' "SESSION_STR, e->acct_info.username, e->acct_info.psid); - e->status = PS_AUTH_FAILED; - return send_failed_session_open_reply(sec, fd); - } - if (req->ipv4) strlcpy(e->acct_info.ipv4, req->ipv4, sizeof(e->acct_info.ipv4)); if (req->ipv6) @@ -460,6 +407,24 @@ int handle_sec_auth_session_open(sec_mod_st *sec, int fd, const SecAuthSessionMs } } + rep.username = e->acct_info.username; + rep.groupname = e->acct_info.groupname; + rep.hostname = e->hostname; + rep.ip = e->acct_info.remote_ip; + rep.tls_auth_ok = e->tls_auth_ok; + + /* Fixme: possibly we should allow for completely random seeds */ + if (sec->config->predictable_ips != 0) { + rep.ipv4_seed = hash_any(e->acct_info.username, strlen(e->acct_info.username), 0); + } else { + ret = gnutls_rnd(GNUTLS_RND_NONCE, &rep.ipv4_seed, sizeof(rep.ipv4_seed)); + if (ret < 0) + return -1; + } + + rep.sid.data = e->sid; + rep.sid.len = sizeof(e->sid); + rep.reply = AUTH__REP__OK; lpool = talloc_new(e); diff --git a/src/sec-mod-sup-config.c b/src/sec-mod-sup-config.c index 4ffde81f..9b2ad933 100644 --- a/src/sec-mod-sup-config.c +++ b/src/sec-mod-sup-config.c @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include diff --git a/src/sec-mod.c b/src/sec-mod.c index b7094417..9360caca 100644 --- a/src/sec-mod.c +++ b/src/sec-mod.c @@ -180,24 +180,6 @@ int load_pins(struct perm_cfg_st *config, struct pin_st *s) return 0; } -static int send_refresh_cookie_key(sec_mod_st * sec, void *key_data, unsigned key_size) -{ - SecRefreshCookieKey msg = SEC_REFRESH_COOKIE_KEY__INIT; - int ret; - - msg.key.data = key_data; - msg.key.len = key_size; - - ret = send_msg16(sec, sec->cmd_fd, SM_CMD_REFRESH_COOKIE_KEY, &msg, - (pack_size_func) sec_refresh_cookie_key__get_packed_size, - (pack_func) sec_refresh_cookie_key__pack); - if (ret < 0) { - seclog(sec, LOG_WARNING, "sec-mod error in sending cookie key"); - } - - return 0; -} - static int handle_op(void *pool, int cfd, sec_mod_st * sec, uint8_t type, uint8_t * rep, size_t rep_size) { @@ -501,8 +483,6 @@ static void handle_sigterm(int signo) static void check_other_work(sec_mod_st *sec) { - time_t now = time(0); - if (need_exit) { unsigned i; @@ -516,23 +496,6 @@ static void check_other_work(sec_mod_st *sec) exit(0); } - if (sec->config->cookie_rekey_time > 0 && now - sec->cookie_key_last_update > sec->config->cookie_rekey_time) { - uint8_t cookie_key[COOKIE_KEY_SIZE]; - int ret; - - ret = gnutls_rnd(GNUTLS_RND_RANDOM, cookie_key, sizeof(cookie_key)); - if (ret >= 0) { - if (send_refresh_cookie_key(sec, cookie_key, sizeof(cookie_key)) == 0) { - sec->cookie_key_last_update = now; - memcpy(sec->cookie_key, cookie_key, sizeof(cookie_key)); - } else { - seclog(sec, LOG_ERR, "could not notify main for new cookie key"); - } - } else { - seclog(sec, LOG_ERR, "could not refresh cookie key"); - } - } - if (need_reload) { seclog(sec, LOG_DEBUG, "reloading configuration"); reload_cfg_file(sec, sec->perm_config, 0); @@ -777,7 +740,7 @@ static int load_keys(sec_mod_st *sec, unsigned force) * key operations. */ void sec_mod_server(void *main_pool, struct perm_cfg_st *perm_config, const char *socket_file, - uint8_t cookie_key[COOKIE_KEY_SIZE], int cmd_fd, int cmd_fd_sync) + int cmd_fd, int cmd_fd_sync) { struct sockaddr_un sa; socklen_t sa_len; @@ -819,11 +782,6 @@ void sec_mod_server(void *main_pool, struct perm_cfg_st *perm_config, const char exit(1); } - memcpy(sec->cookie_key, cookie_key, COOKIE_KEY_SIZE); - sec->cookie_key_last_update = time(0); - - sec->dcookie_key.data = sec->cookie_key; - sec->dcookie_key.size = COOKIE_KEY_SIZE; sec->perm_config = talloc_steal(sec, perm_config); sec->config = sec->perm_config->config; diff --git a/src/sec-mod.h b/src/sec-mod.h index c3f1ea6c..02225ce2 100644 --- a/src/sec-mod.h +++ b/src/sec-mod.h @@ -21,18 +21,14 @@ #ifndef SEC_MOD_H # define SEC_MOD_H -#include #include #include #include +#include #define SESSION_STR "(session: %.5s)" typedef struct sec_mod_st { - gnutls_datum_t dcookie_key; /* the key to generate cookies */ - uint8_t cookie_key[COOKIE_KEY_SIZE]; - time_t cookie_key_last_update; - struct cfg_st *config; struct perm_cfg_st *perm_config; gnutls_privkey_t *key; @@ -94,8 +90,6 @@ typedef struct client_entry_st { unsigned status; /* PS_AUTH_ */ char hostname[MAX_HOSTNAME_SIZE]; /* the requested hostname */ - uint8_t *cookie; /* the cookie associated with the session */ - unsigned cookie_size; uint8_t dtls_session_id[GNUTLS_MAX_SESSION_ID]; @@ -146,6 +140,6 @@ int handle_sec_auth_stats_cmd(sec_mod_st * sec, const CliStatsMsg * req); void sec_auth_user_deinit(sec_mod_st * sec, client_entry_st * e); void sec_mod_server(void *main_pool, struct perm_cfg_st *config, const char *socket_file, - uint8_t cookie_key[COOKIE_KEY_SIZE], int cmd_fd, int cmd_fd_sync); + int cmd_fd, int cmd_fd_sync); #endif diff --git a/src/vpn.h b/src/vpn.h index 649e91d4..75fe9712 100644 --- a/src/vpn.h +++ b/src/vpn.h @@ -104,6 +104,9 @@ inline static const char *proto_to_str(fw_proto_t proto) #define MIN_NO_COMPRESS_LIMIT 64 #define DEFAULT_NO_COMPRESS_LIMIT 256 +/* The time after a disconnection the cookie is valid */ +#define DEFAULT_COOKIE_RECON_TIMEOUT 120 + /* Timeout (secs) for communication between main and sec-mod */ #define MAIN_SEC_MOD_TIMEOUT 120 @@ -210,7 +213,6 @@ typedef enum { SM_CMD_AUTH_BAN_IP, SM_CMD_AUTH_BAN_IP_REPLY, SM_CMD_AUTH_CLI_STATS, - SM_CMD_REFRESH_COOKIE_KEY, MAX_SM_MAIN_CMD, } cmd_request_t; @@ -314,7 +316,6 @@ struct cfg_st { unsigned restrict_user_to_routes; /* whether the firewall script will be run for the user */ unsigned deny_roaming; /* whether a cookie is restricted to a single IP */ time_t cookie_timeout; /* in seconds */ - time_t cookie_rekey_time; /* in seconds */ time_t session_timeout; /* in seconds */ unsigned persistent_cookies; /* whether cookies stay valid after disconnect */ @@ -478,4 +479,12 @@ enum option_types { OPTION_NUMERIC, OPTION_STRING, OPTION_BOOLEAN, OPTION_MULTI_ #include +void reload_cfg_file(void *pool, struct perm_cfg_st* config, unsigned archive); +void clear_old_configs(struct perm_cfg_st* config); +void clear_cfg(struct perm_cfg_st* config); +void write_pid_file(void); +void remove_pid_file(void); + +extern sigset_t sig_default_set; + #endif diff --git a/src/worker-auth.c b/src/worker-auth.c index 4dc77a82..08c01635 100644 --- a/src/worker-auth.c +++ b/src/worker-auth.c @@ -39,7 +39,6 @@ #include #include "html.h" #include -#include #include #include @@ -720,19 +719,17 @@ static int recv_auth_reply(worker_st * ws, int sd, char **txt, unsigned *pcounte ws->sid_set = 1; } - if (msg->has_cookie == 0 || - msg->cookie.len == 0 || + if (msg->has_sid == 0 || + msg->sid.len != sizeof(ws->cookie) || msg->dtls_session_id.len != sizeof(ws->session_id)) { ret = ERR_AUTH_FAIL; goto cleanup; } - - ws->cookie = talloc_memdup(ws, msg->cookie.data, msg->cookie.len); - if (ws->cookie) { - ws->cookie_size = msg->cookie.len; - ws->cookie_set = 1; - } + + memcpy(ws->cookie, msg->sid.data, msg->sid.len); + ws->cookie_set = 1; + memcpy(ws->session_id, msg->dtls_session_id.data, msg->dtls_session_id.len); @@ -813,7 +810,7 @@ void cookie_authenticate_or_exit(worker_st *ws) /* we have authenticated against sec-mod, we need to complete * our authentication by forwarding our cookie to main. */ - ret = auth_cookie(ws, ws->cookie, ws->cookie_size); + ret = auth_cookie(ws, ws->cookie, sizeof(ws->cookie)); if (ret < 0) { oclog(ws, LOG_WARNING, "failed cookie authentication attempt"); if (ret == ERR_AUTH_FAIL) { @@ -880,7 +877,7 @@ int auth_cookie(worker_st * ws, void *cookie, size_t cookie_size) int post_common_handler(worker_st * ws, unsigned http_ver, const char *imsg) { int ret, size; - char str_cookie[BASE64_ENCODE_RAW_LENGTH(ws->cookie_size)+1]; + char str_cookie[BASE64_ENCODE_RAW_LENGTH(sizeof(ws->cookie))+1]; size_t str_cookie_size = sizeof(str_cookie); char msg[MAX_BANNER_SIZE + 32]; const char *success_msg_head; @@ -900,7 +897,7 @@ int post_common_handler(worker_st * ws, unsigned http_ver, const char *imsg) success_msg_foot_size = sizeof(oc_success_msg_foot)-1; } - oc_base64_encode((char *)ws->cookie, ws->cookie_size, + oc_base64_encode((char *)ws->cookie, sizeof(ws->cookie), (char *)str_cookie, str_cookie_size); /* reply */ diff --git a/src/worker-http-handlers.c b/src/worker-http-handlers.c index ca523f0a..c42d272b 100644 --- a/src/worker-http-handlers.c +++ b/src/worker-http-handlers.c @@ -36,7 +36,6 @@ #include #include -#include #include #define HTML_404 "

404 Not Found

\r\n" diff --git a/src/worker-http.c b/src/worker-http.c index 24072e18..2f09aa0e 100644 --- a/src/worker-http.c +++ b/src/worker-http.c @@ -370,21 +370,26 @@ void header_value_check(struct worker_st *ws, struct http_req_st *req) tmplen--; } + /* we allow for BASE64_DECODE_LENGTH reporting few bytes more + * than the expected */ nlen = BASE64_DECODE_LENGTH(tmplen); - ws->cookie = talloc_size(ws, nlen); - if (ws->cookie == NULL) + if (nlen < sizeof(ws->cookie) || nlen > sizeof(ws->cookie)+8) return; + /* we assume that - should be build time optimized */ + if (sizeof(ws->buffer) < sizeof(ws->cookie)+8) + abort(); + ret = oc_base64_decode((uint8_t*)p, tmplen, - ws->cookie, &nlen); - if (ret == 0) { - oclog(ws, LOG_DEBUG, + ws->buffer, &nlen); + if (ret == 0 || nlen != sizeof(ws->cookie)) { + oclog(ws, LOG_INFO, "could not decode cookie: %.*s", tmplen, p); ws->cookie_set = 0; } else { - ws->cookie_size = nlen; + memcpy(ws->cookie, ws->buffer, sizeof(ws->cookie)); ws->auth_state = S_AUTH_COOKIE; ws->cookie_set = 1; } diff --git a/src/worker-misc.c b/src/worker-misc.c index ca6a1b1f..55e983c3 100644 --- a/src/worker-misc.c +++ b/src/worker-misc.c @@ -40,7 +40,6 @@ #include #include -#include #include #ifdef HAVE_SIGALTSTACK diff --git a/src/worker-resume.c b/src/worker-resume.c index 14353892..97687a67 100644 --- a/src/worker-resume.c +++ b/src/worker-resume.c @@ -37,7 +37,6 @@ #include #include "common.h" #include "ipc.pb-c.h" -#include #include diff --git a/src/worker-vpn.c b/src/worker-vpn.c index 15df0448..0716d086 100644 --- a/src/worker-vpn.c +++ b/src/worker-vpn.c @@ -54,7 +54,6 @@ #include #include "ipc.pb-c.h" -#include #include #include diff --git a/src/worker.h b/src/worker.h index 5e7e7f74..cb878987 100644 --- a/src/worker.h +++ b/src/worker.h @@ -29,7 +29,6 @@ #include #include #include -#include #include #include #include @@ -253,8 +252,7 @@ typedef struct worker_st { unsigned cert_groups_size; char hostname[MAX_HOSTNAME_SIZE]; - uint8_t *cookie; - unsigned cookie_size; + uint8_t cookie[SID_SIZE]; unsigned int cookie_set;