std.crypto.tls.Client hangs on reading application data

[Southporter]

Zig Version

0.16.0-dev.463+f624191f9

Steps to Reproduce and Observed Behavior

This also happens in 0.15.1

1. Open a tcp connection to an imap server (i.e. imap.gmail.com:993).
2. Wrap this in a TLS client.
3. Try to read the server info from the client.

Example main.zig

const std = @import("std");
const repro = @import("repro");

const tls = std.crypto.tls;
const net = std.net;

var read_buf: [tls.Client.min_buffer_len]u8 = undefined;
var write_buf: [tls.Client.min_buffer_len]u8 = undefined;
var client_read_buf: [1024]u8 = undefined;
var client_write_buf: [1024]u8 = undefined;

pub fn main() !void {
    const allocator = std.heap.page_allocator;
    const stream = try net.tcpConnectToHost(allocator, "imap.gmail.com", 993);
    defer stream.close();

    var cipher_reader = stream.reader(&read_buf);
    var cipher_writer = stream.writer(&write_buf);
    var ca_bundle: std.crypto.Certificate.Bundle = .{};
    try ca_bundle.rescan(allocator);
    defer ca_bundle.deinit(allocator);

    var client = try tls.Client.init(cipher_reader.interface(), &cipher_writer.interface, .{
        .host = .{
            .explicit = "imap.gmail.com",
        },
        .ca = .{
            .bundle = ca_bundle,
        },
        .read_buffer = &client_read_buf,
        .write_buffer = &client_write_buf,
    });
    defer client.end() catch {};

    const server_info = try client.reader.takeDelimiterExclusive('\n');
    std.debug.print("Server info: {s}\n", .{server_info});
}

The client will hang waiting for information.

I ran this in lldb and found the issue may be related to readIndirect returning zero for application_data.

zig/lib/std/crypto/tls/Client.zig

Line 1278 in c50aa2b

.application_data => {

If I apply this patch to the Client, then it starts working. I'm not sure how this change would impact the http client though, as that my be doing other bookkeeping that would interfere here.

          .application_data => {
-             r.end += cleartext.len;
-             return 0;
+            return cleartext.len;
        },

Expected Behavior

TLS cleartext is processed correctly, does not hang, and does not skip application data.

[jedisct1]

Hi,

Do you have a test case to reproduce this?

[Rexicon226]

Do you have a test case to reproduce this?

It's a bit hidden, but if you click on the words "Example main.zig", you will find a repro. Fwiw, I reproduced it locally, it hangs for me

[jedisct1]

I think we should keep r.end += cleartext.len . But indeed, the function should return the number of bytes that have been processed.

[squeek502]

After looking into this a bit here, I think tls.Client.readIndirect is potentially a red herring.

As far as I can tell, what's happening is the readv call in the input File.Reader is the thing that's hanging, see linked comment above for some details. Could it be that the readv is just waiting for more data that will never come?

[Southporter]

Thanks everyone for looking into this

I think tls.Client.readIndirect is potentially a red herring.

I disagree, in fact, I thing the readv is ther red herring.

Let me see if I can narrate what is happening.

---

We call in to Reader.takeDelimeterInclusive.

This gets to tls.Client.readIndirect. readIndirect attempts to read from the ciphertext and gets the handshake message. It correctly handles it, doesn't write anything to the output and returns 0 bytes written. This is expected.

This gets back up to std.Io.Reader.peekDelimeterInclusive. Because it returned 0, it attempts to stream again.

This gets back to tls.Client.readIndirect which again attempts to read the ciphertext and decrypt it. This time it gets application data. It writes the cleartext to the output, updates the reader's end, and returns 0.

This gets back up to std.Io.Reader.peekDelimeterIncluesive. Because stream returns 0 again, it does not handle the data that has been written to the buffer. Instead it tries to call stream again.

This gets back to tls.Client.readIndircet which then calls into readv waiting for the server to send more data. It hangs here because the server won't send anymore data until we write to it.

---

Perhaps tls.Client.readIndirect is currently correct. It's interaction with Reader.peekDelimeterInclusive is the problem here. Because it returns 0, fill assumes that nothing has been written. If readIndirect does return n then fill correctly exits and returns the cleartext up for us to check for the delimeter. I think this would be the same for other api calls as well. It could be takeByte as well and still get the same issue.

[Southporter]

I think we should keep r.end += cleartext.len . But indeed, the function should return the number of bytes that have been processed.

I tried this, but this ends up causing problems too. Because Reader.peekDelimeterInclusive also updates r.end with the number, if we do return the number of bytes, it gets double counted and we end up with a 2x larger buffer.

[Southporter]

Further investigation. If you change the end lines of the repro to the following:

    const b = try client.reader.peekByte();
    std.debug.print("First byte: {x}\n", .{b});
    const server_info = try client.reader.takeDelimiterExclusive('\n');
    std.debug.print("Server info: {s}\n", .{server_info});

Then it works. peekByte fills the buffer correctly, and then, because there is already cleartext in the buffer, the takeDelimiterExclusive can read from the buffer.

[squeek502]

I think I found the root problem (the working peekByte alternative was a helpful clue). The problem is that peekDelimiterInclusive (called by takeDelimiterExclusive) doesn't handle its call to r.vtable.stream correctly when the delimiter is not found in the existing buffered data.

This code:

zig/lib/std/Io/Reader.zig

Lines 805 to 812 in bc4da9a

	const end_cap = r.buffer[r.end..];
	var writer: Writer = .fixed(end_cap);
	const n = r.vtable.stream(r, &writer, .limited(end_cap.len)) catch |err| switch (err) {
	error.WriteFailed => unreachable,
	else => |e| return e,
	};
	r.end += n;
	if (std.mem.indexOfScalarPos(u8, end_cap[0..n], 0, delimiter)) |end| {

effectively assumes that r.vtable.stream will return non-zero, since if r.vtable.stream does return 0, then it will always search for the delimiter in a 0-length slice ([0..n]). In other words, this function isn't taking into account this part of the stream doc comment:

zig/lib/std/Io/Reader.zig

Lines 42 to 45 in bc4da9a

	/// In addition to, or instead of writing to `w`, the implementation may
	/// choose to store data in `buffer`, modifying `seek` and `end`
	/// accordingly. Implementations are encouraged to take advantage of
	/// this if it simplifies the logic.

Will submit a PR with a fix shortly.