From c9662282a1477116034060a589f20fc55ea115a4 Mon Sep 17 00:00:00 2001 From: Alan Jowett Date: Fri, 21 Feb 2020 12:44:48 -0700 Subject: [PATCH] Prevent tampering of our_ip, ip, session_start_time in SEC_AUTH_INIT from ocserv-worker to ocserv->sm and reject replay of auth_init_messages from old sessions. Resolves: #252 Signed-off-by: Alan Jowett --- src/Makefile.am | 3 ++- src/common/common.h | 1 + src/common/hmac.c | 58 ++++++++++++++++++++++++++++++++++++++++++ src/common/hmac.h | 39 ++++++++++++++++++++++++++++ src/ipc.proto | 2 ++ src/main-sec-mod-cmd.c | 2 +- src/main.c | 23 +++++++++++++++++ src/main.h | 4 ++- src/sec-mod-auth.c | 29 +++++++++++++++++++++ src/sec-mod.c | 4 ++- src/sec-mod.h | 5 +++- src/worker-auth.c | 27 +++----------------- src/worker-vpn.c | 3 --- src/worker.h | 3 +++ 14 files changed, 172 insertions(+), 31 deletions(-) create mode 100644 src/common/hmac.c create mode 100644 src/common/hmac.h diff --git a/src/Makefile.am b/src/Makefile.am index 439d1a14..a60fc968 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -45,7 +45,8 @@ ocserv_SOURCES = main.c main-auth.c worker-vpn.c worker-auth.c tlslib.c \ main-ban.c main-ban.h common-config.h valid-hostname.c \ str.c str.h gettime.h $(CCAN_SOURCES) $(HTTP_PARSER_SOURCES) \ sec-mod-acct.h setproctitle.c setproctitle.h sec-mod-resume.h \ - sec-mod-cookies.c defs.h inih/ini.c inih/ini.h + sec-mod-cookies.c defs.h inih/ini.c inih/ini.h \ + common/hmac.h common/hmac.c diff --git a/src/common/common.h b/src/common/common.h index 51bc64d1..36d9f0da 100644 --- a/src/common/common.h +++ b/src/common/common.h @@ -36,6 +36,7 @@ void *_talloc_size2(void *ctx, size_t size); #define MAX_IP_STR 46 + #define PROTOBUF_ALLOCATOR(name, pool) \ ProtobufCAllocator name = {.alloc = _talloc_size2, .free = _talloc_free2, .allocator_data = pool} diff --git a/src/common/hmac.c b/src/common/hmac.c new file mode 100644 index 00000000..e7476249 --- /dev/null +++ b/src/common/hmac.c @@ -0,0 +1,58 @@ +/* + * Copyright (C) 2020 Microsoft Corporation + * + * Author: Alan Jowett + * + * 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 + */ + +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +bool hmac_init_key(size_t key_length, uint8_t * key) +{ + return gnutls_rnd(GNUTLS_RND_RANDOM, key, key_length) == 0; +} + +void generate_hmac(size_t key_length, const uint8_t * key, size_t component_count, + const hmac_component_st * components, uint8_t digest[HMAC_DIGEST_SIZE]) +{ + struct hmac_sha256_ctx ctx; + size_t i; + + hmac_sha256_set_key(&ctx, key_length, key); + + for (i = 0; i < component_count; i++) { + + if (components[i].data) { + hmac_sha256_update(&ctx, + components[i].length, + (const uint8_t *)components[i].data); + } + } + + hmac_sha256_digest(&ctx, HMAC_DIGEST_SIZE, digest); + + safe_memset(&ctx, 0, sizeof(ctx)); +} diff --git a/src/common/hmac.h b/src/common/hmac.h new file mode 100644 index 00000000..1d72ba6e --- /dev/null +++ b/src/common/hmac.h @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2020 Microsoft Corporation + * + * Author: Alan Jowett + * + * 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 HMAC_H +#define HMAC_H +#include + +#define HMAC_DIGEST_SIZE 32 + +bool hmac_init_key(size_t key_length, uint8_t * key); + +typedef struct hmac_component_st { + size_t length; + void * data; +} hmac_component_st; + +void generate_hmac(size_t key_length, const uint8_t * key, size_t component_count, + const hmac_component_st * components, uint8_t digest[HMAC_DIGEST_SIZE]); + +#endif diff --git a/src/ipc.proto b/src/ipc.proto index b6f583ea..ce54968d 100644 --- a/src/ipc.proto +++ b/src/ipc.proto @@ -215,6 +215,8 @@ message sec_auth_init_msg optional string our_ip = 10; optional string user_agent = 11; optional string vhost = 12; + required uint64 session_start_time = 13; + required bytes hmac = 14; } /* SEC_AUTH_CONT */ diff --git a/src/main-sec-mod-cmd.c b/src/main-sec-mod-cmd.c index 5fc5bb44..133aa5b8 100644 --- a/src/main-sec-mod-cmd.c +++ b/src/main-sec-mod-cmd.c @@ -787,7 +787,7 @@ int run_sec_mod(main_server_st *s, int *sync_fd) set_cloexec_flag (fd[0], 1); set_cloexec_flag (sfd[0], 1); clear_unneeded_mem(s->vconfig); - sec_mod_server(s->main_pool, s->config_pool, s->vconfig, p, fd[0], sfd[0]); + sec_mod_server(s->main_pool, s->config_pool, s->vconfig, p, fd[0], sfd[0], sizeof(s->hmac_key), s->hmac_key); exit(0); } else if (pid > 0) { /* parent */ close(fd[0]); diff --git a/src/main.c b/src/main.c index 8f6dc199..c2a1abd3 100644 --- a/src/main.c +++ b/src/main.c @@ -62,6 +62,7 @@ #include #include #include +#include #ifdef HAVE_GSSAPI # include @@ -1042,6 +1043,7 @@ static void listen_watcher_cb (EV_P_ ev_io *w, int revents) int fd, ret; int cmd_fd[2]; pid_t pid; + hmac_component_st hmac_components[3]; if (ltmp->sock_type == SOCK_TYPE_TCP || ltmp->sock_type == SOCK_TYPE_UNIX) { /* connection on TCP port */ @@ -1120,6 +1122,22 @@ static void listen_watcher_cb (EV_P_ ev_io *w, int revents) ws->dtls_tptr.fd = -1; ws->conn_fd = fd; ws->conn_type = stype; + ws->session_start_time = time(0); + + human_addr2((const struct sockaddr *)&ws->remote_addr, ws->remote_addr_len, ws->remote_ip_str, sizeof(ws->remote_ip_str), 0); + human_addr2((const struct sockaddr *)&ws->our_addr, ws->our_addr_len, ws->our_ip_str, sizeof(ws->our_ip_str), 0); + + hmac_components[0].data = ws->remote_ip_str; + hmac_components[0].length = ws->remote_ip_str ? strlen(ws->remote_ip_str) : 0; + hmac_components[1].data = ws->our_ip_str; + hmac_components[1].length = ws->our_ip_str ? strlen(ws->our_ip_str) : 0; + hmac_components[2].data = &ws->session_start_time; + hmac_components[2].length = sizeof(ws->session_start_time); + + generate_hmac(sizeof(s->hmac_key), s->hmac_key, sizeof(hmac_components) / sizeof(hmac_components[0]), hmac_components, (uint8_t*) ws->sec_auth_init_hmac); + + // Clear the HMAC key + safe_memset((uint8_t*)s->hmac_key, 0, sizeof(s->hmac_key)); /* Drop privileges after this point */ drop_privileges(s); @@ -1268,6 +1286,11 @@ int main(int argc, char** argv) s->top_fd = -1; s->ctl_fd = -1; + if (!hmac_init_key(sizeof(s->hmac_key), (uint8_t*)(s->hmac_key))) { + fprintf(stderr, "unable to generate hmac key\n"); + exit(1); + } + list_head_init(&s->proc_list.head); list_head_init(&s->script_list.head); ip_lease_init(&s->ip_leases); diff --git a/src/main.h b/src/main.h index c7a21afe..e483fdaa 100644 --- a/src/main.h +++ b/src/main.h @@ -34,7 +34,7 @@ #include #include #include - +#include #include "vhost.h" #if defined(__FreeBSD__) || defined(__OpenBSD__) @@ -255,6 +255,8 @@ typedef struct main_server_st { void *main_pool; /* talloc main pool */ void *config_pool; /* talloc config pool */ + const uint8_t hmac_key[HMAC_DIGEST_SIZE]; + /* used as temporary buffer (currently by forward_udp_to_owner) */ uint8_t msg_buffer[MAX_MSG_SIZE]; } main_server_st; diff --git a/src/sec-mod-auth.c b/src/sec-mod-auth.c index 0e037c5d..a4d0a21e 100644 --- a/src/sec-mod-auth.c +++ b/src/sec-mod-auth.c @@ -53,6 +53,7 @@ #include #include #include +#include #ifdef HAVE_GSSAPI # include @@ -768,9 +769,37 @@ int handle_sec_auth_init(int cfd, sec_mod_st *sec, const SecAuthInitMsg *req, pi unsigned i; unsigned need_continue = 0; vhost_cfg_st *vhost; + hmac_component_st hmac_components[3]; + uint8_t computed_hmac[HMAC_DIGEST_SIZE]; + time_t now = time(0); + + if (req->hmac.len != HMAC_DIGEST_SIZE || !req->hmac.data) { + seclog(sec, LOG_AUTH, "hmac is the wrong size"); + return -1; + } + + /* Authenticate the client parameters */ + hmac_components[0].data = req->ip; + hmac_components[0].length = req->ip ? strlen(req->ip) : 0; + hmac_components[1].data = req->our_ip; + hmac_components[1].length = req->our_ip ? strlen(req->our_ip) : 0; + hmac_components[2].data = (void*)&req->session_start_time; + hmac_components[2].length = sizeof(req->session_start_time); + + generate_hmac(sizeof(sec->hmac_key), sec->hmac_key, sizeof(hmac_components) / sizeof(hmac_components[0]), hmac_components, computed_hmac); + + if (memcmp(computed_hmac, req->hmac.data, req->hmac.len) != 0) { + seclog(sec, LOG_AUTH, "hmac presented by client doesn't match parameters provided - possible replay"); + return -1; + } vhost = find_vhost(sec->vconfig, req->vhost); + if ((now - req->session_start_time) > vhost->perm_config.config->auth_timeout) { + seclog(sec, LOG_AUTH, "hmac presented by client expired - possible replay"); + return -1; + } + e = new_client_entry(sec, vhost, req->ip, pid); if (e == NULL) { seclog(sec, LOG_ERR, "cannot initialize memory"); diff --git a/src/sec-mod.c b/src/sec-mod.c index 9683b01f..a60250f5 100644 --- a/src/sec-mod.c +++ b/src/sec-mod.c @@ -887,7 +887,8 @@ static int load_keys(sec_mod_st *sec, unsigned force) * key operations. */ void sec_mod_server(void *main_pool, void *config_pool, struct list_head *vconfig, - const char *socket_file, int cmd_fd, int cmd_fd_sync) + const char *socket_file, int cmd_fd, int cmd_fd_sync, + size_t hmac_key_length, const uint8_t * hmac_key) { struct sockaddr_un sa; socklen_t sa_len; @@ -933,6 +934,7 @@ void sec_mod_server(void *main_pool, void *config_pool, struct list_head *vconfi sec->vconfig = vconfig; sec->config_pool = config_pool; sec->sec_mod_pool = sec_mod_pool; + memcpy((uint8_t*)sec->hmac_key, hmac_key, hmac_key_length); tls_cache_init(sec, &sec->tls_db); sup_config_init(sec); diff --git a/src/sec-mod.h b/src/sec-mod.h index e2c413c7..3a190c43 100644 --- a/src/sec-mod.h +++ b/src/sec-mod.h @@ -25,6 +25,7 @@ #include #include #include +#include #include "common/common.h" #include "vhost.h" @@ -47,6 +48,7 @@ typedef struct sec_mod_st { uint32_t avg_auth_time; /* the average time spent in (sucessful) authentication */ uint32_t total_authentications; /* successful authentications: to calculate the average above */ time_t last_stats_reset; + const uint8_t hmac_key[HMAC_DIGEST_SIZE]; } sec_mod_st; typedef struct stats_st { @@ -164,6 +166,7 @@ void sec_auth_user_deinit(sec_mod_st *sec, client_entry_st *e); void sec_mod_server(void *main_pool, void *config_pool, struct list_head *vconfig, const char *socket_file, - int cmd_fd, int cmd_fd_sync); + int cmd_fd, int cmd_fd_sync, + size_t hmac_key_length, const uint8_t * hmac_key); #endif diff --git a/src/worker-auth.c b/src/worker-auth.c index 5090f82a..24ccb4ef 100644 --- a/src/worker-auth.c +++ b/src/worker-auth.c @@ -1330,27 +1330,6 @@ int basic_auth_handler(worker_st * ws, unsigned http_ver, const char *msg) return ret; } -static char *get_our_ip(worker_st * ws, char str[MAX_IP_STR]) -{ - int ret; - struct sockaddr_storage sockaddr; - socklen_t socklen; - - if (ws->our_addr_len > 0) { - return human_addr2((struct sockaddr*)&ws->our_addr, ws->our_addr_len, str, MAX_IP_STR, 0); - } - - if (ws->udp_state != UP_ACTIVE) - return NULL; - - socklen = sizeof(sockaddr); - ret = getsockname(ws->dtls_tptr.fd, (struct sockaddr*)&sockaddr, &socklen); - if (ret == -1) - return NULL; - - return human_addr2((struct sockaddr*)&sockaddr, socklen, str, MAX_IP_STR, 0); -} - #define USERNAME_FIELD "username" #define GROUPNAME_FIELD "group%5flist" #define GROUPNAME_FIELD2 "group_list" @@ -1369,7 +1348,6 @@ int post_auth_handler(worker_st * ws, unsigned http_ver) char *username = NULL; char *password = NULL; char *groupname = NULL; - char our_ip_str[MAX_IP_STR]; char *msg = NULL; unsigned def_group = 0; unsigned pcounter = 0; @@ -1475,7 +1453,10 @@ int post_auth_handler(worker_st * ws, unsigned http_ver) ireq.vhost = ws->vhost->name; ireq.ip = ws->remote_ip_str; - ireq.our_ip = get_our_ip(ws, our_ip_str); + ireq.our_ip = ws->our_ip_str; + ireq.session_start_time = ws->session_start_time; + ireq.hmac.data = (uint8_t*)ws->sec_auth_init_hmac; + ireq.hmac.len = sizeof(ws->sec_auth_init_hmac); if (req->user_agent[0] != 0) ireq.user_agent = req->user_agent; diff --git a/src/worker-vpn.c b/src/worker-vpn.c index 52ce2630..e0e880e9 100644 --- a/src/worker-vpn.c +++ b/src/worker-vpn.c @@ -736,7 +736,6 @@ void vpn_server(struct worker_st *ws) "could not disable system calls, kernel might not support seccomp"); } } - ws->session_start_time = time(0); if (ws->remote_addr_len == sizeof(struct sockaddr_in)) ws->proto = AF_INET; @@ -819,8 +818,6 @@ void vpn_server(struct worker_st *ws) settings.on_body = http_body_cb; http_req_init(ws); - human_addr2((void*)&ws->remote_addr, ws->remote_addr_len, ws->remote_ip_str, sizeof(ws->remote_ip_str), 0); - if (WSCONFIG(ws)->listen_proxy_proto) { oclog(ws, LOG_DEBUG, "proxy-hdr: peer is %s\n", ws->remote_ip_str); } diff --git a/src/worker.h b/src/worker.h index 15b7e13b..fa2db868 100644 --- a/src/worker.h +++ b/src/worker.h @@ -36,6 +36,7 @@ #include #include #include +#include #include "vhost.h" typedef enum { @@ -201,7 +202,9 @@ typedef struct worker_st { socklen_t our_addr_len; struct sockaddr_storage remote_addr; /* peer's address */ socklen_t remote_addr_len; + char our_ip_str[MAX_IP_STR]; char remote_ip_str[MAX_IP_STR]; + const uint8_t sec_auth_init_hmac[HMAC_DIGEST_SIZE]; int proto; /* AF_INET or AF_INET6 */