[tor-commits] [tor/master] tls: Make buf_read_from_tls() read at most bytes

[dgoulet at torproject.org]

commit 3adabaf3e925f3ad395a2a0a2dbc92aa1d018ec4
Author: David Goulet <dgoulet at torproject.org>
Date:   Fri Jun 19 10:35:50 2020 -0400

    tls: Make buf_read_from_tls() read at most bytes
    
    The buf_read_from_tls() function was designed to read up to a certain number
    of bytes a TLS socket using read_to_chunk_tls() which boils down to SSL_read()
    (with OpenSSL, common case).
    
    However, at the end of the loop, the returned number of bytes from
    read_to_chunk_tls() was treated like the syscall read() for which if less
    bytes than the total asked are returned, it signals EOF.
    
    But, with SSL_read(), it returns up to a TLS record which can be less than
    what was asked. The assumption that it was EOF was wrong which made the while
    loop exiting before it was able to consume all requested bytes (at_most
    parameter).
    
    The general use case that Tor sees is that it will ask the network layer to
    give it at most 16KB (that is roughly 32 cells) but because of KIST scheduler,
    the highest possible TLS record we currently observe is 4096 bytes (4KB or 8
    cells). Thus the loop would at best always return 8 cells even though much
    more could be on the TLS socket. See ticket #40006 for more details.
    
    Fixes #40006
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 changes/ticket40006       | 6 ++++++
 src/lib/tls/buffers_tls.c | 5 +++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/changes/ticket40006 b/changes/ticket40006
new file mode 100644
index 000000000..ad10e236c
--- /dev/null
+++ b/changes/ticket40006
@@ -0,0 +1,6 @@
+  o Major bugfix (TLS, buffer):
+    - When attempting to read N bytes on a TLS connection, really try to read
+      those N bytes. Before that, Tor would stop reading after the first TLS
+      record which can be smaller than N bytes even though more data was waiting
+      on the TLS connection socket. The remaining data would have been read at
+      the next mainloop event. Fixes bug 40006; bugfix on 0.1.0.5-rc.
diff --git a/src/lib/tls/buffers_tls.c b/src/lib/tls/buffers_tls.c
index b92a14d6a..1b99467d2 100644
--- a/src/lib/tls/buffers_tls.c
+++ b/src/lib/tls/buffers_tls.c
@@ -59,6 +59,9 @@ read_to_chunk_tls(buf_t *buf, chunk_t *chunk, tor_tls_t *tls,
  * Second, the TLS stream's events do not correspond directly to network
  * events: sometimes, before a TLS stream can read, the network must be
  * ready to write -- or vice versa.
+ *
+ * On success, return the number of bytes read. On error, a TOR_TLS_* negative
+ * code is returned (expect any of them except TOR_TLS_DONE).
  */
 int
 buf_read_from_tls(buf_t *buf, tor_tls_t *tls, size_t at_most)
@@ -92,8 +95,6 @@ buf_read_from_tls(buf_t *buf, tor_tls_t *tls, size_t at_most)
       return r; /* Error */
     tor_assert(total_read+r <= BUF_MAX_LEN);
     total_read += r;
-    if ((size_t)r < readlen) /* eof, block, or no more to read. */
-      break;
   }
   return (int)total_read;
 }