Handling of null bytes in forms is problematic about rocket HOT 6 CLOSED

The same problem happens with the other form:

#[macro_use]
extern crate rocket;

use rocket::form::{Contextual, Form, Strict};
use rocket::serde::Serialize;


#[derive(Debug, FromForm, Serialize)]
#[serde(crate = "rocket::serde")]
struct KeyParam<'a> {
    pub key: Option<&'a str>,
}

#[post("/", data = "<p>")]
async fn bug_test2(p:Form<Strict<KeyParam<'_>>>,) -> String {
    match p.key {
        Some(v) => {
            if v.is_empty() {
                return "Is Empty".to_string();
            } else if v == "\0" {
                return "Is \\0".to_string();
            } else {
                return v.to_string();
            }
        }
        None => {
            return "Is None".to_string();
        }
    }
}

#[launch]
fn rocket() -> _ {
    rocket::build().mount("/", routes![bug_test2,])
}

Also returns "Is \0" instead of "Is Empty".

from rocket.

The behavior is perfectly correct according to the URL living standard, 5.1, which we follow here. The specifically relevant fragments from the spec are:

If bytes contains a 0x3D (=), then let name be the bytes from the start of bytes up to but excluding its first 0x3D (=), and let value be the bytes, if any, after the first 0x3D (=) up to the end of bytes. If 0x3D (=) is the first byte, then name will be the empty byte sequence. If it is the last, then value will be the empty byte sequence.

Let nameString and valueString be the result of running UTF-8 decode without BOM on the percent-decoding of name and value, respectively.

Clearly, the name and value can contain UTF-8, and that are not terminated by a null byte.

I'm no expert in "application/x-www-form-urlencoded", but looking at https://en.wikipedia.org/wiki/Percent-encoding it says "using only the US-ASCII characters legal within a URI"

You seem to be confusing x-www-form-urlencoded encoded data as part of a URL and as part of a payload. In the former case, other RFCs do indeed restrict the character set to ASCII visibles, and Rocket implements those RFCs precisely. In the latter case, the payload can contain any valid UTF-8, and x-www-form-urlencoded only defines its structure, namely how to parse out name/value pairs.

In the following example, the client can send "&key=\0" and the server will return "Is \0" instead of "Is Empty".

This is exactly correct, according to the standard. We need simple refer to the fragments I've called out above to verify this.

I feel that, by default (but optional), Rocket should trim up until the first \0 in a value as part of its sanitization. At least for "application/x-www-form-urlencoded".

This would be strictly incorrect and as such we simply cannot provide it as an option.

Doing otherwise can lead to bizarre behavior bugs because in complex programs (where not only Rust is involved) what C programs see and what Rust programs see will differ. Particularly, string length.

If a C program truncates name/value sequences from this context to the first null byte, than that program is incorrect. Clearly, name/value strings are not C-strings as otherwise they would be null-terminated.

This can lead to security bugs in applications.

Any disagreement about "what to look at" can lead to security bugs. Here, however, the standard is clear about what terminates a string and what doesn't, and we are following that standard. This is as secure as they come.

from rocket.

The behavior is perfectly correct according to the URL living standard, 5.1, which we follow here. The specifically relevant fragments from the spec are:

Clearly, the name and value can contain UTF-8, and that are not terminated by a null byte.

Ok.

You seem to be confusing x-www-form-urlencoded encoded data as part of a URL and as part of a payload. In the former case, other RFCs do indeed restrict the character set to ASCII visibles, and Rocket implements those RFCs precisely. In the latter case, the payload can contain any valid UTF-8, and x-www-form-urlencoded only defines its structure, namely how to parse out name/value pairs.

Ok. Fair enough.

Any disagreement about "what to look at" can lead to security bugs. Here, however, the standard is clear about what terminates a string and what doesn't, and we are following that standard. This is as secure as they come.

Poison Null Byte is a common attack.

I feel that at the very least this behavior should be documented.

Something along the lines:

# Security

It is valid & normal for the values returned by field_value in a form to contain null bytes ('\0') in
the middle of a string. Make sure to sanitize these values if you intend to use them as
input to APIs that expect a null-terminated string.

The same happens with the requests->forms section of the guide.

If you're ok with it I can submit a PR to add this phrase to the documentation.

from rocket.

It may also be worth adding an out of the box validator for it (untested code, may not compile):

fn validate_null_byte<'v>(txt: &str) -> form::Result<'v, ()> {
    let pos = txt.find('\0');
    match pos {
        Some(p) => {
            if p == txt.len() - 1 {
                Ok(())
            }
            else {
                Err(Error::validation("Null byte in nul-terminated string"))
            }
        },
        None => Ok(())
    }
}

from rocket.

Poison Null Byte is a common attack.

But that attack works precisely because the application is truncating where it shouldn't, where you suggest. The crux of the issue is that truncating at a null-byte outside of null-terminated strings is simply incorrect. Your link exemplifies this.

I feel that at the very least this behavior should be documented.

Something along the lines:

# Security

It is valid & normal for the values returned by field_value in a form to contain null bytes ('\0') in
the middle of a string. Make sure to sanitize these values if you intend to use them as
input to APIs that expect a null-terminated string.

I'm don't believe we should add this, nor a validator. In any context in which you're going from Rust to C strings (or other null-terminated strings), you're going to either use unsafe or use APIs which make this abundantly clear. See, for instance, the CStr APIs. This is where that documentation should (and currently does) live. In other words, there's nothing inherently special about null-terminated strings as far as Rocket or Rust is concerned - their special nature lies at FFI boundaries, which are incredibly unsafe with inherent security issues which we cannot hope to enumerate or control.

Furthermore, there's nothing inherently special about form field values -- any Rust string, and strings from other contexts, including URIs via percent-encoding, can contain null bytes -- and so documenting this here and only here has no particular value. For these reasons, I'm closing out this issue. I hope this makes sense.

from rocket.

I'm very dissatisfied by this answer.

But that attack works precisely because the application is truncating where it shouldn't, where you suggest. The crux of the issue is that truncating at a null-byte outside of null-terminated strings is simply incorrect. Your link exemplifies this.

I'm don't believe we should add this, nor a validator. In any context in which you're going from Rust to C strings (or other null-terminated strings), you're going to either use unsafe or use APIs which make this abundantly clear. See, for instance, the CStr APIs. This is where that documentation should (and currently does) live. In other words, there's nothing inherently special about null-terminated strings as far as Rocket or Rust is concerned - their special nature lies at FFI boundaries, which are incredibly unsafe with inherent security issues which we cannot hope to enumerate or control.

I find this line of reason arrogant.

Rust is a relatively new language. Googling about Rust is hard because documentation is scarce or not well indexed. Not everyone is deeply familiar with all the details of Rust and all the RFCs behind HTTP.

The HTTP protocol has been historically text-based. NUL is not part of valid text. No human-written text will ever contain NUL in the middle of a sentence. Even for machine generated sequences, it's not normal to send NUL in the middle of a sentence. No valid UTF-8 sequence will contain null bytes unless it's representing the literal NUL codepoint.
We are talking about strings, not arrays or vectors. Yes, Strings in Rust may contain anything, but they're meant to contain text. Your user may incorrectly assume Rocket already stopped at the first null byte and skipped through to the next '&' character for the next key.

Dealing with problematic byte sequences is the reason base64 was invented. Other ecosystems historically shielded their users from NUL characters in strings in server code.

You always, always have to treat your users in the documentation like they're 5 year old learning how to walk and eager to break everything.

In other words, there's nothing inherently special about null-terminated strings as far as Rocket or Rust is concerned.

That is saying "LOL, my code is bug free. It's your fault". Just because your code is free from security issues doesn't mean you don't have an obligation to inform your users of dangerous quirks that are predicted to cause problems.

Actually, this is Open Source. You're not getting paid to maintain it, strictly you don't have an obligation. But if that's your stance when a wanna-be contributor offers to add documentation for you, then this project should stay away from production and should remain as a pet project. I'm sorry to be this harsh.

I'm don't believe we should add this, nor a validator. In any context in which you're going from Rust to C strings (or other null-terminated strings), you're going to either use unsafe or use APIs

I was putting just one example. You don't know how this value is going to be handled. It may be sent to a SQL database, it may be sent to another server (with entirely different infrastructure), it may be sent to another client written in who-knows-what.

When two backends disagree on what they're seeing, at best they will result errors.

When that happens this either causes panics, which are costly to handle. Performance costs can result in Denial of Service.

Or this error is handled correctly (no panic), but it still results in errors delivered to the end-user which results in either confusion, potentially still exploitable for DoS, and many support headaches. It results in downtime and other costs.

At worst, these two backends end up behaving in unpredictable ways that end up causing lots of nightmares to debug and then fix the aftermath.

Furthermore, there's nothing inherently special about form field values -- any Rust string, and strings from other contexts, including URIs via percent-encoding, can contain null bytes -- and so documenting this here and only here has no particular value. For these reasons, I'm closing out this issue. I hope this makes sense.

Good point. I haven't realized that. %00 can be used to inject NUL inside a string in a GET. It's not just forms. I'm simply saying that this, what you just said (phrased slightly differently in a big colored box), should be stated in the requests section.

That it. They've been informed. If the user later messes up it's their fault. But they have been warned.

their special nature lies at FFI boundaries, which are incredibly unsafe with inherent security issues which we cannot hope to enumerate or control.

The crux of the problem here is that you're assuming the user knows about implementation details in your software.

Indeed we can't possibly enumerate every problem at FFI boundaries because by definition it's unknown and potentially limitless. But mentioning implementation details about your own API that are known to cause problems at FFI is a great start.

NUL bytes are incredibly problematic, big enough that they are worth mentioning.

Closing words

You're treating your users as if they're experts and know as much as you do, and are as familiar as you are with all the specs you've read and with the code you've written. They're not.

It's normal, I often incorrectly make those assumptions in my code that I write until someone points it out.

This is not a technical problem, it's a human problem (the biggest cause of security bugs by far).

Again, I am offering to submit a PR to add this information to the documentation. We may discuss the exact wording to use, but I strongly believe this quirk must be mentioned in the docs.

from rocket.