| Message ID | 20221126162648.150678-2-maximilian.fillinger@foxcrypto.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel,1/2] Correct tls-crypt-v2 metadata length in man page | expand |
Am 26.11.22 um 17:26 schrieb Max Fillinger: > The current code only checks if the base64-encoded metadata is at most > 980 characters. However, that can encode up to 735 bytes of data, while > only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn > prints a misleading error message saying that the base64 cannot be > decoded. > > This patch checks the decoded length to show an accurate error message. > > Acked-By: Arne Schwabe <arne@rfc2549.org>
Hi, On Sat, Nov 26, 2022 at 05:26:48PM +0100, Max Fillinger wrote: > The current code only checks if the base64-encoded metadata is at most > 980 characters. However, that can encode up to 735 bytes of data, while > only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn > prints a misleading error message saying that the base64 cannot be > decoded. > > This patch checks the decoded length to show an accurate error message. This patch looks overly complex to me for "change 735 to 733" - could you (or Arne) explain the change a bit better? (I see that the patch has an ACK, so this is not a showstopper, just be being confused) gert
Am 12.12.22 um 13:03 schrieb Gert Doering: > Hi, > > On Sat, Nov 26, 2022 at 05:26:48PM +0100, Max Fillinger wrote: >> The current code only checks if the base64-encoded metadata is at most >> 980 characters. However, that can encode up to 735 bytes of data, while >> only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn >> prints a misleading error message saying that the base64 cannot be >> decoded. >> >> This patch checks the decoded length to show an accurate error message. > > This patch looks overly complex to me for "change 735 to 733" - could > you (or Arne) explain the change a bit better? > > (I see that the patch has an ACK, so this is not a showstopper, just > be being confused) Problem is that base64 does padding. So e.g. a => YQ== ab => YWI= abc => YWJj So checking the length of the base64 gives you only something that is rounded up to the next 3 bytes (base64 encodes 3 bytes to 4 bytes). So if you have a limit like 733, you need to actually decode the base64 to check if it is short enough. The alternative would be to only allow 732 bytes, so we could check the base64 length again or use 735 bytes and use a maximum tls-crypt wrapped key size of 1026 bytes (which sounds a bit weird) Arne
Hi! > -----Original Message----- > From: Gert Doering [mailto:gert@greenie.muc.de] > Sent: maandag 12 december 2022 13:03 > To: Maximilian Fillinger <maximilian.fillinger@foxcrypto.com> > Cc: openvpn-devel@lists.sourceforge.net > Subject: Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls- > crypt-v2 metadata > > Hi, > > On Sat, Nov 26, 2022 at 05:26:48PM +0100, Max Fillinger wrote: > > The current code only checks if the base64-encoded metadata is at most > > 980 characters. However, that can encode up to 735 bytes of data, > while > > only up to 733 bytes are allowed. When passing 734 or 735 bytes, > openvpn > > prints a misleading error message saying that the base64 cannot be > > decoded. > > > > This patch checks the decoded length to show an accurate error > message. > > This patch looks overly complex to me for "change 735 to 733" - could > you (or Arne) explain the change a bit better? What's going on is that the tls-crypt-v2 metadata can be at most 733 bytes. But the metadata is supplied in base64. If you want to encode 733 bytes in base64, you need 980 characters. Right now, openvpn just checks that we have at most 980 base64 characters and then tries to decode them into a 733 byte buffer. But 980 characters of base64 can encode up to 735 bytes. In that case, openvpn gives a fatal error about being unable to decode the base64 which I find misleading. My patch always allocates a large enough buffer to decode the base64 and checks that the decoded length is <= 733. An alternative would be to check the base64 length, decode to a 735 bytes buffer, then check the decoded length. I thought it's cleaner to have one length check, but I don't have a strong opinion about this.
Hi, On Mon, Dec 12, 2022 at 12:24:10PM +0000, Maximilian Fillinger wrote: > Right now, openvpn just checks that we have at most 980 base64 characters > and then tries to decode them into a 733 byte buffer. But 980 characters > of base64 can encode up to 735 bytes. In that case, openvpn gives a fatal > error about being unable to decode the base64 which I find misleading. > > My patch always allocates a large enough buffer to decode the base64 and > checks that the decoded length is <= 733. An alternative would be to check > the base64 length, decode to a 735 bytes buffer, then check the decoded > length. I thought it's cleaner to have one length check, but I don't have > a strong opinion about this. Thanks (to you and Arne, your mails arrived here about the same time) - I was indeed confused about pre-decode / post-decode buffer size, and padding. Will proceed with merging as soon as I have the EEN patch out of the way. gert
Am 26.11.22 um 17:26 schrieb Max Fillinger: > The current code only checks if the base64-encoded metadata is at most > 980 characters. However, that can encode up to 735 bytes of data, while > only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn > prints a misleading error message saying that the base64 cannot be > decoded. > > This patch checks the decoded length to show an accurate error message. > Gert looked at this again and we found some issues now: This patch leaves an unused define TLS_CRYPT_V2_MAX_B64_METADATA_LEN that is no longer used. TLS_CRYPT_V2_MAX_METADATA_LEN is actually 734 sice it includes the type byte for the type of metadata, which gives us the 733 bytes of metadata decoded from base64. > BCAP(&metadata)); > @@ -644,10 +640,18 @@ tls_crypt_v2_write_client_key_file(const char *filename, > msg(M_FATAL, "ERROR: failed to base64 decode provided metadata"); > goto cleanup; > } > + if (decoded_len > TLS_CRYPT_V2_MAX_METADATA_LEN) > + { > + msg(M_FATAL, > + "ERROR: metadata too long (%d bytes, max %u bytes)", > + decoded_len, TLS_CRYPT_V2_MAX_METADATA_LEN - 1); > + goto cleanup; > + } The error message correctly uses 733 as length but the check should also check for TLS_CRYPT_V2_MAX_METADATA_LEN -1 or use >= Maybe we can add a unit test for this
> So if you have a limit like 733, you need to actually decode the base64 > to check if it is short enough. The alternative would be to only allow > 732 bytes, so we could check the base64 length again or use 735 bytes > and use a maximum tls-crypt wrapped key size of 1026 bytes (which sounds > a bit weird) Allowing 732 bytes/979 characters actually seems like the cleanest solution to me. It somehow just didn't occur to me. Well, now that my solution is acked, we can just go with it.
Hi,
On Mon, Dec 12, 2022 at 05:06:47PM +0000, Maximilian Fillinger wrote:
> Well, now that my solution is acked, we can just go with it.
It got an after-NAK, as there is an off-by-one... so feel free to
send a v2 either way :-)
gert
diff --git a/src/openvpn/base64.h b/src/openvpn/base64.h index f49860fc..7b4224a5 100644 --- a/src/openvpn/base64.h +++ b/src/openvpn/base64.h @@ -38,6 +38,10 @@ #define OPENVPN_BASE64_LENGTH(binary_length) \ ((((8 * binary_length) / 6) + 3) & ~3) +/** Compute the maximal number of bytes encoded in a base64 string. */ +#define OPENVPN_BASE64_DECODED_LENGTH(base64_length) \ + ((base64_length / 4) * 3) + int openvpn_base64_encode(const void *data, int size, char **str); int openvpn_base64_decode(const char *str, void *data, int size); diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 2fc79111..5d247b84 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -627,15 +627,11 @@ tls_crypt_v2_write_client_key_file(const char *filename, } ASSERT(buf_write(&dst, client_key.keys, sizeof(client_key.keys))); - struct buffer metadata = alloc_buf_gc(TLS_CRYPT_V2_MAX_METADATA_LEN, &gc); + struct buffer metadata; if (b64_metadata) { - if (TLS_CRYPT_V2_MAX_B64_METADATA_LEN < strlen(b64_metadata)) - { - msg(M_FATAL, - "ERROR: metadata too long (%d bytes, max %u bytes)", - (int)strlen(b64_metadata), TLS_CRYPT_V2_MAX_B64_METADATA_LEN); - } + size_t b64_length = strlen(b64_metadata); + metadata = alloc_buf_gc(OPENVPN_BASE64_DECODED_LENGTH(b64_length) + 1, &gc); ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_USER, 1)); int decoded_len = openvpn_base64_decode(b64_metadata, BEND(&metadata), BCAP(&metadata)); @@ -644,10 +640,18 @@ tls_crypt_v2_write_client_key_file(const char *filename, msg(M_FATAL, "ERROR: failed to base64 decode provided metadata"); goto cleanup; } + if (decoded_len > TLS_CRYPT_V2_MAX_METADATA_LEN) + { + msg(M_FATAL, + "ERROR: metadata too long (%d bytes, max %u bytes)", + decoded_len, TLS_CRYPT_V2_MAX_METADATA_LEN - 1); + goto cleanup; + } ASSERT(buf_inc_len(&metadata, decoded_len)); } else { + metadata = alloc_buf_gc(1 + sizeof(int64_t), &gc); int64_t timestamp = htonll((uint64_t)now); ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_TIMESTAMP, 1)); ASSERT(buf_write(&metadata, ×tamp, sizeof(timestamp)));
The current code only checks if the base64-encoded metadata is at most 980 characters. However, that can encode up to 735 bytes of data, while only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn prints a misleading error message saying that the base64 cannot be decoded. This patch checks the decoded length to show an accurate error message. Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> --- src/openvpn/base64.h | 4 ++++ src/openvpn/tls_crypt.c | 18 +++++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-)