Attention is currently required from: flichtenheld, plaisthos. MaxF has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/798?usp=email )
Change subject: Implement HKDF expand function based on RFC 8446 ...................................................................... Patch Set 4: Code-Review-2 (5 comments) Patchset: PS4: The implementation of HKDF-Expand is correct, but the implementation of HKDF-Expand-Label is slightly wrong in a way that causes different (label, context) pairs to produce identical keys (in a predictable way). Another thing: Are there any plans to actually use the context argument? File src/openvpn/crypto_epoch.h: http://gerrit.openvpn.net/c/openvpn/+/798/comment/152f282d_bc633665 : PS4, Line 32: * - salt is always assumed to be zero length (ie not supported) : * - IKM (secret) is assumed to be always 32 bytes salt and IKM are arguments for HKDF-Extract, not HKDF-Expand. File src/openvpn/crypto_epoch.c: http://gerrit.openvpn.net/c/openvpn/+/798/comment/006aedfa_6f857acd : PS4, Line 67: hmac_ctx_cleanup(hmac_ctx); : hmac_ctx_free(hmac_ctx); Not exactly about this commit itself, but looking at these functions, it's not clear to me why we need both. The doc comments for both say "free the HMAC context". http://gerrit.openvpn.net/c/openvpn/+/798/comment/ea9a9a5f_e4e2916c : PS4, Line 83: } We could check label_len and context_len too: * label_len <= 250 (because the total length including prefix must be <= 255) * context_len <= 255 http://gerrit.openvpn.net/c/openvpn/+/798/comment/b32d3125_e4ee1228 : PS4, Line 86: /* 2 byte for the outlen encoded as uint16, 5 bytes for "ovpn " */ : int hkdf_label_len = 2 + 5 + label_len + context_len; : struct buffer hkdf_label = alloc_buf_gc(hkdf_label_len, &gc); : : : buf_write_u16(&hkdf_label, out_len); : buf_write(&hkdf_label, "ovpn ", 5); : buf_write(&hkdf_label, label, label_len); : if (context_len > 0) : { : buf_write(&hkdf_label, context, context_len); : } That's not exactly how this works. When an RFC specifies a variable-length array, it has to be prefixed with a fixed-size length field. In this case, the maximum length for label and context is 255 each, so the length fields are 1 byte. See RFC 1832. That means hkdf_label_len = 2 + 1 + 5 + label_len + 1 + context_len. The label needs to be prefixed with label_len + 5 (the prefix is counted), the context is prefixed with context_len. Otherwise, the inputs would be ambiguous: label = "abcdef", context = "" and label = "abc", context = "def" would give the same result. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/798?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I3a1c6561f4d9a69e2a441d49dff620b4258a1bcc Gerrit-Change-Number: 798 Gerrit-PatchSet: 4 Gerrit-Owner: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: MaxF <m...@max-fillinger.net> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Comment-Date: Wed, 20 Nov 2024 16:48:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
