diff mbox series

[Openvpn-devel] Fix password usage with ENABLE_PKCS11

Message ID 20260316224531.315912-1-luca.boccassi@gmail.com
State New
Headers
Series [Openvpn-devel] Fix password usage with ENABLE_PKCS11 |

Commit Message

Luca Boccassi March 16, 2026, 10:43 p.m. UTC 
  From: Luca Boccassi <luca.boccassi@gmail.com>

With ENABLE_PKCS11 USER_PASS_LEN is set to 4096 bytes.

But if a user specifies a large password, as allowed by the limit,
there are two places in the client that fail to handle it:

- in the TLS handling functions a single read/write is done into/out of
  the TLS layer, using a fixed size buffer at 2048 bytes, so the rest
  of the password is truncated
- in the management protocol when processing a "password" field another
  fixed buffer at 256 bytes is used, so the rest of the password is
  truncated

Use int_max(xyz, USER_PASS_LEN) to define these buffers. In normal builds
the current hardcoded values will stay the same, as they are higher.
In ENABLE_PKCS11 builds the 4096 value will be higher which will allow
longer passwords to work when communicating to the server and in the
management protocol.

Testing new client/old server shows no issues with a password that follows
the existing 2048 bytes limit. In order to use a > 2048 bytes password,
the server needs to be updated to this change too, otherwise it will truncate
it, but that is the same as status quo: only passwords < 2048 bytes work, so
there should be no regression.

This enables the use case where JIT use-once tokens are used as passwords.

Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
---
Alternative to:

https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/20260315230620.1594780-1-luca.boccassi%40gmail.com/#msg59309452

Instead of hardcoding larger buffers, simply use USER_PASS_LEN, so that
builds without ENABLE_PKCS11 are not affected.

With these small changes, it is possible to successfully
connect to an Azure VPN endpoint using the OpenVPN 2.7 client,
using a dummy username, an Entra token as password and the
server-secret from the azvpn XML config that users get as tls-auth
key.

 src/openvpn/common.h        | 2 +-
 src/openvpn/manage.c        | 2 +-
 src/openvpn/options.h       | 4 ++--
 src/openvpn/options_parse.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

Arne Schwabe March 17, 2026, 12:06 a.m. UTC | #1 
Am 16.03.2026 um 23:43 schrieb luca.boccassi@gmail.com:
> From: Luca Boccassi <luca.boccassi@gmail.com>
>
> With ENABLE_PKCS11 USER_PASS_LEN is set to 4096 bytes.
>
> But if a user specifies a large password, as allowed by the limit,
> there are two places in the client that fail to handle it:
>
> - in the TLS handling functions a single read/write is done into/out of
>    the TLS layer, using a fixed size buffer at 2048 bytes, so the rest
>    of the password is truncated
> - in the management protocol when processing a "password" field another
>    fixed buffer at 256 bytes is used, so the rest of the password is
>    truncated
>
> Use int_max(xyz, USER_PASS_LEN) to define these buffers. In normal builds
> the current hardcoded values will stay the same, as they are higher.
> In ENABLE_PKCS11 builds the 4096 value will be higher which will allow
> longer passwords to work when communicating to the server and in the
> management protocol.
>
> Testing new client/old server shows no issues with a password that follows
> the existing 2048 bytes limit. In order to use a > 2048 bytes password,
> the server needs to be updated to this change too, otherwise it will truncate
> it, but that is the same as status quo: only passwords < 2048 bytes work, so
> there should be no regression.
>
> This enables the use case where JIT use-once tokens are used as passwords.
>
> Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
> ---
> Alternative to:
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/20260315230620.1594780-1-luca.boccassi%40gmail.com/#msg59309452
>
> Instead of hardcoding larger buffers, simply use USER_PASS_LEN, so that
> builds without ENABLE_PKCS11 are not affected.
>
> With these small changes, it is possible to successfully
> connect to an Azure VPN endpoint using the OpenVPN 2.7 client,
> using a dummy username, an Entra token as password and the
> server-secret from the azvpn XML config that users get as tls-auth
> key.
>
>   src/openvpn/common.h        | 2 +-
>   src/openvpn/manage.c        | 2 +-
>   src/openvpn/options.h       | 4 ++--
>   src/openvpn/options_parse.c | 2 +-
>   4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/common.h b/src/openvpn/common.h
> index aa7b7217..e1096b98 100644
> --- a/src/openvpn/common.h
> +++ b/src/openvpn/common.h
> @@ -67,7 +67,7 @@ typedef unsigned long ptr_type;
>    * maximum size of a single TLS message (cleartext).
>    * This parameter must be >= PUSH_BUNDLE_SIZE
>    */
> -#define TLS_CHANNEL_BUF_SIZE 2048
> +#define TLS_CHANNEL_BUF_SIZE max_int(2048, USER_PASS_LEN) "

This would still mean that builds that use ENABLE_PKCS11 are 
potientially incompatible with builds without that flag and with older 
version.

Arne
Selva Nair March 17, 2026, 12:07 a.m. UTC | #2 
On Mon, Mar 16, 2026 at 6:48 PM <luca.boccassi@gmail.com> wrote:
>
> From: Luca Boccassi <luca.boccassi@gmail.com>
>
> With ENABLE_PKCS11 USER_PASS_LEN is set to 4096 bytes.
>
> But if a user specifies a large password, as allowed by the limit,
> there are two places in the client that fail to handle it:
>

The misunderstanding here is the assumption that ENABLE_PKCS11 builds
were supposed to support username/password > 128. They do not -- the
4096 number was added for some local usage (like private key passwords
and PINs, or some misunderstanding, I guess), not meant to apply to
password data to be passed from client to server or other way -- that
never worked.

As you have already seen, there are existing limits in multiple places
that make larger than 128 byte username/password not to work reliably
(or possibly 255). Management and option parsing always had 255 byte
limitations, the 2048 tls-channel-buffer limits password+username to
about 1600 bytes or thereabout.

I'm not saying we should not support larger passwords. In fact I do
feel that allowing the management interface to pass longer passwords
could be a useful goal if it can be done with minimal disruption (say
keep username + password fit in current control TLS buffers). But even
there, the "right" way to do it may be to use multi-line base64 as we
already do in the daemon -> client direction. If anyone has
"passwords" longer than 1k bytes or so, they are doing something wrong
and fixing it in the wrong place. "I have no access to the broken
piece of software, so let us fix it here" is not going to be
acceptable.

In any case, this has to be done carefully. There are a number of
implicit "protocol" changes in your patch:

(i) option parsing size -- are we going to define a new version of
config files that support longer lines? Looks like a wrong solution.
(ii) management interface -- do we support this only for UI
client-versions > x? How would the UI client know whether the daemon
will accept long lines?
(iii) control channel --- how to ensure backward compatibility?

I do not see your patches addressing any of these questions. Hence the
pushback. As Arne suggested, consider applying it locally for personal
use if it works in your case.

Selva

PS. This is not the first time this issue has come up -- search
openvpn devel list archives for past discussions. Here are some:

https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16674.html
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12535.html



Selva
Luca Boccassi March 22, 2026, 1:48 a.m. UTC | #3 
On Tue, 17 Mar 2026 at 00:07, Selva Nair <selva.nair@gmail.com> wrote:
>
> On Mon, Mar 16, 2026 at 6:48 PM <luca.boccassi@gmail.com> wrote:
> >
> > From: Luca Boccassi <luca.boccassi@gmail.com>
> >
> > With ENABLE_PKCS11 USER_PASS_LEN is set to 4096 bytes.
> >
> > But if a user specifies a large password, as allowed by the limit,
> > there are two places in the client that fail to handle it:
> >
>
> The misunderstanding here is the assumption that ENABLE_PKCS11 builds
> were supposed to support username/password > 128. They do not -- the
> 4096 number was added for some local usage (like private key passwords
> and PINs, or some misunderstanding, I guess), not meant to apply to
> password data to be passed from client to server or other way -- that
> never worked.
>
> As you have already seen, there are existing limits in multiple places
> that make larger than 128 byte username/password not to work reliably
> (or possibly 255). Management and option parsing always had 255 byte
> limitations, the 2048 tls-channel-buffer limits password+username to
> about 1600 bytes or thereabout.
>
> I'm not saying we should not support larger passwords. In fact I do
> feel that allowing the management interface to pass longer passwords
> could be a useful goal if it can be done with minimal disruption (say
> keep username + password fit in current control TLS buffers). But even
> there, the "right" way to do it may be to use multi-line base64 as we
> already do in the daemon -> client direction. If anyone has
> "passwords" longer than 1k bytes or so, they are doing something wrong
> and fixing it in the wrong place. "I have no access to the broken
> piece of software, so let us fix it here" is not going to be
> acceptable.

Thanks for the suggestion, I have sent a new version that only adds
the multi-line base64 protocol as recommended. Verified with a custom
build of NetworkManager-openvpn that uses this new management
interface.
Luca Boccassi March 25, 2026, 12:35 p.m. UTC | #4 
On Sun, 22 Mar 2026 at 01:48, Luca Boccassi <luca.boccassi@gmail.com> wrote:
>
> On Tue, 17 Mar 2026 at 00:07, Selva Nair <selva.nair@gmail.com> wrote:
> >
> > On Mon, Mar 16, 2026 at 6:48 PM <luca.boccassi@gmail.com> wrote:
> > >
> > > From: Luca Boccassi <luca.boccassi@gmail.com>
> > >
> > > With ENABLE_PKCS11 USER_PASS_LEN is set to 4096 bytes.
> > >
> > > But if a user specifies a large password, as allowed by the limit,
> > > there are two places in the client that fail to handle it:
> > >
> >
> > The misunderstanding here is the assumption that ENABLE_PKCS11 builds
> > were supposed to support username/password > 128. They do not -- the
> > 4096 number was added for some local usage (like private key passwords
> > and PINs, or some misunderstanding, I guess), not meant to apply to
> > password data to be passed from client to server or other way -- that
> > never worked.
> >
> > As you have already seen, there are existing limits in multiple places
> > that make larger than 128 byte username/password not to work reliably
> > (or possibly 255). Management and option parsing always had 255 byte
> > limitations, the 2048 tls-channel-buffer limits password+username to
> > about 1600 bytes or thereabout.
> >
> > I'm not saying we should not support larger passwords. In fact I do
> > feel that allowing the management interface to pass longer passwords
> > could be a useful goal if it can be done with minimal disruption (say
> > keep username + password fit in current control TLS buffers). But even
> > there, the "right" way to do it may be to use multi-line base64 as we
> > already do in the daemon -> client direction. If anyone has
> > "passwords" longer than 1k bytes or so, they are doing something wrong
> > and fixing it in the wrong place. "I have no access to the broken
> > piece of software, so let us fix it here" is not going to be
> > acceptable.
>
> Thanks for the suggestion, I have sent a new version that only adds
> the multi-line base64 protocol as recommended. Verified with a custom
> build of NetworkManager-openvpn that uses this new management
> interface.

Hi Selva,

Did you have any chance/time to have a quick look at
https://patchwork.openvpn.net/project/openvpn2/patch/20260322014606.1415363-1-luca.boccassi@gmail.com/
yet? Would love to hear if I understood your suggestion correctly.
Thanks!
diff mbox series

Patch

diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index aa7b7217..e1096b98 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -67,7 +67,7 @@  typedef unsigned long ptr_type;
  * maximum size of a single TLS message (cleartext).
  * This parameter must be >= PUSH_BUNDLE_SIZE
  */
-#define TLS_CHANNEL_BUF_SIZE 2048
+#define TLS_CHANNEL_BUF_SIZE max_int(2048, USER_PASS_LEN)
 
 /* TLS control buffer minimum size
  *
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index df72f15f..ea9bd8b0 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -2655,7 +2655,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(max_int(1024, USER_PASS_LEN));
         man->connection.out = buffer_list_new();
 
         /*
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 3d8b5059..a46c0d7c 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -53,8 +53,8 @@ 
 /*
  * Max size of options line and parameter.
  */
-#define OPTION_PARM_SIZE 256
-#define OPTION_LINE_SIZE 256
+#define OPTION_PARM_SIZE max_int(256, USER_PASS_LEN)
+#define OPTION_LINE_SIZE max_int(256, USER_PASS_LEN)
 
 extern const char title_string[];
 
diff --git a/src/openvpn/options_parse.c b/src/openvpn/options_parse.c
index cb51ad24..1372a5fb 100644
--- a/src/openvpn/options_parse.c
+++ b/src/openvpn/options_parse.c
@@ -374,7 +374,7 @@  read_config_file(struct options *options, const char *file, int level, const cha
                 int offset = 0;
                 CLEAR(p);
                 ++line_num;
-                if (strlen(line) == OPTION_LINE_SIZE)
+                if ((int)strlen(line) == OPTION_LINE_SIZE)
                 {
                     msg(msglevel,
                         "In %s:%d: Maximum option line length (%d) exceeded, line starts with %s",