Limit the number of filters

[jvoisin]

Chaining filters is becoming an increasingly popular primitive to exploit PHP applications. Limiting the usage of only a few of them at the time should, if not close entirely, make it significantly less attractive.

I hardcoded the number of filters to 5 for now, but this can be changed to an ini preference if wanted/needed, albeit I fail to see a valid usecase for anything requiring chaining 5 filters or more. I'd love to keep it hardcoded, and to lower the number to 3.

This should close #10453

/cc @arnaud-lb @cfreal

[arnaud-lb]

I'm in favor of this idea, as this would make some bugs less likely to be exploitable.

I agree that legitimate use-cases requiring many filters seem unlikely, but it may be worth it to run a survey of open source code to validate that.

As the technique relies on injecting arbitrary strings in an application, that are then interpreted as URLs, would it be enough to limit the number of filters in string representations of php://filter URLs? The benefit would be that stream_filter_append() would be unaffected, so we could limit the number of filters in php://filter URLs further (e.g. to 3 filters as you are suggesting) while reducing the risk of BC breaks.

As a side note, I'm starting to think that accepting URLs in stream functions was a bad idea. We have allow_url_fopen / allow_url_include to mitigate that, but the former is enabled by default. This makes arbitrary-file i/o vulnerabilities worse and more likely. Also, the syntax of these URLs, or how to escape components in them, is not always clear, so they are not easy to use. I think that we should have an object API to build these URLs, I/O functions should accept object representation of these URLs, and allow_url_fopen should be eventually disabled by default. We should also have APIs to manipulate paths securely (e.g. in a way that prevents path traversal).

[jvoisin]

As the technique relies on injecting arbitrary strings in an application, that are then interpreted as URLs, would it be enough to limit the number of filters in string representations of php://filter URLs? The benefit would be that stream_filter_append() would be unaffected, so we could limit the number of filters in php://filter URLs further (e.g. to 3 filters as you are suggesting) while reducing the risk of BC breaks.

Good point, I updated the PR to use this approach instead.

[arnaud-lb]

Looked at usages of php://filter in the wild, and here is what I've noticed:

• Most uses are either fully static URLs with a single filter such as 'php://filter/write=string.rot13/resource=data:text/plain,hello', or only the resource part is dynamic, e.g. 'php://filter/write=string.rot13/resource=' . $file. This is often used in unit tests.
• Most uses in which the filter part is dynamic, or in which there are more than one filter, are exploit tools or exploits
• The few legitimate uses in which the filter part is dynamic only allow a single filter.

So the proposed change will not break any of the code indexed by sourcegraph and is unlikely to break any code if this is representative of private code bases, even with a limit of 3.

The idea and the change look good to me, but I think that you need to write to the mailing list. If there are objections this may require an RFC.

[jvoisin]

There is a lot of mailing lists, which one shall I send a message to?

[arnaud-lb]

The Internals one ([email protected])

[bukka]

I'm not sure I like it in the current form. I think this needs to be for sure at least INI and it should be done through the deprecation path as this is a clear BC break. We cannot judge impact based on source graph as majority of applications are in private code bases. Also there are internal filters added in some cases (e.g. transfer-encoding in http wrapper) so there can be some hidden filters as well even though there are not many. I think ideally it should be included in 8.5 deprecation RFC.

[bukka]

Also throwing exception is inconsistent with current stream error handling so it might require discussion on it's own. It should be probably just warning. We could possibly just add warning and keep it working till next major but not sure if it's ideal. Maybe just separate RFC for this would be best as others would probably prefere exception but we should do some checking how it fits to the current error handling and if some changes there should be done too.

[arnaud-lb]

Thank you @bukka. If we go the deprecation route, it may be worth it to investigate a larger deprecation scope in my opinion. Do you have an opinion about the last part of #16699 (comment) ? Specifically, we could deprecate fopen("php://filter/...") and require the user to pass a non-string representation instead (probably an object). This would make it less likely that an attacker is able to inject an URL, and also improve the experience of building such URLs.

Also there are internal filters added in some cases (e.g. transfer-encoding in http wrapper)

The change only limits the number of filters in php://filter URLs, not in streams, so this should be unaffected.

[hashkitten]

Suggested test cases:

php://filter/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/read=string.rot13/resource=php://output
php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://filter/read=string.rot13/resource=php://output