Mongoose accepts HTTP requests with invalid versions

[kenballus]

To see this for yourself,

0. Compile and run the following Mongoose HTTP server:

   
   // Copyright (c) 2020 Cesanta Software Limited
   // All rights reserved
   // Heavily modified by Ben Kallus.

include

include "mongoose.h"

static int const s_debug_level = MG_LL_INFO; static char const s_listening_address[] = "http://0.0.0.0:80";

// Handle interrupts, like Ctrl-C static int s_signo; static void signal_handler(int signo) { s_signo = signo; }

static size_t base64_encoded_len(size_t const plaintext_len) { // Base64-encoding takes 3n bytes to 4n bytes. // Then, there's padding, so we add on a correction term. // Note that we do not care about '\0' terminators here. return 4 * (plaintext_len / 3) + (plaintext_len % 3 != 0 ? 4 : 0); }

define INITIAL_BUF_SIZE (65535)

// buf: A pointer to a heap-allocated buffer (might be realloced) // buf_size: The size of buf (gets adjusted if buf is realloced) // buf_size_remaining: The number of free bytes at the end of buf (gets adjusted if *buf is realloced) // src: A buffer that we'll copy into buf // src_size: The number of bytes to copy from src to buf. void add_to_buffer(char *const buf_ptr, size_t const buf_size_ptr, size_t const buf_size_remaining_ptr, char const const src, size_t const src_size) { char buf = buf_ptr; size_t buf_size = buf_size_ptr; size_t buf_size_remaining = buf_size_remaining_ptr; while (buf_size_remaining < src_size) { // Is there enough room? char *const new_buf = realloc(buf, buf_size + INITIAL_BUF_SIZE); // If not, make buf INITIAL_BUF_SIZE bigger. if (new_buf == NULL) { // Did it work? puts("Out of memory!"); exit(1); // If not, fail. } buf = new_buf; // Update buf and friends. buf_size += INITIAL_BUF_SIZE; buf_size_remaining += INITIAL_BUF_SIZE; } memcpy(buf + (buf_size - buf_size_remaining), src, src_size); // Copy the data into the buffer buf_size_remaining -= src_size;

*buf_ptr = buf;                                                      // Fill the in-out args
*buf_size_ptr = buf_size;
*buf_size_remaining_ptr = buf_size_remaining;

}

static char B64_SCRATCH_SPACE[INITIAL_BUF_SIZE]; static char STRCAT_SCRATCH_SPACE[INITIAL_BUF_SIZE]; static char const PAYLOAD_START[] = "{\"headers\":["; static char const PAYLOAD_FIRST_HEADER_START[] = "[\""; static char const PAYLOAD_HEADER_START[] = ",[\""; static char const PAYLOAD_HEADER_MID[] = "\",\""; static char const PAYLOAD_HEADER_END[] = "\"]"; static char const PAYLOAD_BODY_BEGIN[] = "],\"body\":\""; static char const PAYLOAD_URI_BEGIN[] = "\",\"uri\":\""; static char const PAYLOAD_METHOD_BEGIN[] = "\",\"method\":\""; static char const PAYLOAD_VERSION_BEGIN[] = "\",\"version\":\""; static char const PAYLOAD_END[] = "\"}"; static void cb(struct mg_connection c, int ev, void ev_data) { if (ev != MG_EV_HTTP_MSG) { return; } struct mg_http_message parsed_request = {0}; parsed_request = (struct mg_http_message )ev_data; // mg_http_parse((char *)c->recv.buf, c->recv.len, &parsed_request);

char *buf = (char *)malloc(INITIAL_BUF_SIZE); // The buffer that contains the request.
size_t buf_size = INITIAL_BUF_SIZE;           // Its size
size_t buf_size_remaining = INITIAL_BUF_SIZE; // The amount of buf that's left
add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_START, sizeof(PAYLOAD_START) - 1);

for (int i = 0; i < MG_MAX_HTTP_HEADERS; i++) {
    if (parsed_request.headers[i].name.buf == NULL && parsed_request.headers[i].value.buf == NULL) { // Empty header signifies end of headers?
        break;
    }
    struct mg_http_header const curr = parsed_request.headers[i];
    if (i == 0) {
        add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_FIRST_HEADER_START, sizeof(PAYLOAD_FIRST_HEADER_START) - 1);
    } else {
        add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_HEADER_START, sizeof(PAYLOAD_HEADER_START) - 1);
    }

    // Name
    if (base64_encoded_len(curr.name.len) + 1 > sizeof(B64_SCRATCH_SPACE)) { // + 1 because mg_base64_encode adds a null
        mg_http_reply(c, 400, "", "Header name too long");
        free(buf);
        return;
    }
    mg_base64_encode((unsigned char *)curr.name.buf, curr.name.len, B64_SCRATCH_SPACE, sizeof(B64_SCRATCH_SPACE));
    add_to_buffer(&buf, &buf_size, &buf_size_remaining, B64_SCRATCH_SPACE, base64_encoded_len(curr.name.len));
    add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_HEADER_MID, sizeof(PAYLOAD_HEADER_MID) - 1);

    // Value
    if (base64_encoded_len(curr.value.len) + 1 > sizeof(B64_SCRATCH_SPACE)) { // + 1 because mg_base64_encode adds a null
        mg_http_reply(c, 400, "", "Header value too long");
        free(buf);
        return;
    }
    mg_base64_encode((unsigned char *)curr.value.buf, curr.value.len, B64_SCRATCH_SPACE, sizeof(B64_SCRATCH_SPACE));
    add_to_buffer(&buf, &buf_size, &buf_size_remaining, B64_SCRATCH_SPACE, base64_encoded_len(curr.value.len));
    add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_HEADER_END, sizeof(PAYLOAD_HEADER_END) - 1);
}

// Body
if (base64_encoded_len(parsed_request.body.len) + 1 > sizeof(B64_SCRATCH_SPACE)) {
    mg_http_reply(c, 400, "", "Request body too large");
    free(buf);
    return;
}
add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_BODY_BEGIN, sizeof(PAYLOAD_BODY_BEGIN) - 1);
mg_base64_encode((unsigned char *)parsed_request.body.buf, parsed_request.body.len, B64_SCRATCH_SPACE, sizeof(B64_SCRATCH_SPACE));
add_to_buffer(&buf, &buf_size, &buf_size_remaining, B64_SCRATCH_SPACE, base64_encoded_len(parsed_request.body.len));

// URI
if (parsed_request.uri.len + 1 + parsed_request.query.len + 1 > sizeof(STRCAT_SCRATCH_SPACE)) {
    mg_http_reply(c, 400, "", "Request URI too long");
    free(buf);
    return;
}
size_t uri_len = 0;
STRCAT_SCRATCH_SPACE[uri_len] = '\0';
if (parsed_request.uri.len > 0) {
    memcpy(STRCAT_SCRATCH_SPACE, parsed_request.uri.buf, parsed_request.uri.len);
    uri_len += parsed_request.uri.len;
    STRCAT_SCRATCH_SPACE[uri_len] = '\0';
}
if (parsed_request.query.len > 0) {
    STRCAT_SCRATCH_SPACE[uri_len] = '?';
    uri_len++;
    memcpy(STRCAT_SCRATCH_SPACE + uri_len, parsed_request.query.buf, parsed_request.query.len);
    uri_len += parsed_request.query.len;
    STRCAT_SCRATCH_SPACE[uri_len] = '\0';
}
if (base64_encoded_len(uri_len) + 1 > sizeof(B64_SCRATCH_SPACE)) {
    mg_http_reply(c, 400, "", "Request URI too long");
    free(buf);
    return;
}
add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_URI_BEGIN, sizeof(PAYLOAD_URI_BEGIN) - 1);
mg_base64_encode((unsigned char *)STRCAT_SCRATCH_SPACE, uri_len, B64_SCRATCH_SPACE, sizeof(B64_SCRATCH_SPACE));
add_to_buffer(&buf, &buf_size, &buf_size_remaining, B64_SCRATCH_SPACE, base64_encoded_len(uri_len));

// Method
if (base64_encoded_len(parsed_request.method.len) + 1 > sizeof(B64_SCRATCH_SPACE)) {
    mg_http_reply(c, 400, "", "Request method too long");
    free(buf);
    return;
}
add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_METHOD_BEGIN, sizeof(PAYLOAD_METHOD_BEGIN) - 1);
mg_base64_encode((unsigned char *)parsed_request.method.buf, parsed_request.method.len, B64_SCRATCH_SPACE, sizeof(B64_SCRATCH_SPACE));
add_to_buffer(&buf, &buf_size, &buf_size_remaining, B64_SCRATCH_SPACE, base64_encoded_len(parsed_request.method.len));

// Version
if (base64_encoded_len(parsed_request.method.len) + 1 > sizeof(B64_SCRATCH_SPACE)) {
    mg_http_reply(c, 400, "", "Request method too long");
    free(buf);
    return;
}
add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_VERSION_BEGIN, sizeof(PAYLOAD_VERSION_BEGIN) - 1);
mg_base64_encode((unsigned char *)parsed_request.proto.buf, parsed_request.proto.len, B64_SCRATCH_SPACE, sizeof(B64_SCRATCH_SPACE));
add_to_buffer(&buf, &buf_size, &buf_size_remaining, B64_SCRATCH_SPACE, base64_encoded_len(parsed_request.proto.len));

add_to_buffer(&buf, &buf_size, &buf_size_remaining, PAYLOAD_END, sizeof(PAYLOAD_END));

mg_http_reply(c, 200, "Content-Type: application/json\r\n", "%s", buf);
free(buf);

}

int main(void) { // Initialise stuff signal(SIGINT, signal_handler); signal(SIGTERM, signal_handler); mg_log_set(s_debug_level);

struct mg_mgr mgr;
mg_mgr_init(&mgr);

struct mg_connection *c = mg_http_listen(&mgr, s_listening_address, cb, NULL);
if (c == NULL) {
    MG_ERROR(("Cannot listen on %s. Use http://ADDR:PORT or :PORT", s_listening_address));
    exit(EXIT_FAILURE);
}

// Start infinite event loop
MG_INFO(("Mongoose version : v%s", MG_VERSION));
MG_INFO(("Listening on     : %s", s_listening_address));
while (s_signo == 0) {
    mg_mgr_poll(&mgr, 1000);
}
mg_mgr_free(&mgr);
MG_INFO(("Exiting on signal %d", s_signo));
return 0;

}

1. Run the following to send it a request with an invalid version, and extract it out of the response, indicating that the invalid version made through validation in the server:
```sh
echo $(printf 'GET / Invalid!!\r\nHost: whatever\r\n\r\n' \
     | nc localhost 80 \
     | python3 -c 'import sys; s = sys.stdin.buffer.read(); sys.stdout.buffer.write(s[s.index(b"\r\n\r\n") + 4:])' \
     | jq '.["version"]' \
     | sed 's/"//g' \
     | base64 -d \
)

Invalid!!

This is unexpected, because RFC 9112 specifies that HTTP versions must match the following ABNF rule:

  HTTP-version  = HTTP-name "/" DIGIT "." DIGIT
  HTTP-name     = %s"HTTP"

This is equivalent to the following regex: HTTP/[0-9]\.[0-9]. The ideal fix here would be to reject HTTP versions that do not match this regex.

Environment

• mongoose version: master (6bb40e6ec96f95bfd36a816b430ea2726fac9d05)
• Compiler: GCC 13.2.0
• Platform: AMD64 Linux

[cpq]

@kenballus thank you!

1. Mongoose was NEVER meants RFC compliant.
2. Does this issue lead to a security threat?
3. Would fixing this add any value to the end-user? I don't think it would, but please let us know if you think otherwise.

[kenballus]

I can't think of any immediate security implications of this behavior. If Mongoose isn't attempting to be compliant with the standards, then I won't report issues of this nature in the future.

[cpq]

@kenballus thank you!

Yes we are not interested in the standard compliance just for the sake of it, which do not contribute to the usability or security. Most of your reports do contribute to those, so if you continue reporting, we'd much appreciate it.

[kenballus]

Yes we are not interested in the standard compliance just for the sake of it, which do not contribute to the usability or security. Most of your reports do contribute to those, so if you continue reporting, we'd much appreciate it.

Got it; thanks.

A final note though:

Standards compliance is, of course, meaningless in-and-of-itself. However, in protocols like HTTP, where messages are written and rewritten as they pass through chains of gateways, the effects are parsing discrepancies become difficult to predict. Standards compliance is a good way to ensure that these discrepancies are not exploitable, because the standards authors try to ensure that compliant servers are not vulnerable to this sort of thing.

For instance, Mongoose allows ` (SP) characters within versions, but many servers don't. Similarly, many servers allow ` (SP) characters within paths, but Mongoose doesn't.

Thus, when Mongoose receives the following request line,

GET / HTTP/1.1 HTTP/1.1

... it sees the path as /, and the version as HTTP/1.1 HTTP/1.1.

When some other servers (Lighttpd, Libevent, FastHTTP) receive the same request line, they see the path as / HTTP/1.1 and the version as HTTP/1.1. If a cache server interpreted the message in this second way, and forwarded the message to Mongoose, that's a vector for cache poisoning.

In short, it's difficult to predict when these discrepancies matter. The best we can do is comply with the standards, so that if these discrepancies matter in the future, we can safely say that we did the best we could.

For this reason, I've changed my mind about this one; it probably does matter.

[cpq]

@kenballus thank you.

Please forgive me my arrogance. I am trying to wrap my head around this, and still don't understand how gateways and Mongoose play together here. Mongoose is at the end of the request chain, correct? How the fact it accepts a bad version, is an attack vector? Could you give a example please?

I can see:

1. Scenario 1. A client sends a requests with bad version to Mongoose, there are no gateways in between, Mongoose sends a response, everyone is happy.
2. Scenario 2. A client sends a requests with bad version to Mongoose, there IS gateway in between, which does .. what? And if that request eventually hits Mongoose, Mongoose responds, then what?

[kenballus]

Please forgive me my arrogance.

And please forgive mine too :)

I'll try to make it more concrete. Let's say we have a setup like this:

Mongoose <-> Gateway <-> Client

Suppose that the gateway has a rule specifying that requests for /.ssh/id_rsa should be blocked.

If a client sends the following request to the caching gateway:

GET /.ssh/id_rsa whatever HTTP/1.1\r\n
Host: whatever\r\n
\r\n

The gateway sees the request as having URI=/.ssh/id_rsa whatever, version=HTTP/1.1 and forwards the request to Mongoose. This doesn't trip the rule because, from the gateway's perspective, the request is asking for /.ssh/id_rsa whatever, not /.ssh/id_rsa.

Mongoose sees the request as having the URI /.ssh/id_rsa, version=whatever HTTP/1.1, so it responds with the content of .ssh/id_rsa.

Hopefully it's clear why this is a problem.

[cpq]

According to https://datatracker.ietf.org/doc/html/rfc2616#section-5.1, the valid request is this:

Method SP Request-URI SP HTTP-Version CRLF

According to that, GET /.ssh/id_rsa whatever HTTP/1.1\r\n request is invalid, as it has 4 items, whereas only 3 are allowed.

Am I reading that correctly? If we patch Mongoose to allow only 3 space-separated items (per standard), would it solve this issue?

[kenballus]

If we patch Mongoose to allow only 3 space-separated items (per standard), would it solve this issue?

That is one fix for the problem described above. An easier (and more correct) solution would be to enforce that received HTTP versions always match HTTP/[0-9]\.[0-9].

[cpq]

A request is no version is also correct (https://serverfault.com/questions/1130764/is-get-a-valid-http-request) , so I guess we could check the version - it could be either empty, or match HTTP/x.y

[kenballus]

A request with no version is correct HTTP/0.9, but not correct HTTP/1.0 or 1.1. If you want to support all three protocols, then yes, empty versions should also be permitted.