Protecting against buffer overflows

Brainstorming
1

As I create more and more buffers in my life, and I see all the CVEs caused by buffer overflows, I’ve been growing towards this pattern of working with buffers in Zig:

const buf: [512]u8,
const buf_len: u9,

// 1. An overflow guard
if (buf.len < input.len) {
    @branchHint(.unlikely);
    return error.BufferOverflow;
}
@memcpy(buf[buf_len..input.len], input);
// 2. A post length check by buf_len
buf_len += input.len;

Notice that maxInt(u9) == 511.
To me, if an input will be exactly 512 bytes for a 512-sized buffer, then it’s quite literally looking for trouble.

Also, I don’t use assert() because you never know what’s going to run in ReleaseFast. But I also want to give the compiler all the information I can so it can optimize as much as possible, hence the @branchHint.

I believe this pattern is significantly cheaper than Address Sanitization used in large programs (like Chrome) and enforced by the kernel through KASAN.

How am I not a genius with this? (no, really, poke holes in my bubble :slightly_smiling_face:).

1 Like
2

I think that branchHint is not neccessary as error paths should already be marked cold.
Imo here you prob should use writer instead. @memcpy is for scenarios where you know you can’t overflow.

var writer: std.Io.Writer = .fixed(&buf);
try writer.writeAll(input); // errors if overflow

Other option is to compile with ReleaseSafe and selectively set hot paths @setRuntimeSafety(false), but this of course makes just overflows panics.

4 Likes
3

Am I missing something? Can you clarify input? Can input.len be less than buf_len? The memcpy is confusing me.

Also, would most consider fuzz testing in debug with something like if (a < b) unreachable fairly robust? Maybe not for the space station? Otherwise, @branchHint seems valuable.

EDIT: I didn’t realize that “error paths should already be marked cold” - thanks @Cloudef

1 Like
4

Using .fixed definitely looks neater and more concise.

Thanks.

Why isn’t .fixed also better for cases where you can’t overflow?

5

It may produce more code in the binary (havent checked though :slight_smile: )

6

Your pattern is the late BoundedArray.
+1 for the team “Resurrect BoundedArray”.

To me, it looks like optimal usage of resources. Code appropriately (like by wrapping the unsafe code in a well-tested BoundedArray), and that won’t be a problem. You can add some extra bytes as “guard rails” in a debug build, set them to some sentinel, and check if they still hold their sentinel value later. Paranoia is debug builds. If you don’t trust your code enough to set it to release mode, that’s good sign it’s not ready for release.

2 Likes
7

Isn’t that what ReleaseSafe mode is for? E.g. builtin range checks without requiring manual intervention? E.g. since Zig has added all those additional semantics over C (slices and arrays carrying their length) to enable to automatic runtime checks. Why do the same thing manually again (like in C code) when the compiler automatically injects range checks anyway? Of course Zig will panic, but that’s the right thing to do for an ‘unintended’ buffer overflow.

For any ‘serious’ Zig applications I would also never consider to distribute them compiled in ReleaseFast mode, but only in ReleaseSafe mode (ReleaseFast should really be called ReleaseUnsafe, and ReleasSafe should just be called Release IMHO).

Instead of returning a soft error which then needs to be handled by the caller (how even? try with less and less data in a loop until the error goes away?) - I would instead add a function which returns the available buffer space, so that the caller can partition the data correctly without running into an unintended buffer overflow.

1 Like