-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
When using Mbed TLS, not all data will be processed. #2668
Comments
I tried testing with |
Can you please check if our built-in TLS stack solves your issue ? |
Initial test with latest
After that returns no TLS Client Hello was sent, eventually the broker disconnected due to its timeout. I'll have some time to tinker with it this evening (US Central timezone) |
It requires EC certificates and keys, the API is the same. |
I am able to reproduce the issue with Other messages, earlier in the MQTT handshake did require multiple iterations through this loop, as many as 5 iterations. Here is the same patch, with logging diff --git a/mongoose.c b/mongoose.c
index 62fde56a..48080ee8 100644
--- a/mongoose.c
+++ b/mongoose.c
@@ -5533,13 +5533,23 @@ static void read_conn(struct mg_connection *c, struct pkt *pkt) {
mg_error(c, "oom");
} else {
// Decrypt data directly into c->recv
- long n = mg_tls_recv(c, &io->buf[io->len], io->size - io->len);
- if (n == MG_IO_ERR) {
- mg_error(c, "TLS recv error");
- } else if (n > 0) {
- // Decrypted successfully - trigger MG_EV_READ
- io->len += (size_t) n;
- mg_call(c, MG_EV_READ, &n);
+ int ct = 0;
+ while(true) {
+ ++ct;
+ long n = mg_tls_recv(c, &io->buf[io->len], io->size - io->len);
+ if (n == MG_IO_ERR) {
+ mg_error(c, "TLS recv error");
+ break;
+ } else if (n > 0) {
+ // Decrypted successfully - trigger MG_EV_READ
+ io->len += (size_t) n;
+ mg_call(c, MG_EV_READ, &n);
+ if(ct > 1) {
+ MG_DEBUG(("TLS recv loop %d", ct));
+ }
+ } else {
+ break;
+ }
}
} I attach logs of the transaction with and without my patch And a pcap just for good measure 192.168.192.115 is the mongoose mqtt server under test Note that round-trip time between those two is around 22ms, I am suspecting that you need a certain amount of latency in order to repro the issue, otherwise the TCP packets would not accumulate so many TLS records. This could be verified by testing again with NODELAY turned on. Anyway, this is enough for me today. |
@cpq This seems to be yet another corner case. @tubular-rheostat Thank you, Brian, you helped a lot. |
@tubular-rheostat thanks! loops is receive code are dangerous. Brian, could you try another patch please? Try to change this line Line 171 in 7581229
To this: return tls == NULL ? 0 : c->rtls.len + mbedtls_ssl_get_bytes_avail(&tls->ssl); |
That change didn't help. I'm using Mongoose builtin TCP, the only code I saw referring to that edited function was in I can try running my test code with BSD sockets but I probably won't have time to work on it until the weekend. If it helps here is some of my test program: https://gist.github.com/tubular-rheostat/1cdd37e82aef3e2117fc5d377e060b1b |
Brian, as can be seen on the link, that change is in Lines 10904 to 10907 in 7581229
Please build and test with MbedTLS, this applies to both sockets and built-in TCP stack. |
That is what I did. I copied the When I said the function is not used outside of |
I see something similar on Windows, with
And With Seems something has changed in MBedTLS recently. I built it from |
@gvanem Mmmmm.... I think that is another issue, this one is not MbedTLS specific but our problem. |
@tubular-rheostat Oops, you are right @cpq I think the fix is cool. The elephant in the room is that our built-in TCP/IP stack's
WDYT ? |
I will try that last patch on my test env., probably in the evening (US Central time). I tried testing with BSD sockets + MbedTLS but could never repro the issue (with unmodified For testing on the Mac I also tried simulating latency by adding a delay to the MQTT broker machine's network interface, on the assumption that latency helps repro the issue, but it had no effect, using the Linux |
Yeah, this is really hard to reproduce. My suggestion would work on our built-in TCP/IP stack but is just an idea, we haven't actually tried it. If you can, that would be a nice input. |
I tried the most recent patch (modified slightly to get it to compile) against master
I think the underlying MbedTLS error code was I was not able to work out what is going wrong. |
I was able to repro the issue on my laptop, see https://github.com/tubular-rheostat/mongoose-test-2668 for a test program. Hopefully this makes further development work and testing easier. Apologies if you don't have a Mac to test on... |
I tried retesting with @scarpile's latest patch and didn't have the problem with TLS handshake again, it was able to connect. I don't know what I did that was different. It did not resolve my issue though. I don't think the new I was experimenting with an alternate fix, just have long mg_tls_recv(struct mg_connection *c, void *buf, size_t len) {
struct mg_tls *tls = (struct mg_tls *) c->tls;
size_t total_read = 0;
while (total_read < len) {
long n = mbedtls_ssl_read(&tls->ssl, buf + total_read, len - total_read);
if(n > 0) {
total_read += (size_t) n;
} else if (n == MBEDTLS_ERR_SSL_WANT_READ || n == MBEDTLS_ERR_SSL_WANT_WRITE) {
if(total_read == 0) {
return MG_IO_WAIT;
}
return (long) total_read;
} else {
if(total_read == 0) {
return MG_IO_ERR;
}
return (long) total_read;
}
}
// oops we filled the entire buffer; unread data may still remain!
return (long) total_read;
} This code has some issues
If you refer to the lwIP implementation [github.com/lwip-tcpip] of MbedTLS they also have a loop that continues reading until the read call returns an error code. I was reading about OpenSSL and it implies similar limitations [openssl.org] reading multiple records requires multiple calls to
|
Anyway, thanks for looking at the issue. I'm happy with the patch from my initial comment and will continue working with that. Let me know if you want me to try anything else in my test environment. |
Brian, we already have a loop in place, at the outer level, we will avoid inner loops as much as possible. cpq is busy with Embedded World now, he is the man and he also has a Mac, we will fix this issue as we always do; it may take a bit longer. |
@tubular-rheostat @scaprile thanks! I used Brian's test program, just modified it to use pcap instead of tun , just to avoid sudo-ing and ifconfig-in all the time. Then I changed it to use broker.hivemq.com:8883 instead of local mosquitto, cause local mosquitto did not start for whatever reasom. I applied Sergio's patch and it works OK (using ca.pem file from test/data):
The suggested patch looks good to me. @scaprile do you want to turn it into a PR? |
Thanks @cpq for looking at the issue again. I see some improvement but can still repro the issue. With master I didn't know about pcap I'll check out the example. |
@tubular-rheostat thanks. do we use the same connection when the script is executed the 2nd time? in other words, what's different the 2nd time? And, does it fail with built-in TLS? |
The same connection is used the 2nd time. From the steps in my demonstration code I just repeated step 11. |
@tubular-rheostat did you try the pcap example? |
@tubular-rheostat Please try #2890 and let us know |
Environment
If more than one TLS record is in the same TCP packet Mongoose will process only the first record, leaving the second in the buffer until another packet is received. A possible solution is to call
mbedtls_ssl_read
in a loop until it returnsMBEDTLS_ERR_SSL_WANT_READ
or similar value when there is no more data.Image from WireShark showing two TLS records in one TCP packet
My description that follows refers to MQTT but that is coincidental.
My test program is an MQTT client, it subscribes to
$SYS/broker/clients/connected
andtest_req
.That command connects to the broker and sends a message. Because I am subscribed to the broker's client count topic, this often causes two messages to be published: The updated client count as well as the message on
test_req
.I applied this patch to
7.13
and it resolved the issue:The text was updated successfully, but these errors were encountered: