Skip to content

Commit 7cf2cbb

Browse files
committed
std.crypto.tls.Client.readvAdvanced: fix bugs
* When there is buffered cleartext, return it without calling the underlying read function. This prevents buffer overflow due to space used up by cleartext. * Avoid clearing the buffer when the buffered cleartext could not be completely given to the result read buffer, and there is some buffered ciphertext left. * Instead of rounding up the amount of bytes to ask for to the nearest TLS record size, round down, with a minimum of 1. This prevents the code path from being taken which requires extra memory copies. * Avoid calling `@memcpy` with overlapping arguments. closes #15590
1 parent 378264d commit 7cf2cbb

File tree

1 file changed

+29
-9
lines changed

1 file changed

+29
-9
lines changed

lib/std/crypto/tls/Client.zig

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -924,7 +924,9 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
924924
const amt = @intCast(u15, vp.put(partial_cleartext));
925925
c.partial_cleartext_idx += amt;
926926

927-
if (c.partial_ciphertext_end == c.partial_ciphertext_idx) {
927+
if (c.partial_cleartext_idx == c.partial_ciphertext_idx and
928+
c.partial_ciphertext_end == c.partial_ciphertext_idx)
929+
{
928930
// The buffer is now empty.
929931
c.partial_cleartext_idx = 0;
930932
c.partial_ciphertext_idx = 0;
@@ -935,7 +937,7 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
935937
c.partial_ciphertext_end = 0;
936938
assert(vp.total == amt);
937939
return amt;
938-
} else if (amt <= partial_cleartext.len) {
940+
} else if (amt > 0) {
939941
// We don't need more data, so don't call read.
940942
assert(vp.total == amt);
941943
return amt;
@@ -970,8 +972,8 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
970972
},
971973
};
972974

973-
// Cleartext capacity of output buffer, in records, rounded up.
974-
const buf_cap = (cleartext_buf_len +| (max_ciphertext_len - 1)) / max_ciphertext_len;
975+
// Cleartext capacity of output buffer, in records. Minimum one full record.
976+
const buf_cap = @max(cleartext_buf_len / max_ciphertext_len, 1);
975977
const wanted_read_len = buf_cap * (max_ciphertext_len + tls.record_header_len);
976978
const ask_len = @max(wanted_read_len, cleartext_stack_buffer.len);
977979
const ask_iovecs = limitVecs(&ask_iovecs_buf, ask_len);
@@ -1029,7 +1031,7 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
10291031
if (frag1.len < second_len)
10301032
return finishRead2(c, first, frag1, vp.total);
10311033

1032-
@memcpy(frag[0..in], first);
1034+
limitedOverlapCopy(frag, in);
10331035
@memcpy(frag[first.len..][0..second_len], frag1[0..second_len]);
10341036
frag = frag[0..full_record_len];
10351037
frag1 = frag1[second_len..];
@@ -1059,7 +1061,7 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
10591061
if (frag1.len < second_len)
10601062
return finishRead2(c, first, frag1, vp.total);
10611063

1062-
@memcpy(frag[0..in], first);
1064+
limitedOverlapCopy(frag, in);
10631065
@memcpy(frag[first.len..][0..second_len], frag1[0..second_len]);
10641066
frag = frag[0..full_record_len];
10651067
frag1 = frag1[second_len..];
@@ -1176,8 +1178,10 @@ pub fn readvAdvanced(c: *Client, stream: anytype, iovecs: []const std.os.iovec)
11761178
if (c.partial_ciphertext_idx > c.partial_cleartext_idx) {
11771179
// We have already run out of room in iovecs. Continue
11781180
// appending to `partially_read_buffer`.
1179-
const dest = c.partially_read_buffer[c.partial_ciphertext_idx..];
1180-
@memcpy(dest[0..msg.len], msg);
1181+
@memcpy(
1182+
c.partially_read_buffer[c.partial_ciphertext_idx..][0..msg.len],
1183+
msg,
1184+
);
11811185
c.partial_ciphertext_idx = @intCast(@TypeOf(c.partial_ciphertext_idx), c.partial_ciphertext_idx + msg.len);
11821186
} else {
11831187
const amt = vp.put(msg);
@@ -1223,22 +1227,38 @@ fn finishRead(c: *Client, frag: []const u8, in: usize, out: usize) usize {
12231227
return out;
12241228
}
12251229

1230+
/// Note that `first` usually overlaps with `c.partially_read_buffer`.
12261231
fn finishRead2(c: *Client, first: []const u8, frag1: []const u8, out: usize) usize {
12271232
if (c.partial_ciphertext_idx > c.partial_cleartext_idx) {
12281233
// There is cleartext at the beginning already which we need to preserve.
12291234
c.partial_ciphertext_end = @intCast(@TypeOf(c.partial_ciphertext_end), c.partial_ciphertext_idx + first.len + frag1.len);
1230-
@memcpy(c.partially_read_buffer[c.partial_ciphertext_idx..][0..first.len], first);
1235+
// TODO: eliminate this call to copyForwards
1236+
std.mem.copyForwards(u8, c.partially_read_buffer[c.partial_ciphertext_idx..][0..first.len], first);
12311237
@memcpy(c.partially_read_buffer[c.partial_ciphertext_idx + first.len ..][0..frag1.len], frag1);
12321238
} else {
12331239
c.partial_cleartext_idx = 0;
12341240
c.partial_ciphertext_idx = 0;
12351241
c.partial_ciphertext_end = @intCast(@TypeOf(c.partial_ciphertext_end), first.len + frag1.len);
1242+
// TODO: eliminate this call to copyForwards
12361243
std.mem.copyForwards(u8, c.partially_read_buffer[0..first.len], first);
12371244
@memcpy(c.partially_read_buffer[first.len..][0..frag1.len], frag1);
12381245
}
12391246
return out;
12401247
}
12411248

1249+
fn limitedOverlapCopy(frag: []u8, in: usize) void {
1250+
const first = frag[in..];
1251+
if (first.len <= in) {
1252+
// A single, non-overlapping memcpy suffices.
1253+
@memcpy(frag[0..first.len], first);
1254+
} else {
1255+
// Need two memcpy calls because one alone would overlap.
1256+
@memcpy(frag[0..in], first[0..in]);
1257+
const leftover = first.len - in;
1258+
@memcpy(frag[in..][0..leftover], first[in..][0..leftover]);
1259+
}
1260+
}
1261+
12421262
fn straddleByte(s1: []const u8, s2: []const u8, index: usize) u8 {
12431263
if (index < s1.len) {
12441264
return s1[index];

0 commit comments

Comments
 (0)