Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saml external browser #1217

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
3 changes: 2 additions & 1 deletion Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ openfortivpn_SOURCES = src/config.c src/config.h src/hdlc.c src/hdlc.h \
src/http.c src/http.h src/io.c src/io.h src/ipv4.c \
src/ipv4.h src/log.c src/log.h src/tunnel.c \
src/tunnel.h src/main.c src/ssl.h src/xml.c \
src/xml.h src/userinput.c src/userinput.h
src/xml.h src/userinput.c src/userinput.h \
src/idlistener.c src/idlistener.h
openfortivpn_CPPFLAGS = -DSYSCONFDIR=\"$(sysconfdir)\" \
-DPPP_PATH=\"@PPP_PATH@\" \
-DNETSTAT_PATH=\"@NETSTAT_PATH@\" \
Expand Down
20 changes: 20 additions & 0 deletions src/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ const struct vpn_config invalid_cfg = {
.user_agent = NULL,
.hostcheck = NULL,
.check_virtual_desktop = NULL,
.auth_id = NULL,
.listen_port = 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable names are very generic. Maybe, being more specific about what the listen_port is actually listen for might improve readability.

};

/*
Expand Down Expand Up @@ -466,6 +468,17 @@ int load_config(struct vpn_config *cfg, const char *filename)
} else if (strcmp(key, "check-virtual-desktop") == 0) {
free(cfg->check_virtual_desktop);
cfg->check_virtual_desktop = strdup(val);
} else if (strcmp(key, "ext-browser-saml") == 0) {
long port = 8020;
if (val != NULL) {
port = strtol(val, NULL, 0);
if (port < 1 || port > 65535) {
log_error("Bad ext-browser-saml port in configuration file: \"%s\".\n",
val);
goto err_free;
}
}
cfg->listen_port = (uint16_t) port;
} else {
log_warn("Bad key in configuration file: \"%s\".\n", key);
goto err_free;
Expand Down Expand Up @@ -534,6 +547,13 @@ void merge_config(struct vpn_config *dst, struct vpn_config *src)
free(dst->cookie);
dst->cookie = src->cookie;
}
if (src->auth_id != invalid_cfg.auth_id) {
free(dst->auth_id);
dst->auth_id = src->auth_id;
}
if (src->listen_port != invalid_cfg.listen_port) {
dst->listen_port = src->listen_port;
}
if (src->pinentry) {
free(dst->pinentry);
dst->pinentry = src->pinentry;
Expand Down
2 changes: 2 additions & 0 deletions src/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ struct vpn_config {
char password[PASSWORD_SIZE + 1];
int password_set;
char otp[OTP_SIZE + 1];
uint16_t listen_port;
char *auth_id;
char *cookie;
char *otp_prompt;
unsigned int otp_delay;
Expand Down
10 changes: 8 additions & 2 deletions src/http.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
* @param[out] dest the buffer to write the URL-encoded string
* @param[in] str the input string to be escaped
*/
static void url_encode(char *dest, const char *str)
void url_encode(char *dest, const char *str)
{
while (*str != '\0') {
if (isalnum(*str) || *str == '-' || *str == '_' ||
Expand Down Expand Up @@ -667,7 +667,13 @@ int auth_log_in(struct tunnel *tunnel)

tunnel->cookie[0] = '\0';

if (username[0] == '\0' && tunnel->config->password[0] == '\0') {
if (tunnel->config->auth_id != NULL) {
char url[256];
snprintf(url,sizeof(url), "/remote/saml/auth_id?id=%s",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Size check is missing. In case the auth ID gets too long, it will be cut off without notice.

tunnel->config->auth_id);
ret = http_request(tunnel, "GET", url, "", &res,
&response_size);
}else if (username[0] == '\0' && tunnel->config->password[0] == '\0') {
ret = http_request(tunnel, "GET", "/remote/login",
data, &res, &response_size);
} else {
Expand Down
13 changes: 13 additions & 0 deletions src/http.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,19 @@
#define ERR_HTTP_PERMISSION -6
#define ERR_HTTP_NO_COOKIE -7


/*
* URL-encodes a string for HTTP requests.
*
* The dest buffer size MUST be at least strlen(str) * 3 + 1.
*
* @param[out] dest the buffer to write the URL-encoded string
* @param[in] str the input string to be escaped
*/
void url_encode(char *dest, const char *str);



static inline const char *err_http_str(int code)
{
if (code > 0)
Expand Down
142 changes: 142 additions & 0 deletions src/idlistener.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
/*
* Copyright (c) 2024 Filippo Rossoni
*
* 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 3 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 <http://www.gnu.org/licenses/>.
*/
#include "idlistener.h"
#include "log.h"
#include "http.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <unistd.h>

#define MAX_REQUEST_SIZE 4096

static void print_url(struct vpn_config *cfg) {
char url[512];
snprintf(url, sizeof(url), "https://%s:%d/remote/saml/start?redirect=1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Size check missing. For long server names, the string might be cut off.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Size check missing. For long server names, the string might be cut off.

cfg->gateway_host, cfg->gateway_port);

if (cfg->realm[0] != '\0') {
strncat(url, "&realm=", sizeof(url) - 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my understanding the 3rd argument is how may bytes from the src argument it should copy, but not the total size of both strings combined.

Also it has to be checked if there is actual space in url to fit the additional string. According th the man page, behavior is undefined.

char *dt = url + strlen(url);
url_encode(dt, cfg->realm);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the documentation of url_encode it is stated that the buffer size must be at least 3x the size of cfg->realm +1. I can't find that reflected in the code here.

}
log_info("Authenticate at %s\n", url);
}

// Function to parse HTTP request and extract parameter "id"
static char* parse_request(const char *request) {
char *id_param;
char *query_start = strchr(request, '?');
if (query_start != NULL) {
id_param = strstr(query_start, "id=");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I case there is a more complex query string in the requested url, this will find possibly also other id= string like

GET /?myid=foo&id=bar

if (id_param != NULL) {
id_param += 3; // Length of "id="
char *id_end = strchr(id_param, '&');
if (id_end == NULL) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might strsep do what you want here?

id_end = strchr(id_param, ' ');
}
if (id_end == NULL) {
id_end = strchr(id_param, '\r');
}
if (id_end == NULL) {
id_end = id_param + strlen(id_param); // End of string
}
*id_end = '\0'; // Null-terminate the string
return strdup(id_param);

}
}
return NULL;
}

void send_response(int sockfd, const char *message) {
char response[MAX_REQUEST_SIZE];
sprintf(response, "HTTP/1.1 200 OK\r\n"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the message is coming from internal, maybe not everyone in future will be aware that there is a size restriction. At least the sprintf should be restricted to write out of bounds. Better would be to do a size check.

"Content-Length: %lu\r\n"
"Content-Type: text/html\r\n\r\n"
"%s", strlen(message), message);
write(sockfd, response, strlen(response));
}

char* listen_for_id(struct vpn_config *cfg) {
int sockfd = socket(AF_INET, SOCK_STREAM, 0);
if (sockfd < 0) {
log_error("Error opening socket");
exit(1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds a bit hard ;)
Why not return -1 ?

}

struct sockaddr_in serv_addr;

serv_addr.sin_family = AF_INET;
serv_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
serv_addr.sin_port = htons(cfg->listen_port);

int opt = 1;
if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) {
log_error("Error setting SO_REUSEADDR");
}
if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEPORT, &opt, sizeof(int)) < 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was meant to be sizeof(opt) like above.

log_error("error set SO_REUSEPORT");
}

if (bind(sockfd, (struct sockaddr*) &serv_addr, sizeof(serv_addr)) < 0) {
perror("Error on binding");
exit(1);
}



listen(sockfd, 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error check missing

log_debug("Server listening on port %d to retrieve the id\n",
cfg->listen_port);

print_url(cfg);

// Accept incoming connections
struct sockaddr_in cli_addr;
socklen_t clilen = sizeof(cli_addr);
int newsockfd = accept(sockfd, &cli_addr, &clilen);
if (newsockfd < 0) {
log_error("Error on accept");
return NULL;
}
close(sockfd);

// Read HTTP request from client
char buffer[MAX_REQUEST_SIZE];
bzero(buffer, MAX_REQUEST_SIZE);
read(newsockfd, buffer, MAX_REQUEST_SIZE - 1);
log_debug("Received HTTP request:\n%s\n", buffer);

char *id = parse_request(buffer);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a check if the id string has zero length, just in case something went wrong. I won't make sense to continue in that case.

if (id != NULL) {
log_debug("Extracted id: %s\n", id);
// Send response to client
send_response(newsockfd,
"<html><body><h1>ID retrieved. Connecting...!</h1></body></html>");
} else {
log_error("id parameter not found\n");
send_response(newsockfd,
"<html><body><h1>ERROR! id not found</h1></body></html>");
}
close(newsockfd);

return id;
}

15 changes: 15 additions & 0 deletions src/idlistener.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* cookieRetriever.h

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a name mistmatch?

Also the file name and function name are every generic. It would improve understandability if it whould somehow mention SAML somewhere.

*
* Created on: 28 apr 2024
* Author: filippor
*/

#ifndef SRC_IDLISTENER_H_
#define SRC_IDLISTENER_H_
#include <stddef.h>
#include "config.h"

char *listen_for_id(struct vpn_config *cfg );

#endif /* SRC_IDLISTENER_H_ */
44 changes: 40 additions & 4 deletions src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
#include "config.h"
#include "tunnel.h"
#include "userinput.h"
#include "idlistener.h"
#include "log.h"

#include <openssl/ssl.h>

#include <unistd.h>
Expand Down Expand Up @@ -78,6 +78,7 @@
#define usage \
"Usage: openfortivpn [<host>[:<port>]] [-u <user>] [-p <pass>]\n" \
" [--cookie=<cookie>] [--cookie-on-stdin]\n" \
" [--ext-browser-saml[=<listen-port>]] [--auth-id=<id>]\n" \
" [--otp=<otp>] [--otp-delay=<delay>] [--otp-prompt=<prompt>]\n" \
" [--pinentry=<program>] [--realm=<realm>]\n" \
" [--ifname=<ifname>] [--set-routes=<0|1>]\n" \
Expand Down Expand Up @@ -117,6 +118,10 @@ PPPD_USAGE \
" -p <pass>, --password=<pass> VPN account password.\n" \
" --cookie=<cookie> A valid session cookie (SVPNCOOKIE).\n" \
" --cookie-on-stdin Read the cookie (SVPNCOOKIE) from standard input.\n" \
" --ext-browser-saml[=<port>] Print an http address and start listen to recieve \n"\
" the autentication id to proceed the connection \n"\
" the default port if omitted is 8020\n"\
" --auth-id=<id> login with this id on address /remote/saml/auth_id?id=<id>\n"\
" -o <otp>, --otp=<otp> One-Time-Password.\n" \
" --otp-prompt=<prompt> Search for the OTP prompt starting with this string.\n" \
" --otp-delay=<delay> Wait <delay> seconds before sending the OTP.\n" \
Expand Down Expand Up @@ -225,6 +230,8 @@ int main(int argc, char *argv[])
.password = {'\0'},
.password_set = 0,
.cookie = NULL,
.listen_port =0,
.auth_id = NULL,
.otp = {'\0'},
.otp_prompt = NULL,
.otp_delay = 0,
Expand Down Expand Up @@ -286,6 +293,8 @@ int main(int argc, char *argv[])
{"password", required_argument, NULL, 'p'},
{"cookie", required_argument, NULL, 0},
{"cookie-on-stdin", no_argument, NULL, 0},
{"ext-browser-saml", optional_argument,NULL,0},
{"auth-id", required_argument,NULL,0},
{"otp", required_argument, NULL, 'o'},
{"otp-prompt", required_argument, NULL, 0},
{"otp-delay", required_argument, NULL, 0},
Expand Down Expand Up @@ -604,6 +613,24 @@ int main(int argc, char *argv[])
free(cookie);
break;
}
if (strcmp(long_options[option_index].name,
"ext-browser-saml") == 0) {
long port = 8020;
if (optarg != NULL) {
port = strtol(optarg, NULL, 0);
if (port < 1 || port > 65535) {
log_error("Specify a valid listen port or omit for parameter ext-browser-saml\n");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

Specify a valid listen port for parameter ext-browser-saml or omit it to use the default.

goto user_error;
}
}
cli_cfg.listen_port = (uint16_t)port;
break;
}
if (strcmp(long_options[option_index].name, "auth-id") == 0) {
free(cli_cfg.auth_id);
cli_cfg.auth_id = strdup(optarg);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it handy to have at least a zero length check to catch shell mistakes like empty variables when calling as early as possible.

break;
}
goto user_error;
case 'h':
printf("%s%s%s%s%s%s%s", usage, summary,
Expand Down Expand Up @@ -706,11 +733,11 @@ int main(int argc, char *argv[])
log_error("Specify a valid host:port couple.\n");
goto user_error;
}
// Check username
if (cfg.username[0] == '\0' && !cfg.cookie)
// Check authentication method
if (cfg.username[0] == '\0' && !cfg.cookie && !cfg.auth_id && cfg.listen_port==0)
// Need either username or cert
if (cfg.user_cert == NULL) {
log_error("Specify a username.\n");
log_error("Specify a authentication method.\n");
goto user_error;
}
// If username but no password given, interactively ask user
Expand All @@ -732,6 +759,15 @@ int main(int argc, char *argv[])
if (cfg.otp[0] != '\0')
log_debug("One-time password = \"%s\"\n", cfg.otp);

if (cfg.listen_port != 0){
if(cfg.auth_id){
log_warn("auth-id will be ignored conflict with --ext-browser-saml");
}
log_debug("Will listen on port \"%d\" for authentication id \n", cfg.listen_port);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's about the same message in idlistener.c:listen_for_id

free(cfg.auth_id);
cfg.auth_id = listen_for_id(&cfg);
}

if (geteuid() != 0) {
log_error("This process was not spawned with root privileges, which are required.\n");
ret = EXIT_FAILURE;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry, for putting it here, but I didn't find out how to comment on files that there not changed.)

This MR is missing changes to README.md and doc/openfortivpn.1.in

Expand Down