Increase TLS Control Channel Buffer Size

Increase TLS_CHANNEL_BUF_SIZE to 4096. When the build is enabled with ENABLE_PKCS11, the password field can be 4096. The old size of TLS_CHANNEL_BUF_SIZE was 2048, which is not enough.

---
 src/openvpn/common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4e6f4809..7e9b5f3a 100644
@@ -75,7 +75,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 4096
 
 /*
  * This parameter controls the maximum size of a bundle

2.15.0.windows.1

Hi,

Please do not keep re-sending this patch just because you did not get a response right away. This messes up our patch tracking in patchwork, and annoys those that will eventually find time to review it.

Such a change might cause incompatibilities or overflows in unexpected ways, so a review is complex and not just anyone can do it - and those few that understand the openvpn protocol <-> tls interaction well enough are quite busy with other work.

So, patience please.

gert

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

Thank you for reminding this.  The reason that I have resent was there was a typo in the commit comment.
Best regards,Ning

    On Friday, March 16, 2018, 9:46:07 AM PDT, Gert Doering <[email protected]....> wrote:  

Hi,

Please do not keep re-sending this patch just because you did not get a response right away.  This messes up our patch tracking in patchwork, and annoys those that will eventually find time to review it.

Such a change might cause incompatibilities or overflows in unexpected ways, so a review is complex and not just anyone can do it - and those few that understand the openvpn protocol <-> tls interaction well enough are quite busy with other work.

So, patience please.

gert

now what should I write here...

Gert Doering - Munich, Germany                            [email protected]...  

Hi,

Apologies in advance if I'm misreading the intent of this patch.

I do not follow the logic behind this. Sure, ENABLE_PKCS11 increases the password buffer size to 4096, but pkcs11 password is only consumed locally, not sent out to the remote.

Now, even if a auth-user-pass password or challenge response gets as large as 4096 bytes, there are other places in the code that
need "fixing" to permit such long "passwords" (or response) -- e.g., it cannot be submitted via the management as the parser expects a max line length of 255 and word length of 255.

Further, even a TLS_CHANNEL_BUF_SIZE of 4096 does not support a password that long as there are other things like username, key-source, local-options that have to fit into the buffer (see key_method_2_write). So what's the logic behind 4096?

Finally, are there any particular challenge-response framework that requires such long responses? Even for U2F, we wouldn't need to send a response that large. U2F registration response could get very long (still mostly under 2000 bytes), but doing U2F key registration via the authentication channel is not a good implementation. Authentication responses are much smaller.

In short, it looks premature to increase this value before we start supporting long responses via the management, and a real practical need for long challenge response strings that won't fit in the current 2048 bytes (minus overhead) arise.

Selva

Sorry, I did not state the intent clearly in the request.
We wanted to use password field to pass authentication string, such as Token string,which can be long, the user auth plug in on the sever side can validate the token asthe part authentication step. Acquiring a token, such as JWT, usually involves user interaction with the token brokerservice provider.  So it is better to do this on the client side.
I agreed on the following statement.  To support a long password field in TLS channels,the Buf size of 4096 is not enough in key_method_2_write.  Should we propose to increase the buffer size further?
"Further, even a TLS_CHANNEL_BUF_SIZE of 4096 does not support a
password that long as there are other things like username, key-source,
local-options that have to fit into the buffer (see key_method_2_write). So
what's the logic behind 4096?"
Regards,Ning On Friday, March 16, 2018, 10:30:45 AM PDT, Selva Nair <[email protected]...> wrote:

Hi,

Apologies in advance if I'm misreading the intent of this patch.

I do not follow the logic behind this. Sure, ENABLE_PKCS11 increases the password buffer size to 4096, but pkcs11 password is only consumed locally, not sent out to the remote.

Now, even if a auth-user-pass password or challenge response gets as large as 4096 bytes, there are other places in the code that
need "fixing" to permit such long "passwords" (or response) -- e.g., it cannot be submitted via the management as the parser expects a max line length of 255 and word length of 255.

Further, even a TLS_CHANNEL_BUF_SIZE of 4096 does not support a password that long as there are other things like username, key-source, local-options that have to fit into the buffer (see key_method_2_write). So what's the logic behind 4096?

Finally, are there any particular challenge-response framework that requires such long responses? Even for U2F, we wouldn't need to send a response that large. U2F registration response could get very long (still mostly under 2000 bytes), but doing U2F key registration via the authentication channel is not a good implementation. Authentication responses are much smaller.

In short, it looks premature to increase this value before we start supporting long responses via the management, and a real practical need for long challenge response strings that won't fit in the current 2048 bytes (minus overhead) arise.

Selva

Hi,

Its hard for me to imagine what authentication would need to pass more than ~1600 bytes (that's about the current limit based on the 2048 byte buffer).
Unless you are trying to fit in data unrelated to auth in the token. The U2F registration case I mentioned earlier would be such an example.

How large is the data you need to transfer? If its not purely authentication data
(i.e., a challenge string + signature at most), sending it as password is not the right approach. If there is extra text included for convenience, pre-compressing
may help.

How do you plan to pass the "password" to openvpn client -- from stdin? The management i/f will not let you pass such a long password or username.

Increasing to 4096 looks marginally useful to me and will fit a reasonably sized user name (say < 128 bytes) and a largish challenge-response (upto ~3400 bytes) leaving space for other things. Beyond that is unlikely to be necessary.

I mentioned the overhead in key_method_2_write only because your logic behind this increase didn't make sense to me.

Selva

Both of key_method_2_write and key_method_2_read take TLS_Channel_Bug_Size as buffer size.  The current size, 2048 is not enough to read/write a long password response.  I have notice the management interface has a much smaller than 2048 buffer size to read/write.  Currently, if the management interface is not used, increasing tls channel buffer size will serve the need.
As token provider, the size of token can be bigger or smaller.  Sometime, it has more than 1600 as a token.  To accommodate that, a bigger buffer size will be needed.
ThanksNing

    On Monday, March 19, 2018, 10:11:25 PM PDT, Selva Nair <[email protected]...> wrote:  

Hi,

Its hard for me to imagine what authentication would need to pass more than ~1600 bytes  (that's about the current limit based on the 2048 byte buffer).
Unless you are trying to fit in data unrelated to auth in the token. The U2F registration case I mentioned earlier would be such an example.

How large is the data you need to transfer? If its not purely authentication data
(i.e., a challenge string + signature at most), sending it as password is not the right approach. If there is extra text included for convenience, pre-compressing
may help.

How do you plan to pass the "password" to openvpn client -- from stdin? The management i/f will not let you pass such a long password or username.

Increasing to 4096 looks marginally useful to me and will fit a reasonably sized user name (say < 128 bytes) and a largish challenge-response (upto ~3400 bytes) leaving space for other things. Beyond that is unlikely to be necessary.

I mentioned the overhead in key_method_2_write only because your logic behind this increase didn't make sense to me.

Selva

Hi,

I don't think this is achieving what you want. The username/password size on the OpenVPN protocol are fixed at 128 bytes max, and can not be changes without introducing a new protocol version or interoperability problems. See the USER_PASS_LEN variable used in key_method_2_read.

So, NAK to this patch.

-Steffan

Hi,

USER_PASS_LEN is 4096 (not 128) for builds with ENABLE_PKCS11 and that was the assumption behind this patch.

I'm not endorsing this patch but we do need changes to management interface and option parsing to allow long user/pass strings to support newer challenge-response
protocols. Those changes are not hard but this patch falls short as I had pointed
out earlier.

Selva

Hi,
Hi,

You're of course absolutely right. USER_PASS_LEN is not actually an interop issue here, the user/pass strings are not fixed length. Too quick with the answer, apologies.

But I think changing TLS_CHANNEL_BUF_SIZE will result in tricky behaviour; when reading from the TLS channel, current implementations read upto 2048 bytes, and pass that to key_method_2_read() as if it was the whole message. If the user/pass exceeds the old max, that will result in truncated username/passwords, and finally password verification errors. It's not fatal, but sounds like really annoying to debug...

-Steffan