From 7d0e92c6a543ae12ee3f871e4f20c72acf791eb4 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 | 17 +++---- src/http.h | 1 - src/http_server.c | 119 ++++++++++++++++++++++------------------------ src/http_server.h | 2 +- src/main.c | 24 +++++----- 7 files changed, 86 insertions(+), 88 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..d8a579d5 100644 --- a/src/http.c +++ b/src/http.c @@ -622,7 +622,7 @@ static int try_otp_auth(struct tunnel *tunnel, const char *buffer, /* - * Authenticates to gateway by sending username and password. + * Authenticates to gateway by sending a SAML session token. * * @return 1 in case of success * < 0 in case of error @@ -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..73ea142d 100644 --- a/src/http_server.c +++ b/src/http_server.c @@ -1,64 +1,71 @@ -#include #include #include #include +#include +#include + #include "config.h" #include "log.h" -#include #include "tunnel.h" -#include -#include -#include +// Convenience function to send a response with a user readable status message and the +// request URL shown for debug purposes. +// The response is shown in the user's browser after being redirected from the Fortinet Server. +static void send_status_response(int socket, const char *userMessage) { + const char *reply = "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: %lu\r\n" + "Connection: close\r\n\r\n" + "%s"; // User readable message + + int requiredSize = snprintf(NULL, 0, reply, strlen(userMessage), userMessage) + 1; + char *buffer = alloca(requiredSize); + snprintf(buffer, requiredSize, reply, strlen(userMessage), userMessage); + ssize_t write_result = write(socket, buffer, strlen(buffer)); + (void)write_result; +} 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" - "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); - (void)write_result; - - // dup2(new_socket, STDOUT_FILENO); - // dup2(new_socket, STDERR_FILENO); - - - 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)) { + log_error("Bad request\n"); + send_status_response(new_socket, "Invalid redirect response from Fortinet server. VPN could not be established."); + return -1; + } + + if (strlen(request) < 10 || strncmp(request, "GET /?id=", 9) != 0) { log_error("Bad request\n"); + send_status_response(new_socket, "Invalid redirect response from Fortinet server. VPN could not be established."); 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"); + send_status_response(new_socket, "Invalid formatting of Fortinet server redirect response. VPN could not be established."); return -1; } 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"); + send_status_response(new_socket, "Invalid SAML session id received from Fortinet server. VPN could not be established."); return -1; } @@ -67,19 +74,24 @@ int process_request(int new_socket, char *id) { for (int i = 0; i < id_length; i++) { if (isalnum(id[i]) || id[i] == '-') continue; log_error("Invalid id format\n"); + send_status_response(new_socket, "Invalid SAML session id received from Fortinet server. VPN could not be established."); return -1; } - // close(new_socket); + log_info("Extracted id: %s\n", id); + send_status_response(new_socket, + "SAML session id received from Fortinet server. VPN will be established..." + "\r\nYou can close this browser tab now."); return 0; } /** - * run a http server to listen for saml login requests + * Run a http server to listen for SAML login requests + * + * @return 0 in case of success + * < 0 in case of error */ -void* start_http_server(void *void_config) { - struct vpn_config *config = (struct vpn_config *)void_config; - +int start_http_server(struct vpn_config *config) { int server_fd, new_socket; struct sockaddr_in address; int opt = 1; @@ -89,67 +101,50 @@ void* start_http_server(void *void_config) { // Creating socket file descriptor if ((server_fd = socket(AF_INET, SOCK_STREAM, 0)) == 0) { log_error("Failed to create socket\n"); - return NULL; + return -1; } // 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; + return -1; } address.sin_family = AF_INET; - address.sin_addr.s_addr = INADDR_ANY; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); address.sin_port = htons(saml_port); // 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; + return -1; } if (listen(server_fd, 3) < 0) { + close(server_fd); log_error("Failed to listen on socket\n"); - return NULL; + return -1; } 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; } - return NULL; + close(server_fd); + return 0; } diff --git a/src/http_server.h b/src/http_server.h index b6f38103..8d1ff8af 100644 --- a/src/http_server.h +++ b/src/http_server.h @@ -1,3 +1,3 @@ #include "config.h" -void* start_http_server(void *void_config); \ No newline at end of file +int start_http_server(struct vpn_config *config); diff --git a/src/main.c b/src/main.c index e2add2a8..88505cc7 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", @@ -757,16 +757,18 @@ int main(int argc, char *argv[]) } if(cfg.saml_port != 0) { - pthread_t server_thread; + // Wait for the SAML token from the HTTP GET request + if (start_http_server(&cfg) != 0) { + log_error("Failed to retrieve SAML cookie from HTTP\n"); + ret = EXIT_FAILURE; + goto exit; + } - if(pthread_create(&server_thread, NULL, start_http_server, &cfg) != 0){ - log_error("Failed to create saml login server thread\n"); - // ret = EXIT_FAILURE; + if (strlen(cfg.saml_session_id) == 0) { + log_error("Failed to receive SAML session id\n"); + ret = EXIT_FAILURE; goto exit; } - // log_debug("Running http server on port %d\n", cfg.saml_port); - while(get_sig_received() == 0); - goto exit; } do {