Skip to content

Commit

Permalink
std.crypto.tls.Client: make close_notify optional
Browse files Browse the repository at this point in the history
Although RFC 8446 states:

> Each party MUST send a "close_notify" alert before closing its write
> side of the connection

In practice many servers do not do this. Also in practice, truncation
attacks are thwarted at the application layer by comparing the amount of
bytes received with the amount expected via the HTTP headers.
  • Loading branch information
andrewrk committed Jan 3, 2023
1 parent 9ca6d67 commit 7178451
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 2 deletions.
18 changes: 16 additions & 2 deletions lib/std/crypto/tls/Client.zig
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ partial_ciphertext_end: u15,
/// When this is true, the stream may still not be at the end because there
/// may be data in `partially_read_buffer`.
received_close_notify: bool,
/// By default, reaching the end-of-stream when reading from the server will
/// cause `error.TlsConnectionTruncated` to be returned, unless a close_notify
/// message has been received. By setting this flag to `true`, instead, the
/// end-of-stream will be forwarded to the application layer above TLS.
/// This makes the application vulnerable to truncation attacks unless the
/// application layer itself verifies that the amount of data received equals
/// the amount of data expected, such as HTTP with the Content-Length header.
allow_truncation_attacks: bool = false,
application_cipher: tls.ApplicationCipher,
/// The size is enough to contain exactly one TLSCiphertext record.
/// This buffer is segmented into four parts:
Expand Down Expand Up @@ -900,8 +908,14 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
const ask_iovecs = limitVecs(&ask_iovecs_buf, ask_len);
const actual_read_len = try stream.readv(ask_iovecs);
if (actual_read_len == 0) {
// This is either a truncation attack, or a bug in the server.
return error.TlsConnectionTruncated;
// This is either a truncation attack, a bug in the server, or an
// intentional omission of the close_notify message due to truncation
// detection handled above the TLS layer.
if (c.allow_truncation_attacks) {
c.received_close_notify = true;
} else {
return error.TlsConnectionTruncated;
}
}

// There might be more bytes inside `in_stack_buffer` that need to be processed,
Expand Down
10 changes: 10 additions & 0 deletions lib/std/http/Client.zig
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
//! This API is a barely-touched, barely-functional http client, just the
//! absolute minimum thing I needed in order to test `std.crypto.tls`. Bear
//! with me and I promise the API will become useful and streamlined.

const std = @import("../std.zig");
const assert = std.debug.assert;
const http = std.http;
Expand All @@ -10,6 +14,9 @@ headers: std.ArrayListUnmanaged(u8) = .{},
active_requests: usize = 0,
ca_bundle: std.crypto.Certificate.Bundle = .{},

/// TODO: emit error.UnexpectedEndOfStream or something like that when the read
/// data does not match the content length. This is necessary since HTTPS disables
/// close_notify protection on underlying TLS streams.
pub const Request = struct {
client: *Client,
stream: net.Stream,
Expand Down Expand Up @@ -133,6 +140,9 @@ pub fn request(client: *Client, url: Url, options: Request.Options) !Request {
.http => {},
.https => {
req.tls_client = try std.crypto.tls.Client.init(req.stream, client.ca_bundle, url.host);
// This is appropriate for HTTPS because the HTTP headers contain
// the content length which is used to detect truncation attacks.
req.tls_client.allow_truncation_attacks = true;
},
}

Expand Down

0 comments on commit 7178451

Please sign in to comment.