| Message ID | 20221206133647.954724-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Fix connection cookie not including address and fix endianness in test | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> Stared-at-code, ran unit tests beforehand on FreeBSD/amd64 and AIX/PowerPC, verified that it *failed* on PowerPC. Ran unit tests again with the patch, and both FreeBSD/amd64 and AIX/PowerPC succeeded now, so endianness is taken into account now. I have not server-tested this, as it "should not" make a difference, and Arne did test this on s390. The full server tests will run tonight, though. As agreed on IRC, I've removed the comment from test_pkt.c that says "we do not use htons..." because now we do :-) Your patch has been applied to the master and release/2.6 branch. commit 67bef0357280040b83f2185c91c4f830ba542d6b (master) commit 3e63dc9b184b17674dfea3cea7eb55ce15779fb2 (release/2.6) Author: Arne Schwabe Date: Tue Dec 6 14:36:47 2022 +0100 Fix connection cookie not including address and fix endianness in test Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20221206133647.954724-1-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25619.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
Hi, On Tue, Dec 06, 2022 at 04:54:38PM +0100, Gert Doering wrote: > I have not server-tested this, as it "should not" make a difference, > and Arne did test this on s390. The full server tests will run > tonight, though. I *should* have, as that would have caught the fact that this patch breaks pkt_testdriver on Linux, no matter if OpenSSL or mbedTLS [ RUN ] test_calc_session_id_hmac_static [ ERROR ] --- difference at offset 0 0xffffff84 0xffffff8b difference at offset 1 0x73 0xffffffeb difference at offset 2 0x52 0x3d difference at offset 3 0x2b 0x20 difference at offset 4 0x5b 0x14 difference at offset 5 0xffffffa9 0x53 difference at offset 6 0x2a 0xffffffbe difference at offset 7 0x70 0x0a 8 bytes of 0x7ffe60967bd0 and 0x7ffe60967bc8 differ [ LINE ] --- test_pkt.c:492: error: Failure! [ FAILED ] test_calc_session_id_hmac_static ... [==========] 9 test(s) run. [ PASSED ] 8 test(s). [ FAILED ] 1 test(s), listed below: [ FAILED ] test_calc_session_id_hmac_static 1 FAILED TEST(S) FAIL: pkt_testdriver this is not specific to OpenSSL (1.1.1*) or mbedTLS, but seems to be "a Linux thing", so I assume it's something related to sockaddr (as this patch fixes the switch/case to actually look at the IPv4 addr) gert
Am 06.12.22 um 17:19 schrieb Gert Doering: > Hi, > > On Tue, Dec 06, 2022 at 04:54:38PM +0100, Gert Doering wrote: >> I have not server-tested this, as it "should not" make a difference, >> and Arne did test this on s390. The full server tests will run >> tonight, though. > > I *should* have, as that would have caught the fact that this patch > breaks pkt_testdriver on Linux, no matter if OpenSSL or mbedTLS > > [ RUN ] test_calc_session_id_hmac_static > [ ERROR ] --- difference at offset 0 0xffffff84 0xffffff8b > difference at offset 1 0x73 0xffffffeb > difference at offset 2 0x52 0x3d > difference at offset 3 0x2b 0x20 > difference at offset 4 0x5b 0x14 > difference at offset 5 0xffffffa9 0x53 > difference at offset 6 0x2a 0xffffffbe > difference at offset 7 0x70 0x0a > 8 bytes of 0x7ffe60967bd0 and 0x7ffe60967bc8 differ > [ LINE ] --- test_pkt.c:492: error: Failure! > [ FAILED ] test_calc_session_id_hmac_static > ... > [==========] 9 test(s) run. > [ PASSED ] 8 test(s). > [ FAILED ] 1 test(s), listed below: > [ FAILED ] test_calc_session_id_hmac_static > > 1 FAILED TEST(S) > FAIL: pkt_testdriver > > this is not specific to OpenSSL (1.1.1*) or mbedTLS, but seems to > be "a Linux thing", so I assume it's something related to sockaddr > (as this patch fixes the switch/case to actually look at the IPv4 addr) Yeah. macOS has: struct sockaddr_in { __uint8_t sin_len; sa_family_t sin_family; in_port_t sin_port; struct in_addr sin_addr; char sin_zero[8]; }; with sa_family_t also uint8_t and Linux has stupidly complex definition that boils down to: struct sockaddr_in { uint16_t sin_family; in_port_t sin_port; struct in_addr sin_addr char sin_zero[8]; }; So Linux basically has a 16 bit uint16 instead of two uint8_t. Because s390x is big endian, this happens to be same as on all BSDs with first byte being 0 and second byte being the family. I will send a fix that fixes the fix *grumbels* Arne
diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 7891e10ee..46bca21d8 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -495,7 +495,7 @@ calculate_session_id_hmac(struct session_id client_sid, /* Get the valid time quantisation for our hmac, * we divide time by handwindow/2 and allow the previous * and future session time if specified by offset */ - uint32_t session_id_time = now/((handwindow+1)/2) + offset; + uint32_t session_id_time = ntohl(now/((handwindow+1)/2) + offset); hmac_ctx_reset(hmac); /* We do not care about endian here since it does not need to be @@ -504,7 +504,7 @@ calculate_session_id_hmac(struct session_id client_sid, sizeof(session_id_time)); /* add client IP and port */ - switch (af_addr_size(from->addr.sa.sa_family)) + switch (from->addr.sa.sa_family) { case AF_INET: hmac_ctx_update(hmac, (const uint8_t *) &from->addr.in4, sizeof(struct sockaddr_in)); diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c index 2d771e301..1af46b7fb 100644 --- a/tests/unit_tests/openvpn/test_pkt.c +++ b/tests/unit_tests/openvpn/test_pkt.c @@ -435,6 +435,8 @@ test_verify_hmac_none(void **ut_state) hmac_ctx_t *hmac = session_id_hmac_init(); struct link_socket_actual from = { 0 }; + from.dest.addr.sa.sa_family = AF_INET; + struct tls_auth_standalone tas = { 0 }; struct tls_pre_decrypt_state state = { 0 }; @@ -463,7 +465,7 @@ init_static_hmac(void) ASSERT(md_valid("SHA256")); hmac_ctx_t *hmac_ctx = hmac_ctx_new(); - uint8_t key[SHA256_DIGEST_LENGTH] = {1, 2, 3}; + uint8_t key[SHA256_DIGEST_LENGTH] = {1, 2, 3, 0}; hmac_ctx_init(hmac_ctx, key, "SHA256"); return hmac_ctx; @@ -475,14 +477,14 @@ test_calc_session_id_hmac_static(void **ut_state) hmac_ctx_t *hmac = init_static_hmac(); static const int handwindow = 100; - struct openvpn_sockaddr addr = {0 }; + struct openvpn_sockaddr addr = { 0 }; /* we do not use htons functions here since the hmac calculate function * also does not care about the endianness of the data but just assumes * the endianness doesn't change between calls */ addr.addr.in4.sin_family = AF_INET; - addr.addr.in4.sin_addr.s_addr = 0xff000ff; - addr.addr.in4.sin_port = 1194; + addr.addr.in4.sin_addr.s_addr = ntohl(0xff000ff); + addr.addr.in4.sin_port = ntohs(1195); struct session_id client_id = { {0, 1, 2, 3, 4, 5, 6, 7}}; @@ -490,7 +492,7 @@ test_calc_session_id_hmac_static(void **ut_state) now = 1005; struct session_id server_id = calculate_session_id_hmac(client_id, &addr, hmac, handwindow, 0); - struct session_id expected_server_id = { {0xba, 0x83, 0xa9, 0x00, 0x72, 0xbd, 0x93, 0xba }}; + struct session_id expected_server_id = { {0x84, 0x73, 0x52, 0x2b, 0x5b, 0xa9, 0x2a, 0x70 }}; assert_memory_equal(expected_server_id.id, server_id.id, SID_SIZE); struct session_id server_id_m1 = calculate_session_id_hmac(client_id, &addr, hmac, handwindow, -1);
We accidentially checked the adress family size instead of the address family. For unit test checks we need to consider endianess to ensure the hmac for the adress is always the same. The real code does not care about endian since it only needs it to be same on the same architecture. Converting the session to endianess is strictly speaking unecessary for the actual function of the function but is almost no overhead and makes the unit testing more robust. Reported by David trying to the package on Red Hat/s390x and painfully debugged by setting up a s390x qemu machine that takes 40s just to run ./configure. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl_pkt.c | 4 ++-- tests/unit_tests/openvpn/test_pkt.c | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-)