`index` in `share` can bypass `types` restrictions

[mqudsi]

There is a possible security issue: the presence of index in a share section bypasses the mime types restrictions set in types. This leaks source code for some application types.

Example:

    "routes": {
        "myapp": [
            {
                "match": {
                    "uri": [
                        "!.well-known/*",
                        "~.*/\\..*"
                    ]
                },
                "action": {
                    "return": 403
                }
            },
            {
                "match": {
                    "uri": [ "*.php", "*.php/*" ]
                },
                "action": {
                    "pass": "applications/myapp/direct"
                }
            },
            {
                "match": {
                    "uri": "app/*"
                },
                "action": {
                    "share": "`/var/www/myapp/${uri.substring(3)}`",
                    "index": "index.php",
                    "types": [ "!application/x-httpd-php" ],
                    "fallback": {
                        "pass": "applications/myapp/main"
                    }
                }
            },

A request for /app/dir1 will return the raw source code for /app/dir1/index.php if such a file exists, even though the share section has a mime type filter that is supposed to restrict it to non-php mime types.

[tippexs]

Thanks for the PR and the additonal context @alejandro-colomar we will have a look on the issues @mqudsi and let you know what are the next steps.

[tippexs]

@mqudsi Can you please send a Unit log with the log_route settings enabled and an example curl request to reproduce.

To enable the router logging use the following settings Object

"settings": {"http": {"log_route": true}}

I had to make quite some changes to reproduce this on my end. The match will find the uri only with a * or a leading /. As you can see in my test my routes uses /app/* instead of app/*. Please validate this on your side.

The overall issue is clear. The types filtering must be part of the response processing in Unit. No matter the configuration. To me, the configuration example provided to test this case is very constructed BUT it demonstrated the issue quite well.

Let me share my thoughts about the share-configuration and why it is not an security issue.

"action": {
    "share": "`/tmp/myapp/${uri.substring(4)}`",
    "index": "index.php",
    "types": [
      "!application/x-httpd-php"
    ],
    "fallback": {
      "return": 499
    }

The default is index.html for shares and not index.php. As we are trying to share the directory by requesting curl -v 127.1/app/dir1/ Unit will use the fallback index.php and shares this file. By requesting index.php we would use the configured php route and send the request to an application.

{
  "match": {
    "uri": [
      "*.php",
      "*.php/*"
    ]
  },
  "action": {
    "pass": "applications/myapp/direct"
  }
}

I do agree we must respect the MIME-Filter before sending data back to the client. In this case, even if actively configured by the user Unit should not send the index.php file. My tests showed that we are setting the correct Content-Type in the router. See this response as a reference:

❯ curl -v 127.0.0.1:80/app/dir1/
*   Trying 127.0.0.1:80...
* Connected to 127.0.0.1 (127.0.0.1) port 80
> GET /app/dir1/ HTTP/1.1
> Host: 127.0.0.1
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Last-Modified: Mon, 22 Jan 2024 09:31:33 GMT
< ETag: "65ae27e5-14"
< Content-Type: application/x-httpd-php
< Server: Unit/1.31.1
< Date: Mon, 22 Jan 2024 09:03:45 GMT
< Content-Length: 20
<
<?php echo "test" ?>
* Connection #0 to host 127.0.0.1 left intact

Thanks for reporting this and we will have a look into the PR and come back to you asap.

[alejandro-colomar]

@tippexs

@mqudsi Can you please send a Unit log with the log_route settings enabled and an example curl request to reproduce.

I haven't tried, but the following should reproduce the bug. And it should reproduce it even before adding the configurable index, back when @hongzhidao introduced the bug.

"index": "index.html",
"types": ["!text/html"],

The default is index.html for shares and not index.php. As we are trying to share the directory by requesting curl -v 127.1/app/dir1/ Unit will use the fallback index.php and shares this file. By requesting index.php we would use the configured php route and send the request to an application.

This is not correct. The server will statically serve the index.php file, just as any other file.

Just by naming a file .php it won't magically run as a PHP application. If it's served via "share" it's a static file.

And I agree with the OP, that this can be a security issue.

[hongzhidao]

Hi @mqudsi

A request for /app/dir1 will return the raw source code for /app/dir1/index.php if such a file exists, even though the share section has a mime type filter that is supposed to restrict it to non-php mime types.

Is this a real-world requirement for you? Or is it just a test to check how Unit work with index and share?

The current logic isindex is to show the files in some directory, and types is to filter requests. The types don't filter specified index. And the default index is index.html. Let us know if you have a more detailed read-world example to check if the current behavior is not good enough.

[alejandro-colomar]

@hongzhidao

This is a regression introduced by you, presumably by accident, in one of those huge commits that sneak several unnecessary and dangerous refactors in.

The regression was introduced in c5220944d2ac ("Static: variables in the "share" option.").

$ git describe --contains c5220944d2ac
1.26.0~45

The logic you mention hasn't always been like that. The original behavior is the one that the OP expected, and which I would also expect intuitively.

Below is proof of the original behavior:

$ git show c5220944d2ac^:src/nxt_http_static.c | grepc nxt_http_static_send_ready
(standard input):static void nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data);
(standard input):static void
nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
{
    ...

    if (r->path->start[r->path->length - 1] == '/') {
        /* TODO: dynamic index setting. */
        nxt_str_set(&index, "index.html");
        nxt_str_set(&exten, ".html");

    } else {
        nxt_str_set(&index, "");
        nxt_str_null(&exten);
    }

    f = NULL;
    status = NXT_HTTP_INTERNAL_SERVER_ERROR;

    rtcf = r->conf->socket_conf->router_conf;

    mtype = NULL;

    if (conf->types != NULL && exten.start == NULL) {
        nxt_http_static_extract_extension(r->path, &exten);
        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

        ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
                                       mtype->length);
        if (nxt_slow_path(ret == NXT_ERROR)) {
            goto fail;
        }

        if (ret == 0) {
            status = NXT_HTTP_FORBIDDEN;
            goto fail;
        }
    }

    ...
}

---

Edit:

Now I realize this is not a regression, and the original behavior was as it is now. Just a bit weird.

[hongzhidao]

The feature was introduced with the logic I mentioned. https://github.com/nginx/unit/commit/b9d5eb285a2fca8b9b60d948acc679a5e16b3f94 And it has related tests.

[hongzhidao]

Hi @alejandro-colomar

This is a regression introduced by you, presumably by accident, in one of those huge commits that sneak several unnecessary and dangerous refactors in.

It is not a regression. It was introduced in the initial implementation of the feature with that behavior already. It was just that the sneaky refactor looked very suspicious, but it was a false positive.

And another rabbit hole. I bet this rabbit is of the security subspecies. Hi,

Below is a diff of nxt_http_static_send_ready() by the commit https://github.com/nginx/unit/commit/c5220944d2acdb912c129fc82ac8a83d24e9845d.

It's quite unreadable (thanks for embedding a refactor there), so let's see the code before and after.

The code for non-index cases, which for other reasons assumes 'shr' is null-terminated, in this call assumes it's not null terminated. Yes, it's quite buggy in that regard. That was a bug introduced in https://github.com/nginx/unit/commit/c5220944d2acdb912c129fc82ac8a83d24e9845d ("Static: variables in the "share" option.").

I didn't find issues on the commit, and it has unit tests that passed. I can't understand why you explicitly claim the commit https://github.com/nginx/unit/commit/c5220944d2acdb912c129fc82ac8a83d24e9845d several times with some words that sound serious and misleading.

Did you notice that you might be introducing worse performance and code readability?

path = nxt_str_alloc(r->mem_pool, length);
    if (nxt_slow_path(path == NULL)) {
        goto fail;
    }

... 

exten->length = end - p - 1;

Take the most common configuration as an example:

"share": ".../$uri"

And most of the requests have an extern for example, /index.php. In your new way:

1. Unit has to allocate memory twice. The current way only allocates once.
2. Developers who are interested in source code would be confused about number 1 in the tricky end - p - 1. They usually read the source directly without reading the commit message.

[VBart]

This is an intentional behavior:

1. It doesn't make sense to restrict a file that user EXPLICITLY wrote in the static file serving configuration (note that "index" is about static file sharing, nothing else)
2. It allows to configure Unit to serve only directory indexes and protect any other files inside
3. Serving PHP sources is an issue only if a user had no intention to do so. But in this particular case user clearly stated his intention by specifying PHP file in the directive related to static file serving.

Actually, serving sources of PHP files can be useful not only for sharing the code, but also to store files on one instance and run them on another. Particular configuration allows to serve only directory index and protects any other PHP files in the directory. This can be a possible use case.

By fixing "this bug" you will change behavior and can break some existing configurations. Moreover, this behavior is explicitly documented, quote:

If a share path specifies only the directory name, Unit doesn’t apply MIME filtering.

Has anyone read the documentation? =)

[alejandro-colomar]

Hi @VBart !

Thanks for having a look at this.

This is an intentional behavior:

1. It doesn't make sense to restrict a file that user EXPLICITLY wrote in the static file serving configuration (note that "index" is about static file sharing, nothing else)

Yeah, with hardcoded index files it seems a bit silly. I was thinking of variables in the "index" configuration, but in the end we didn't add that. If the index would be generated via some NJS or some variable, it would be possible that the index file wasn't the intended one. Also, if action chaining is ever implemented, it could also result in similar problems that an index file wasn't intended in some specific route.

2. It allows to configure Unit to serve only directory indexes and protect any other files inside

Hmmm, this is interesting. You could do the same thing with a "match" "uri" restriction, and check that the URI end in /. But you have a point.

3. Serving PHP sources is an issue only if a user had no intention to do so. But in this particular case user clearly stated his intention by specifying PHP file in the directive related to static file serving.

Actually, serving sources of PHP files can be useful not only for sharing the code, but also to store files on one instance and run them on another. Particular configuration allows to serve only directory index and protects any other PHP files in the directory. This can be a possible use case.

Yep, this could be reasonable. While you could also get this with some "match" "uri", it would probably get tricky to write the rule. Having a directory index could make it simpler. I still have concerns about what will happen when Unit becomes a bit more complex, as I said above.

By fixing "this bug" you will change behavior and can break some existing configurations.

Yeah, that would need to be considered.

Moreover, this behavior is explicitly documented, quote:

If a share path specifies only the directory name, Unit doesn’t apply MIME filtering.

Has anyone read the documentation? =)

=)

Still, there is a bug in some other code that Zhidao changed during some refactor, where a string should have a terminating null byte, but didn't. I found that bug while editing this code. That is fixed in the first patches of the PR.

Cheers, Alex

[alejandro-colomar]

Did you notice that you might be introducing worse performance

Yes. Is that performance difference meaningful? Probably not.

and code readability?

Not really. I think code is more readable after the fix, because what you read is what you expect. Before my fix, one can be confused because certain parts of that code assume the string is terminated, and other parts of the code assume it's not terminated. That's very confusing.

Also, I made some calls unconditional, which simplifies the code mentally, even if performance has a small degradation (probably negligible, but there is).

path = nxt_str_alloc(r->mem_pool, length);
    if (nxt_slow_path(path == NULL)) {
        goto fail;
    }

... 

exten->length = end - p - 1;

Take the most common configuration as an example:

"share": ".../$uri"

And most of the requests have an extern for example, /index.php. In your new way:

1. Unit has to allocate memory twice. The current way only allocates once.

The current way is a bug waiting to be triggered. shr->start is being passed to open(2), but shr->length tells us there's no terminating null byte. The behavior is undefined. For some reason there's a 0 in the right place in memory, and the bug didn't trigger.

2. Developers who are interested in source code would be confused about number 1 in the tricky `end - p - 1`. They usually read the source directly without reading the commit message.

That's easy. When you have doubts about some code, check why it was changed, when, and how:

$ git blame HEAD -- src/nxt_http_static.c | grep 'end - p - 1'
019b0814b (Alejandro Colomar 2024-01-23 16:13:33 +0100  779)     exten->length = end - p - 1;
$ git log -1 019b0814b | head
commit 019b0814b0ab937a9c944b04edcd11e66dbe55e8
Author: Alejandro Colomar <alx@kernel.org>
Date:   Tue Jan 23 16:13:33 2024 +0100

    Static: Don't include the terminating null byte in exten

    The 'path' is (or should be) terminated by a null byte ('\0').  But the
    code reading 'exten' assumes there's no null byte.  Thus, we must reduce
    the length of that string by 1.

[VBart]

You could do the same thing with a "match" "uri" restriction, and check that the URI end in /.

If a request doesn't end with /, but points to a directory, it results in redirect to uri/. If you match only uri/ then you will need another rule to maintain this redirect behavior. So, in order to completely replicate current behavior it requires more rules than it may seem on the first glance. More configuration usually means more error prone.

[alejandro-colomar]

You could do the same thing with a "match" "uri" restriction, and check that the URI end in /.

If a request doesn't end with /, but points to a directory, it results in redirect to uri/. If you match only uri/ then you will need another rule to maintain this redirect behavior. So, in order to completely replicate current behavior it requires more rules than it may seem on the first glance. More configuration usually means more error prone.

Agree. I think for now it's preferable to not change that behavior. If Unit is ever made more complex, let's check again.

I'll open a new PR to fix the NUL bugs, and drop the behavior change.

[hongzhidao]

I found it by reading the source code, and the diffs, as documented in the commit messages.

You said you regarded it as a bug by reading the code but not testing it. But from the comments you posted, it looks like serious bugs happened.

Is that performance difference meaningful? Probably not.

It's allocated twice in your way but the current was allocated once...

That's easy. When you have doubts about some code, check why it was changed, when, and how:

There is no end - p - 1 in the past, then you introduce something like it.

I can't see the benefits of the change.

[tippexs]

@mqudsi coming back to you. After doing some more tests and validating the configuration options came to the conclusion this is not a bug if configured like mentioned above. The Documentation is clear about IF share for directory indexing the MIME-Filter will not take affect BUT will take affect for all files served via share.

Thanks for reporting. I will close this and we will work on your other, open issues.