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

Allow longer username and password under Dynamic Challenge/Response P… #295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 218 additions & 0 deletions flake.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions flake.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
{
inputs = {
nixpkgs.url = "github:NixOS/nixpkgs/nixos-22.11";
devenv.url = "github:cachix/devenv";
};

outputs = { self, nixpkgs, devenv, ... } @ inputs:
let
systems = [ "x86_64-linux" "i686-linux" "x86_64-darwin" "aarch64-linux" "aarch64-darwin" ];
forAllSystems = f: builtins.listToAttrs (map (name: { inherit name; value = f name; }) systems);
in
{
devShells = forAllSystems
(system:
let
pkgs = import nixpkgs {
inherit system;
};
in
{
default = devenv.lib.mkShell {
inherit inputs pkgs;
modules = [
{
# https://devenv.sh/reference/options/
packages = [
pkgs.autoconf
pkgs.automake
pkgs.libtool
pkgs.openssl_1_1
pkgs.lz4
pkgs.lzo
pkgs.pam
pkgs.cmocka
];

languages.c.enable = true;

enterShell = ''
# Allows autreconf to find libtool.
export ACLOCAL_PATH=${pkgs.libtool}/share/aclocal:$ACLOCAL_PATH
'';
}
];
};
});
};
}
6 changes: 4 additions & 2 deletions src/openvpn/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,11 @@ typedef unsigned long ptr_type;
/*
* This parameter controls the TLS channel buffer size and the
* maximum size of a single TLS message (cleartext).
* This parameter must be >= PUSH_BUNDLE_SIZE
* This parameter must be >= PUSH_BUNDLE_SIZE. It must also be greater than
* the size of a long (>50Kb) password in the dyanmic challenge/response
* protocol,
*/
#define TLS_CHANNEL_BUF_SIZE 2048
#define TLS_CHANNEL_BUF_SIZE 65536
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this makes sense as TLS record size is only 16k.


/* TLS control buffer minimum size
*
Expand Down
4 changes: 2 additions & 2 deletions src/openvpn/manage.c
Original file line number Diff line number Diff line change
Expand Up @@ -2244,7 +2244,7 @@ man_read(struct management *man)
/*
* read command line from socket
*/
unsigned char buf[256];
unsigned char buf[TLS_CHANNEL_BUF_SIZE];
int len = 0;

#ifdef TARGET_ANDROID
Expand Down Expand Up @@ -2580,7 +2580,7 @@ man_connection_init(struct management *man)
* Allocate helper objects for command line input and
* command output from/to the socket.
*/
man->connection.in = command_line_new(1024);
man->connection.in = command_line_new(TLS_CHANNEL_BUF_SIZE);
man->connection.out = buffer_list_new();

/*
Expand Down
6 changes: 5 additions & 1 deletion src/openvpn/misc.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ struct user_pass
#ifdef ENABLE_PKCS11
#define USER_PASS_LEN 4096
#else
#define USER_PASS_LEN 128
/*
* Increase the username and password length size to 65KB, in order
* to support long passwords under the dynamic challenge/response protocol.
*/
#define USER_PASS_LEN 65536
Copy link
Contributor

Choose a reason for hiding this comment

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

Only increasing the size to 65k when PKCS11 is not enabled feels questionable to say the least. This would in most cases not change anything as PKCS11 is typically enabled.

#endif
/* Note that username and password are expected to be null-terminated */
char username[USER_PASS_LEN];
Expand Down
9 changes: 6 additions & 3 deletions src/openvpn/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,13 @@
#define MAX_PARMS 16

/*
* Max size of options line and parameter.
* Max size of options line and parameter. Note these
* must be able to accomodate large (>50Kb) values in
* order to support long passwords under the dynamic challenge-response
* protocol.
*/
#define OPTION_PARM_SIZE 256
#define OPTION_LINE_SIZE 256
#define OPTION_PARM_SIZE USER_PASS_LEN
#define OPTION_LINE_SIZE OPTION_PARM_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing these constants here is a very drastic change that changes all kind of parsing.


extern const char title_string[];

Expand Down