From dd88111729abd392091f0fbb9889f9823a772319 Mon Sep 17 00:00:00 2001 From: Rainer Keller Date: Sun, 15 Sep 2024 07:37:38 +0200 Subject: [PATCH] Improve SAML authentication In the initial implementation there were still some issues that needed to be fixed. One main improvement is that the http server thread does not run the tunnel thread. Instead the http server is first shut down and then the tunnel is started as usual. --- src/config.c | 6 ++--- src/config.h | 5 ++-- src/http.c | 15 ++++++----- src/http.h | 1 - src/http_server.c | 69 ++++++++++++++--------------------------------- src/main.c | 20 ++++++++------ 6 files changed, 46 insertions(+), 70 deletions(-) diff --git a/src/config.c b/src/config.c index fd892979..12631088 100644 --- a/src/config.c +++ b/src/config.c @@ -45,6 +45,8 @@ const struct vpn_config invalid_cfg = { .password = {'\0'}, .password_set = 0, .cookie = NULL, + .saml_port = 0, + .saml_session_id = {'\0'}, .otp = {'\0'}, .otp_prompt = NULL, .otp_delay = -1, @@ -418,9 +420,7 @@ int load_config(struct vpn_config *cfg, const char *filename) if (strncmp(cfg->user_cert, "pkcs11:", 7) == 0) cfg->use_engine = 1; } else if (strcmp(key, "saml-login") == 0) { - free(cfg->saml_port); - cfg->saml_port = atol(val); - continue; + cfg->saml_port = atoi(val); } else if (strcmp(key, "user-key") == 0) { free(cfg->user_key); cfg->user_key = strdup(val); diff --git a/src/config.h b/src/config.h index 2ddd2c6b..352caae2 100644 --- a/src/config.h +++ b/src/config.h @@ -81,6 +81,7 @@ struct x509_digest { * We believe we are on the safe side using this value. */ #define MAX_DOMAIN_LENGTH 256 +#define MAX_SAML_SESSION_ID_LENGTH 1024 struct vpn_config { char gateway_host[GATEWAY_HOST_SIZE + 1]; @@ -91,8 +92,8 @@ struct vpn_config { int password_set; char otp[OTP_SIZE + 1]; char *cookie; - int saml_port; - char saml_session_id[1024]; + int saml_port; + char saml_session_id[MAX_SAML_SESSION_ID_LENGTH]; char *otp_prompt; unsigned int otp_delay; int no_ftm_push; diff --git a/src/http.c b/src/http.c index 8d6d595c..ff8fb0ab 100644 --- a/src/http.c +++ b/src/http.c @@ -634,12 +634,15 @@ int saml_login(struct tunnel *tunnel) int ret; ssl_connect(tunnel); - char uri[1024]; - snprintf(uri, sizeof(uri), "/remote/saml/auth_id?id=%s", tunnel->config->saml_session_id); + char uri_pattern[] = "/remote/saml/auth_id?id=%s"; + int required_size = snprintf(NULL, 0, uri_pattern, tunnel->config->saml_session_id) + 1; + char *uri = alloca(required_size); + snprintf(uri, required_size, uri_pattern, tunnel->config->saml_session_id); + char *response; uint32_t response_size = 0; ret = http_request(tunnel, "GET", uri, "", &response, &response_size); - if(ret != 1 || response_size <= 15) return ret; + if(ret != 1 || response_size <= 15) return -1; if (memcmp(response, "HTTP/1.1 200 OK", 15) != 0){ log_error("SAML login failed: %s\n", response); return ret; @@ -648,11 +651,9 @@ int saml_login(struct tunnel *tunnel) if (ret == ERR_HTTP_NO_COOKIE){ log_error("SAML login failed: no cookie\n"); return ret; - } - - // free(response); - + } + free(response); return ret; } diff --git a/src/http.h b/src/http.h index 08cd6899..96cd066c 100644 --- a/src/http.h +++ b/src/http.h @@ -19,7 +19,6 @@ #define OPENFORTIVPN_HTTP_H #include "tunnel.h" -#include "config.h" #include diff --git a/src/http_server.c b/src/http_server.c index ca94af74..b9efeed2 100644 --- a/src/http_server.c +++ b/src/http_server.c @@ -1,54 +1,42 @@ -#include #include #include #include -#include "config.h" -#include "log.h" -#include -#include "tunnel.h" #include -#include #include - +#include "config.h" +#include "log.h" +#include "tunnel.h" int process_request(int new_socket, char *id) { - log_info("Processing request\n"); + log_info("Processing HTTP SAML request\n"); int flag = 1; setsockopt(new_socket, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(int)); - const int buffer_size = 2048; - char *response[buffer_size]; - memset(response,' ',buffer_size); - response[buffer_size - 1] = '\0'; - char *reply = "HTTP/1.1 200 OK\r\n" + const char *reply = "HTTP/1.1 200 OK\r\n" "Content-Type: text/plain\r\n" "Connection: close\r\n\r\n" "SAML Login...\r\n\0"; - memcpy(response,reply,strlen(reply)); - // Read the request - ssize_t write_result = write(new_socket, response, buffer_size); + ssize_t write_result = write(new_socket, reply, strlen(reply)); (void)write_result; - // dup2(new_socket, STDOUT_FILENO); - // dup2(new_socket, STDERR_FILENO); - - - + // Read the request char request[1024]; ssize_t read_result = read(new_socket, request, sizeof(request)); // Check for '=id' in the response - if (read_result < 0 || strlen(request) < 10 || strncmp(request, "GET /?id=", 9) != 0) { + // If the recevied request from the server is larger than the buffer, the result will not be null-terminated. + // Causing strlen to behave wrong. + if (read_result < 0 || read_result == sizeof(request) || strlen(request) < 10 || strncmp(request, "GET /?id=", 9) != 0) { log_error("Bad request\n"); return -1; } // Extract the id const int id_start = 9; - char *id_end = memchr(&request[id_start], ' ', 1000); + char *id_end = memchr(&request[id_start], ' ', sizeof(request) - id_start); if (id_end == NULL) { log_error("Bad request format\n"); @@ -57,7 +45,7 @@ int process_request(int new_socket, char *id) { int id_length = id_end - &request[id_start]; - if(id_length == 0 || id_length > 1000) { + if(id_length == 0 || id_length > MAX_SAML_SESSION_ID_LENGTH) { log_error("Bad request id\n"); return -1; } @@ -69,7 +57,7 @@ int process_request(int new_socket, char *id) { log_error("Invalid id format\n"); return -1; } - // close(new_socket); + close(new_socket); log_info("Extracted id: %s\n", id); return 0; } @@ -94,6 +82,7 @@ void* start_http_server(void *void_config) { // Forcefully attaching socket to the port if (setsockopt(server_fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt))) { + close(server_fd); log_error("Failed to set socket options\n"); return NULL; } @@ -104,52 +93,34 @@ void* start_http_server(void *void_config) { // Forcefully attaching socket to the port if (bind(server_fd, (struct sockaddr *)&address, sizeof(address)) < 0) { + close(server_fd); log_error("Failed to bind socket to port %d \n",saml_port); return NULL; } if (listen(server_fd, 3) < 0) { + close(server_fd); log_error("Failed to listen on socket\n"); return NULL; } log_info("Listening for saml login on port: %d\n", saml_port); - int running = 1; - - pthread_t vpn_thread = 0; - - while(running) { + while(1) { if ((new_socket = accept(server_fd, (struct sockaddr *)&address, (socklen_t*)&addrlen)) < 0) { log_error("Failed to accept connection\n"); continue; } int result = process_request(new_socket, config->saml_session_id); + close(new_socket); if(result != 0) { log_error("Failed to process request\n"); continue; } - - - // Kill previous thread if it exists - if (vpn_thread != 0) { - log_error("Stop existing tunnel\n"); - pthread_kill(vpn_thread,SIGTERM); - pthread_join(vpn_thread, NULL); - } - - int thread_create_result = pthread_create(&vpn_thread, NULL, run_tunnel_wrapper, (void *)config); - if (thread_create_result != 0) { - log_error("Failed to create VPN thread\n"); - continue; - } - - - // pthread_detach(vpn_thread); - - result = 0; + break; } + close(server_fd); return NULL; } diff --git a/src/main.c b/src/main.c index e2add2a8..de499dcf 100644 --- a/src/main.c +++ b/src/main.c @@ -228,7 +228,7 @@ int main(int argc, char *argv[]) .password_set = 0, .cookie = NULL, .saml_port = 0, - .saml_session_id = NULL, + .saml_session_id = {'\0'}, .otp = {'\0'}, .otp_prompt = NULL, .otp_delay = 0, @@ -290,7 +290,7 @@ int main(int argc, char *argv[]) {"password", required_argument, NULL, 'p'}, {"cookie", required_argument, NULL, 0}, {"cookie-on-stdin", no_argument, NULL, 0}, - {"saml-login", optional_argument, NULL, 0}, + {"saml-login", optional_argument, NULL, 0}, {"otp", required_argument, NULL, 'o'}, {"otp-prompt", required_argument, NULL, 0}, {"otp-delay", required_argument, NULL, 0}, @@ -615,7 +615,7 @@ int main(int argc, char *argv[]) if(optarg != NULL){ port = strtol(optarg, NULL, 0); } - if (port < 0 || port > UINT_MAX) { + if (port < 0 || port > 65535) { log_warn("Invalid saml listen port: %s! Default port is 8020 \n", optarg); break; } @@ -732,7 +732,7 @@ int main(int argc, char *argv[]) goto user_error; } // If username but no password given, interactively ask user - if (!cfg.password_set && cfg.username[0] != '\0' && !cfg.cookie) { + if (!cfg.password_set && cfg.username[0] != '\0' && !cfg.cookie && !cfg.saml_port) { char hint[USERNAME_SIZE + 1 + REALM_SIZE + 1 + GATEWAY_HOST_SIZE + 10]; sprintf(hint, "%s_%s_%s_password", @@ -761,12 +761,16 @@ int main(int argc, char *argv[]) if(pthread_create(&server_thread, NULL, start_http_server, &cfg) != 0){ log_error("Failed to create saml login server thread\n"); - // ret = EXIT_FAILURE; + ret = EXIT_FAILURE; + goto exit; + } + log_debug("Running http server on port %d\n", cfg.saml_port); + pthread_join(server_thread, NULL); + + if (strlen(cfg.saml_session_id) == 0) { + log_error("Failed to receive SAML session id\n"); goto exit; } - // log_debug("Running http server on port %d\n", cfg.saml_port); - while(get_sig_received() == 0); - goto exit; } do {