Am 20.07.20 um 15:16 schrieb David Sommerseth: > On 17/07/2020 15:47, Arne Schwabe wrote: >> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients. >> >> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling >> v1 op codes and give a good warning message if we encounter >> them in the legal op codes pre-check. >> >> Signed-off-by: Arne Schwabe <a...@rfc2549.org> >> --- >> doc/doxygen/doc_control_processor.h | 6 +- >> doc/doxygen/doc_key_generation.h | 6 +- >> doc/doxygen/doc_protocol_overview.h | 2 +- >> src/openvpn/forward.c | 2 +- >> src/openvpn/helper.c | 5 - >> src/openvpn/init.c | 1 - >> src/openvpn/options.c | 35 +---- >> src/openvpn/options.h | 4 - >> src/openvpn/ssl.c | 230 ++++------------------------ >> src/openvpn/ssl.h | 19 +-- >> src/openvpn/ssl_common.h | 1 - >> 11 files changed, 42 insertions(+), 269 deletions(-) > > [...snip...] > >> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c >> index 00b97352..4144217d 100644 >> --- a/src/openvpn/ssl.c >> +++ b/src/openvpn/ssl.c > > [...snip...] > >> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf) >> return str; >> } >> >> -/* >> - * Handle the reading and writing of key data to and from >> - * the TLS control channel (cleartext). >> - */ > > Is it needed to remove this comment? Or move it after the push_peer_info() > function?
Yeah, we can move the comment.
>
>> -static bool
>> -key_method_1_write(struct buffer *buf, struct tls_session *session)
>> -{
> [...snip...]
>> -}
>> -
>> static bool
>> push_peer_info(struct buffer *buf, struct tls_session *session)
>> {
>> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session
>> *session)
>> * push request, also signal that the client wants
>> * to get push-reply messages without without requiring a round
>> * trip for a push request message*/
>> - if(session->opt->pull)
>> + if (session->opt->pull)
>> {
>> iv_proto |= IV_PROTO_REQUEST_PUSH;
>> }
>> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct
>> tls_session *session)
>
> [...snip...]
>
>> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>> }
>>
>> /* hard reset ? */
>> - if (is_hard_reset(op, 0))
>> + if (is_hard_reset_method2(op))
>> {
>> /* verify client -> server or server -> client connection */
>> - if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
>> - || op == P_CONTROL_HARD_RESET_CLIENT_V2
>> + if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>> || op == P_CONTROL_HARD_RESET_CLIENT_V3) &&
>> !multi->opt.server)
>> - || ((op == P_CONTROL_HARD_RESET_SERVER_V1
>> - || op == P_CONTROL_HARD_RESET_SERVER_V2) &&
>> multi->opt.server))
>> + || ((op == P_CONTROL_HARD_RESET_SERVER_V2) &&
>> multi->opt.server))
>> {
>> msg(D_TLS_ERRORS,
>> "TLS Error: client->client or server->server
>> connection attempted from %s",
>> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>> * Initial packet received.
>> */
>>
>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>> {
>> struct tls_session *session = &multi->session[TM_ACTIVE];
>> struct key_state *ks = &session->key[KS_PRIMARY];
>>
>> - if (!is_hard_reset(op, multi->opt.key_method))
>> - {
>> - msg(D_TLS_ERRORS, "TLS ERROR: initial packet
>> local/remote key_method mismatch, local key_method=%d, op=%s",
>> - multi->opt.key_method,
>> - packet_opcode_name(op));
>> - goto error;
>> - }
>> -
>> /*
>> * If we have no session currently in progress, the initial
>> packet will
>> * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
>> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
>> }
>> }
>>
>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>
> Do we need this if() block? Considering it is exactly the same if() statement
> as the previous if() block. I would suggest merging them, but you might hit a
> challenge with reuse of struct tls_session *session, where one is referring to
> TM_ACTIVE vs TM_UNTRUSTED.
That is something that might be possible but I would consider outside of
patch. The first block actually modifies i (i = TM_ACTIVE;), so that
needs a separate patch with an explaination why/how we simplify the
logic here.
Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
