Add tls and http apis for post handshake #713

fAuernigg · 2023-02-27T14:15:17Z

No description provided.

fAuernigg · 2023-02-27T14:17:59Z

PR #712 is required to fix the tls statemachine issue during tls v1.2 renegotiation.

Without PR #712 the test added for tls renegotiation fails under TLS v1.2

PR #711 is required to fix reading of certificate files on Win32 during renegotiation in the test added in this PR.

cspiel1 · 2023-02-28T14:20:15Z

I think we should preserve the standard naming scheme for call back function of re/baresip:

struct tls_conn_d {
	int (*verifyh) (int ok, void *arg);
	void *arg;

int tls_set_verify_client_handler(struct tls_conn *tc, int depth,
	int (*verifyh) (int ok, void *arg), void *arg);

sreimers · 2023-02-28T14:37:24Z

src/tls/openssl/tls.c

+int tls_set_verify_client_handler(struct tls_conn *tc, int depth,
+	int (*cb) (int ok, void *d), void *d)
+{
+#if !defined(LIBRESSL_VERSION_NUMBER)


I just upgraded LibreSSL for CI tests, can you try to enable these parts. At first quick look all required APIs are available now.

sreimers · 2023-02-28T16:05:31Z

Is TLS 1.2 renegotiation support needed? Maybe better only support secure TLS 1.3 handling for this use case?

cspiel1 · 2023-03-01T06:32:12Z

Is TLS 1.2 renegotiation support needed? Maybe better only support secure TLS 1.3 handling for this use case?

Indeed, we need it for HTTPS clients against MS Azure. And they are very lazy with security updates.

fAuernigg · 2023-03-01T06:39:04Z

Is TLS 1.2 renegotiation support needed? Maybe better only support secure TLS 1.3 handling for this use case?

As far as I known renegotiation in TLSv1.2 has only security flaws, if it is initiated by the client. In the test added in this PR the server initiates renegotiation.

sreimers · 2023-03-01T06:52:48Z

curl/curl#3258 (comment)

Note renegotiation is an extremely dangerous protocol feature. It is disabled by default in BoringSSL for security reasons and was removed in TLS 1.3.

Yeah, I wrote that text in BoringSSL. :-P Those limitations do not avoid renegotiation's problems. They are merely the bare minimum of simplifications we've been able to manage. Mid-renegotiation a lot of state is still pretty fuzzy, and renegotiation still exposes a lot of I/O patterns applications don't expect. I've rarely seen OpenSSL-consuming code that actually gets it right. And one still must be concerned with the renegotiation attacks from way back (lack of binding between the two handshakes without RI extension), as well as 3-SHAKE.

I wonder if this opens client side attacks:

https://github.com/baresip/re/pull/712/files#diff-2907ed5afee94d2389faf81e1d83c4110381960c319b5fb5d0a009cf2b170dc8R233

Since OpenSSL does not prevent this.

fAuernigg · 2023-03-01T07:09:11Z

I assume our only chance to get #712 to main branch is by making it configurable like in boring ssl?
Would this be acceptable?

A http client could then enable it only for certain http connections.

fAuernigg · 2023-03-02T10:01:38Z

I assume our only chance to get #712 to main branch is by making it configurable like in boring ssl? Would this be acceptable?

A http client could then enable it only for certain http connections.

TLS v1.2 renegotiation was removed from this PR. (Preparing another PR with TLSv1.2 enable api per connection)

cspiel1

Took the naming scheme from src/dns/client.c.

cspiel1 · 2023-03-03T07:54:31Z

src/tls/openssl/tls.c

+	}
+	else {
+		tc->cd.verifyh = cb;
+		tc->cd.verify_d = arg;


Rename please!

tc->cd.arg = arg;

cspiel1 · 2023-03-03T07:55:57Z

include/re_tls.h

@@ -46,6 +50,10 @@ int tls_set_certificate_der(struct tls *tls, enum tls_keytype keytype,
 			    const uint8_t *key, size_t len_key);
 int tls_set_certificate(struct tls *tls, const char *cert, size_t len);
 void tls_set_verify_client(struct tls *tls);
+void tls_set_verify_client_trust_all(struct tls *tls);
+int tls_set_verify_client_handler(struct tls_conn *tc, int depth,
+	int (*cb) (int ok, void *arg), void *arg);


rename
cb --> verifyh

cspiel1 · 2023-03-03T07:56:24Z

include/re_tls.h

@@ -28,6 +28,10 @@ enum tls_keytype {
 	TLS_KEYTYPE_EC,
 };

+struct tls_conn_d {
+	int (*verifyh) (int ok, void *arg);
+	void *verify_d;


rename:
verify_d --> arg

cspiel1 · 2023-03-03T08:04:54Z

src/http/server.c

@@ -29,6 +30,7 @@ struct http_sock {
 	struct tcp_sock *ts;
 	struct tls *tls;
 	http_req_h *reqh;
+	https_verify_msg_h *verify_msg_h;


rename to:
verify_msg_h --> verifyh

cspiel1 · 2023-03-03T08:07:12Z

src/http/server.c

+ * @return 0 if success, otherwise errorcode
+ */
+int  https_set_verify_msgh(struct http_sock *sock,
+	https_verify_msg_h *verify_msg_h)


rename
verify_msg_h --> verifyh

cspiel1 · 2023-03-03T08:07:48Z

src/http/server.c

+	if (!sock || !verify_msg_h)
+		return EINVAL;
+
+	sock->verify_msg_h = verify_msg_h;


rename to
sock->verifyh = verifyh;

cspiel1 · 2023-03-03T08:21:14Z

include/re_http.h


 int http_listen_fd(struct http_sock **sockp, re_sock_t fd, http_req_h *reqh,
 		   void *arg);
 int  http_listen(struct http_sock **sockp, const struct sa *laddr,
 		 http_req_h *reqh, void *arg);
 int  https_listen(struct http_sock **sockp, const struct sa *laddr,
 		  const char *cert, http_req_h *reqh, void *arg);
+int  https_set_verify_msgh(struct http_sock *sock,
+			   https_verify_msg_h *verify_msg_h);


rename to

int https_set_verify_msgh(struct http_sock *sock, https_verify_msg_h *verifyh);

alfredh · 2023-03-03T17:51:36Z

I have some more comments, please take them with a grain of salt :)

There are changes to HTTP and TLS modules, and the re-negotiation feature belongs to TLS.
There are probably other protocols sitting on top of TLS which may need the same feature,
for example SIP.

Is it possible to add the feature to TLS module so that all users of TLS gets
the feature transparently ?

For example in retest this means changes to tls.c only, not http.c

If this is possible, we could split this patch in two parts, where the first part
is for TLS and the second part is for HTTP.

alfredh · 2023-03-03T17:53:13Z

src/http/server.c

+ *
+ * @return TLS struct
+ */
+struct tls *http_sock_tls(struct http_sock *sock)


can the input param be const ?

alfredh · 2023-03-03T17:54:01Z

src/tls/openssl/tls.c

@@ -205,6 +207,19 @@ int tls_verify_handler(int ok, X509_STORE_CTX *ctx)
 #endif


+static int tls_verify_idx = -1;
+static once_flag oflag = ONCE_FLAG_INIT;


is it possible to avoid this global data and use the TLS context instead ?

I was trying to do that but it is not possible. It is not nice but openssl requires global indices initialized once. in tls_alloc call_once(&oflag, tls_init_verify_idx) is called.

Code examples are taken from openssl and apache2/httpd implementation to get application inside the openssl verify handlers, which are not supporting application data (so no tls context is present). So the only way to get application data is to use global openssl indices initialized once. (SSL_set_ex_data, SSL_get_ex_data(..., tls_verify_idx))

alfredh · 2023-03-03T17:54:58Z

src/tls/openssl/tls.c

+ *
+ * @return 0 if success, otherwise errorcode
+ */
+int tls_set_posthandshake_auth(struct tls *tls, int value)


can this return void ?

alfredh · 2023-03-03T17:56:16Z

src/tls/openssl/tls.c

+		err = EIO;
+		DEBUG_WARNING("%s: error: %m, ssl_err=%d\n", __func__, err,
+			SSL_get_error(tc->ssl, ret));
+	}


global openssl error queue must be cleared in both error cases

alfredh · 2023-03-03T17:56:49Z

src/tls/openssl/tls.c

+
+	if (!(ret=SSL_verify_client_post_handshake(tc->ssl))) {
+		err = EFAULT;
+		DEBUG_WARNING("%s: error: %m, ssl_err=%d\n", __func__, err,


please avoid func as it makes it harder to grep the code

fAuernigg · 2023-03-06T10:15:37Z

I have some more comments, please take them with a grain of salt :)

There are changes to HTTP and TLS modules, and the re-negotiation feature belongs to TLS. There are probably other protocols sitting on top of TLS which may need the same feature, for example SIP.

Is it possible to add the feature to TLS module so that all users of TLS gets the feature transparently ?

For example in retest this means changes to tls.c only, not http.c

If this is possible, we could split this patch in two parts, where the first part is for TLS and the second part is for HTTP.

I'm afraid but there is no transparent tls post handshake on server side.

For example a http server requests a post handshake (using SSL_verify_client_post_handshake) after it received http data. (Typically a specific http path, which requires more authentication data from the client).

Clients can support tls posthandshake transparently by setting for example a client certificate before a https request and enabling post handshake support using SSL_CTX_set_post_handshake_auth.

Anyhow of course I can split up the tls and http commit to make it easier to implement posthandshake in for example sip:

A sip server side implementation would need to support what "https_set_verify_msgh" of this PR does:

Allow to set a callback receiving OSI level 7 data (e.g. comparable to http data and http path) upon which it can decide, if a post handshake should be done.

src/tls/openssl/tls.c

Add tls_set_conn_verify_client_handler, tls_set_posthandshake_auth, tls_conn_verify_client_post_handshake available in tls >= v1.3

Support post handshake using https_set_verify_msgh and http_sock_tls. Allow decide to request certificate authorisation from client based on content of first http msg.

fAuernigg mentioned this pull request Feb 27, 2023

Client cert renegotiation in http #687

Closed

sreimers reviewed Feb 28, 2023

View reviewed changes

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch from 5db194e to 1b8f3ee Compare March 1, 2023 09:07

fAuernigg marked this pull request as ready for review March 1, 2023 10:43

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch 2 times, most recently from deb2220 to f63c397 Compare March 1, 2023 15:12

fAuernigg marked this pull request as draft March 2, 2023 09:58

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch 2 times, most recently from 4a81e04 to 2dbcfbf Compare March 2, 2023 10:01

fAuernigg marked this pull request as ready for review March 2, 2023 10:09

cspiel1 requested changes Mar 3, 2023

View reviewed changes

alfredh reviewed Mar 3, 2023

View reviewed changes

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch 2 times, most recently from 4ea5112 to 0f5e885 Compare March 8, 2023 14:01

fAuernigg requested review from cspiel1 and removed request for cspiel1 March 8, 2023 14:53

fAuernigg requested review from sreimers, alfredh and cspiel1 and removed request for sreimers, alfredh and cspiel1 March 8, 2023 14:53

sreimers added this to the v2.14.0 milestone Mar 9, 2023

fAuernigg changed the title ~~Add tls and http apis for renegotiation~~ Add tls and http apis for post handshake Mar 11, 2023

fAuernigg requested review from cspiel1 and removed request for alfredh March 13, 2023 08:09

cspiel1 approved these changes Mar 13, 2023

View reviewed changes

cspiel1 requested changes Mar 13, 2023

View reviewed changes

src/tls/openssl/tls.c Outdated Show resolved Hide resolved

src/tls/openssl/tls.c Outdated Show resolved Hide resolved

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch from 0f5e885 to 5ecf983 Compare March 20, 2023 08:59

fAuernigg requested a review from cspiel1 March 20, 2023 08:59

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch from 5ecf983 to 54ff0e1 Compare March 20, 2023 09:10

fAuernigg added 3 commits March 20, 2023 10:12

tls: add tls apis to support tls post handshake

172a70f

Add tls_set_conn_verify_client_handler, tls_set_posthandshake_auth, tls_conn_verify_client_post_handshake available in tls >= v1.3

http: add https support for post handshake

15d1ec4

Support post handshake using https_set_verify_msgh and http_sock_tls. Allow decide to request certificate authorisation from client based on content of first http msg.

test/http: add test for posthandshake with client cert verification

51013cb

fAuernigg force-pushed the add_tls_and_http_apis_for_renegotiation branch from 54ff0e1 to 51013cb Compare March 20, 2023 09:13

cspiel1 approved these changes Mar 20, 2023

View reviewed changes

sreimers merged commit bac6984 into baresip:main Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tls and http apis for post handshake #713

Add tls and http apis for post handshake #713

fAuernigg commented Feb 27, 2023

fAuernigg commented Feb 27, 2023 •

edited

Loading

cspiel1 commented Feb 28, 2023 •

edited

Loading

sreimers Feb 28, 2023

sreimers commented Feb 28, 2023

cspiel1 commented Mar 1, 2023

fAuernigg commented Mar 1, 2023

sreimers commented Mar 1, 2023

fAuernigg commented Mar 1, 2023

fAuernigg commented Mar 2, 2023

cspiel1 left a comment

cspiel1 Mar 3, 2023

cspiel1 Mar 3, 2023

cspiel1 Mar 3, 2023

cspiel1 Mar 3, 2023

cspiel1 Mar 3, 2023

cspiel1 Mar 3, 2023

cspiel1 Mar 3, 2023

alfredh commented Mar 3, 2023

alfredh Mar 3, 2023

alfredh Mar 3, 2023

fAuernigg Mar 8, 2023

alfredh Mar 3, 2023

alfredh Mar 3, 2023

alfredh Mar 3, 2023

fAuernigg commented Mar 6, 2023

Add tls and http apis for post handshake #713

Add tls and http apis for post handshake #713

Conversation

fAuernigg commented Feb 27, 2023

fAuernigg commented Feb 27, 2023 • edited Loading

cspiel1 commented Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

sreimers commented Feb 28, 2023

cspiel1 commented Mar 1, 2023

fAuernigg commented Mar 1, 2023

sreimers commented Mar 1, 2023

fAuernigg commented Mar 1, 2023

fAuernigg commented Mar 2, 2023

cspiel1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfredh commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fAuernigg commented Mar 6, 2023

fAuernigg commented Feb 27, 2023 •

edited

Loading

cspiel1 commented Feb 28, 2023 •

edited

Loading