mirror of
https://github.com/isc-projects/bind9.git
synced 2026-03-15 23:22:35 -04:00
84d71c8e2c
This commit changes TLS DNS so that partial writes by the SSL_write_ex() function are taken into account properly. Now, before doing encryption, we are flushing the buffers for outgoing encrypted data. The problem is fairly complicated and originates from the fact that it is somewhat hard to understand by reading the documentation if and when partial writes are supported/enabled or not, and one can get a false impression that they are not supported or enabled by default (https://www.openssl.org/docs/man3.1/man3/SSL_write_ex.html). I have added a lengthy comment about that into the code because it will be more useful there. The documentation on this topic is vague and hard to follow. The main point is that when SSL_write_ex() fails with SSL_ERROR_WANT_WRITE, the OpenSSL code tells us that we need to flush the outgoing buffers and then call SSL_write_ex() again with exactly the same arguments in order to continue as partial write could have happened on the previous call to SSL_write_ex() (that is not hard to verify by calling BIO_pending(sock->tls.app_rbio) before and after the call to SSL_write_ex() and comparing the returned values). This aspect was not taken into account in the code. Now, one can wonder how that could have led to the behaviour that we saw in the #4255 bug report. In particular, how could we lose one message and duplicate one twice? That is where things get interesting. One needs to keep two things in mind (that is important): Firstly, the possibility that two (or more) subsequent SSL_write_ex() calls will be done with exactly the same arguments is very high (the code does not guarantee that in any way, but in practice, that happens a lot). Secondly, the dnsperf (the software that helped us to trigger the bug) bombed the test server with messages that contained exactly the same data. The only difference in the responses is message IDs, which can be found closer to the start of a message. So, that is what was going on in the older version of the code: 1. During one of the isc_nm_send() calls, the SSL_write_ex() call fails with SSL_ERROR_WANT_WRITE. Partial writing has happened, though, and we wrote a part of the message with the message ID (e.g. 2014). Nevertheless, we have rescheduled the complete send operation asynchronously by a call to tlsdns_send_enqueue(). 2. While the asynchronous request has not been completed, we try to send the message (e.g. with ID 2015). The next isc_nm_send() or re-queued send happens with a call to SSL_write_ex() with EXACTLY the same arguments as in the case of the previous call. That is, we are acting as if we want to complete the previously failed SSL_write_ex() attempt (according to the OpenSSL documentation: https://www.openssl.org/docs/man3.1/man3/SSL_write_ex.html, the "Warnings" section). This way, we already have a start of the message containing the previous ID (2014 in our case) but complete the write request with the rest of the data given in the current write attempt. However, as responses differ only in message ID, we end up sending a valid (properly structured) DNS message but with the ID of the previous one. This way, we send a message with ID from the previous isc_nm_send() attempt. The message with the ID from the send request from this attempt will never be sent, as the code thinks that it is sending it now (that is how we send the message with ID 2014 instead of 2015, as in our example, thus making the message with ID 2015 never to be sent). 3. At some point later, the asynchronous send request (the rescheduled on the first step) completes without an error, sending a second message with the same ID (2014). It took exhausting SSL write buffers (so that a data encryption attempt cannot be completed in one operation) via long DoT streams in order to exhibit the behaviour described above. The exhaustion happened because we have not been trying to flush the buffers often enough (especially in the case of multiple subsequent writes). In my opinion, the origin of the problem can be described as follows: It happened due to making wrong guesses caused by poorly written documentation. |
||
|---|---|---|
| .. | ||
| include | ||
| netmgr | ||
| .gitignore | ||
| aes.c | ||
| app.c | ||
| assertions.c | ||
| astack.c | ||
| backtrace.c | ||
| base32.c | ||
| base64.c | ||
| buffer.c | ||
| commandline.c | ||
| condition.c | ||
| counter.c | ||
| crc64.c | ||
| dir.c | ||
| entropy.c | ||
| entropy_private.h | ||
| errno.c | ||
| errno2result.c | ||
| errno2result.h | ||
| error.c | ||
| event.c | ||
| file.c | ||
| glob.c | ||
| hash.c | ||
| heap.c | ||
| hex.c | ||
| hmac.c | ||
| ht.c | ||
| httpd.c | ||
| interfaceiter.c | ||
| iterated_hash.c | ||
| jemalloc_shim.h | ||
| lex.c | ||
| lib.c | ||
| log.c | ||
| Makefile.am | ||
| managers.c | ||
| md.c | ||
| mem.c | ||
| mem_p.h | ||
| meminfo.c | ||
| mutex.c | ||
| mutexblock.c | ||
| net.c | ||
| netaddr.c | ||
| netmgr_p.h | ||
| netscope.c | ||
| nonce.c | ||
| openssl_shim.c | ||
| openssl_shim.h | ||
| os.c | ||
| os_p.h | ||
| parseint.c | ||
| picohttpparser.c | ||
| picohttpparser.h | ||
| pool.c | ||
| portset.c | ||
| quota.c | ||
| radix.c | ||
| random.c | ||
| ratelimiter.c | ||
| regex.c | ||
| region.c | ||
| resource.c | ||
| result.c | ||
| rwlock.c | ||
| safe.c | ||
| serial.c | ||
| siphash.c | ||
| sockaddr.c | ||
| stats.c | ||
| stdio.c | ||
| stdtime.c | ||
| string.c | ||
| symtab.c | ||
| syslog.c | ||
| task.c | ||
| task_p.h | ||
| taskpool.c | ||
| thread.c | ||
| time.c | ||
| timer.c | ||
| timer_p.h | ||
| tls.c | ||
| tls_p.h | ||
| tm.c | ||
| trampoline.c | ||
| trampoline_p.h | ||
| url.c | ||
| utf8.c | ||