`std.crypto.pwhash` APIs are somewhat confusing

[BrainBlasted]

While working on a project I stumbled upon some oddities with the pwhash APIs. Particularly, I was working with the argon2 API. There are a multitude of ways to misuse this API. Some lead to runtime errors when they could be compiler errors, and others just lead to illegal behavior:

• Not providing an allocator -> runtime error
• Using the wrong encoding -> runtime error
• Using the output parameter -> storing incorrect bytes or causing illegal behavior (depending on how you initialized the buffer)
• Returning the returned slice -> illegal behavior

After presenting this on Zulip I was encouraged to file an issue for a broader discussion.

"Generic" APIs

Starting with the first two, the strHash() API for argon2 looks as such:

pub fn strHash(password: []const u8, options: HashOptions, out: []u8) Error![]const u8

Where HashOptions is a module-local struct:

pub const HashOptions = struct {
    allocator: ?mem.Allocator,
    params: Params,
    mode: Mode = .argon2id,
    encoding: pwhash.Encoding = .phc,
};

This immediately has two failure points when it comes to communicating the intended use of this API:

• allocator is nullable. But setting it to null produces the following runtime error:

error: AllocatorRequired
/var/home/chris/.local/share/zigup/0.14.0/files/lib/std/crypto/argon2.zig:599:48: 0x11a767f in strHash (zig_server)
    const allocator = options.allocator orelse return Error.AllocatorRequired;
                                               ^
/var/home/chris/Projects/zig-server/src/main.zig:54:20: 0x11a730c in strHash__anon_25440 (zig_server)
    const output = try argon2.strHash(password, .{
                   ^
/var/home/chris/Projects/zig-server/src/main.zig:31:24: 0x11afce0 in main (zig_server)
    const hashOutput = try strHash(allocator, password, .{

• Similarly, HashOptions.encoding being set to crypt is possible, but will produce this runtime error:

error: InvalidEncoding
/var/home/chris/.local/share/zigup/0.14.0/files/lib/std/crypto/argon2.zig:608:19: 0x11a7752 in strHash (zig_server)
        .crypt => return Error.InvalidEncoding,
                  ^
/var/home/chris/Projects/zig-server/src/main.zig:54:20: 0x11a7353 in strHash__anon_25440 (zig_server)
    const output = try argon2.strHash(password, .{
                   ^
/var/home/chris/Projects/zig-server/src/main.zig:31:24: 0x11afd20 in main (zig_server)
    const hashOutput = try strHash(allocator, password, .{

Both errors are returned by the strHash function:

    const allocator = options.allocator orelse return Error.AllocatorRequired;
    switch (options.encoding) {
        .phc => return PhcFormatHasher.create(
            allocator,
            password,
            options.params,
            options.mode,
            out,
        ),
        .crypt => return Error.InvalidEncoding,
    }

This seems to be done so strHash and strVerify can be used generically. This is a very uncommon use case. The only generic usage seems to be this benchmark code. On further inspection, the only strHash implementation that does not require an allocator is bcrypt - scrypt also returns an error when given null instead of an allocator.

The same issue is present for strVerify:

pub const VerifyOptions = struct {
    allocator: ?mem.Allocator,
};

pub fn strVerify(
    str: []const u8,
    password: []const u8,
    options: VerifyOptions,
) Error!void {
    const allocator = options.allocator orelse return Error.AllocatorRequired;
    return PhcFormatHasher.verify(allocator, str, password);
}

Conforming to a generic interface does not seem to serve these APIs well, and makes them more difficult to understand for very little benefit.

Buffer Output Param Ambiguity

Returning to the function signature for argon2's strHash:

pub fn strHash(password: []const u8, options: HashOptions, out: []u8) Error![]const u8

Myself and a few others were confused. This function takes an output parameter (called "out"), and returns a slice (or an error). The documentation states:

Compute a hash of a password using the argon2 key derivation function. The function returns a string that includes all the parameters required for verification.

There is no other context present. With no other context, there are a multitude of mistakes a developer may make. Lets start with the usage of the output parameter. Typical correct usage looks something like the following code:

var buffer: [128]u8 = std.mem.zeroes([128]u8);
const hashed_pass = try argon2.strHash(password, .{
    .allocator = allocator,
    .params = argon2.Params.owasp_2id,
}, &buffer);
// Store the hashed password
storeLogin(username, hashed_pass);

The developer creates a buffer with a comptime constant amount of bytes as its maximum size, calls argon2.strHash() with the password, options, and the buffer. They then store the returned slice. But this can very easily be misused in ways that cause invalid logic and/or illegal behavior.

Stack Use-After-Free

This is incorrect code because it causes a stack use-after-free, but it's incredibly easy for a naive developer to write a wrapper function like this:

fn hashPassword(allocator: Allocator, password: []const u8) ![]const u8 {
    var buffer: [128]u8 = std.mem.zeroes([128]u8);
    return argon2.strHash(password, .{
        .allocator = allocator,
        .params = argon2.Params.owasp_2id,
    }, &buffer);
}

The return value of strHash is not a newly allocated region of memory, but rather a temporary slice that represents the written portion of the buffer. This means that when you exit this function, the memory backing buffer -- and thus the slice -- is no longer safe to use. Using this function can result in reading/writing invalid bytes, breaking authentication or worse: causing a security vulnerability. The proposal to detect stack UAF at runtime would help the developer a bit if they wrote code like this, but I believe we can make an API that prevents this issue in the first place.

"Output" Parameter Leads To Incorrect Assumptions

Output parameters are commonly used in systems programming for a multitude of reasons. This specific instance of an output parameter is quite dangerous though. A common pattern has output parameters in place of return values, with the function's return value being something to indicate whether or not an error occurred.

For example, the libsodium C API looks as such (using an example from their docs):

#define PASSWORD "Correct Horse Battery Staple"

char hashed_password[crypto_pwhash_STRBYTES];

if (crypto_pwhash_str
    (hashed_password, PASSWORD, strlen(PASSWORD),
     crypto_pwhash_OPSLIMIT_SENSITIVE, crypto_pwhash_MEMLIMIT_SENSITIVE) != 0) {
    /* out of memory */
}

// store `hashed_password`

And has the following detailed documentation:

The output string is zero-terminated, includes only ASCII characters, and can be safely stored in SQL databases and other data stores. No extra information has to be stored to verify the password.

The Zig strHash() documentation provides no context on how to use the output parameter after the function is called, and a developer may reasonably assume that Zig is using a similar pattern to this libsodium API. Said developer may also think that the return value is simply there for convenience and is equivalent to the buffer after writing. This would be very incorrect, but there is nothing telling the developer this. Calling the parameter out and providing no context in the documentation leaves developers to fill in the blanks with the patterns they know. So consider the following variants of the usage example:

    var buffer: [128]u8 = std.mem.zeroes([128]u8);
    _ = try argon2.strHash(password, .{
        .allocator = allocator,
        .params = argon2.Params.owasp_2id,
    }, &buffer);
    storeLogin(username, buffer);

    var buffer: [128]u8 = undefined;
    _ = try argon2.strHash(password, .{
        .allocator = allocator,
        .params = argon2.Params.owasp_2id,
    }, &buffer);
    storeLogin(username, buffer);

In the first variant, the user will return a slice of bytes padded with zeros. This will fail to verify, but may otherwise be safe. The second variant will read from undefined memory, which is illegal behavior. The best case scenario would be the has failing to be stored and/or failing to verify. The worst case scenario would be a security vulnerability in password management code.

This is not some hypothetical - as part of reviewing the ecosystem and finding usage examples, I spotted a few cases of this mistake in the wild.

Ecosystem Review

Writing a wrapper that returns an allocated buffer for the hashed password seems to be a common pattern in the broader ecosystem. Both Jetzig and Bun have wrappers like this, in addition to others.

• Jetzig 1
• Jetzig 2
• Bun (Bun even has the cheeky note: "This is purposely simple because nobody asked to make it more complicated")
• Zui
• zig-say
• ndg

My Proposal

I believe the pwhash APIs would be better served by having functions that require allocation take an allocator, and returning an allocated slice to the hashed password instead of a temporary slice to an output parameter. This prevents misuse of the buffer, and allows easier use of the slice. They could also be improved by encoding the invariants into the types/function parameters directly. This is an example of what the argon2 API could look like:

const Argon2Options = struct {
    params: Params,
    mode: Mode = .argon2id,
    max_hash_len: usize,
};

fn strHash(allocator: Allocator, password: []const u8, comptime options: Argon2Options) ![]u8 {
    // ...
}

A usage example would look like:

const hashed_pass = try argon2.strHash(allocator, password, .{
    .params = argon2.Params.owasp_2id,
    .max_hash_len = 128,
});
defer allocator.free(hashed_pass);

storeLogin(username, hashed_pass);

There are fewer ways to misuse this variant. The caller does not need to manually initialize a buffer, thus they cannot read from a buffer padded with zeroes or containing undefined memory. You don't lose the comptime buffer length either, since options must be comptime-known.

storeLogin(user_name, buffer);

Is now a completely impossible mistake to make. In addition, return hashed_pass and:

return SomeStruct { .user_name = user_name, .hashed_pass = hashed_pass };

Will not immediately cause illegal behavior.

Managing heap memory can be error-prone too, but I think it lends itself to easier use in common cases. The most common mistake would become a memory leak. Heap UAF or double frees are possible when managing the allocated slice, but that's the case with any heap allocation. Zig also currently provides tooling that makes catching and preventing heap UAF, double frees, and memory leaks easier via DebugAllocator, arenas, etc. Currently stack UAF/dangling pointers are completely unchecked.

Alternative Proposal

Instead of returning a slice to the written portion of the buffer, the API could instead return the number of bytes written to the buffer. This would then be used to get the written slice of the buffer. This doesn't prevent the user from using the whole buffer incorrectly (storing it as the hashed password or using it past its lifetime), but it does remove the confusion from having both an output parameter and a slice as the return type.

[alexrp]

cc @jedisct1

[BrainBlasted]

Is there any interest in moving forward with the proposed solution? Is it blocked by anything?