IMPLEMENTED: Better solution for echo function name filter

Feature Requests / Improvements
1

I appreciate your measures to improve security.
However, the echo function name filter “bricks/code/echo_function_names” is, in my humble opinion, not the best approach.

I use PHP function calls a lot in my projects, especially with custom functions.
Since the new filter only allows to return exact function names, I have to keep a second window open to frequently update my snippet containing the filter. (I’m using a code snippet instead of functions.php of a child theme)

A better solution would be to e.g.
a) allow function name wildcard patterns
b) allow functions in defined PHP namespaces

An external attacker wouldn’t be able to guess my pattern or namespace.
So this solution would be as safe as defining exact function names.

[EDIT]
Ideas from the related post in the FB group (Redirecting...):

  • Pass the function name as parameter to the filter so we can implement our own filter algorithm/rules (for example, allow functions by patterns like projectname_, get_, \namespace.…).

  • Allow disabling the filter during development and show a red flag instead. We can then run the code review and add the whitelist after finishing development.

  • Red flag for {echo:} using functions not whitelisted, so we don’t spend time wondering why we don’t get the expected output

26 Likes
2

I agree with this. Would make development a lot easier.

3

I just realized, that even with my two suggestions the use of echo would be a pain.
Just imagine simple basic PHP functions like implode(), date(), sprintf(), … or WP functions like get_queried_object_id(), get_post_type(), …

I would need to create a very long list of functions to be allowed.

Can we have a configuration switch to completely disable that filter??
Or at least pass the function name as parameter to the filter so we can apply our own filter algorithm.

5 Likes
4

Perhaps some of the trouble could be mitigated if you had that configured functions.php file in your blueprint child theme. That’s what I’ll be doing for now.

5

At a minimum, the ability to enable a Development Mode that bypasses the check until you are done creating/testing the custom code would be better. Then when reenabled, you can sign off on them all at once.

I do most of my custom development during, well, the development stage. Very rarely do I introduce new code snippets into LIVE projects.

Also, when updating to this new security feature, it could be disabled by default so it doesn’t immediately break the site, with maybe a notification to enable the feature and go through the signing process.

I’m holding off updating for a while until this all gets ironed out a bit.

7 Likes
6

Everyone has their own development time and sometimes it overlaps with working time. You propose that invalid code be executed on the site during “the next stage of development or adjustment”. This gives the attacker a good reason to wait until you visit the site and execute his code. For good security there should be no exceptions to the rules!!!

7

Here you are right about the update. When we updated, we must first calmly take our time creating all the signatures, add all the functions to the white list and then only enable the “Strong Security” setting

8

As @demas23 wrote in his comment, disabling the filter is meant for staging environments, which should never be accessible without proper authentication anyway.

3 Likes
9

Does this not work for you? Filter: bricks/code/disable_signatures – Bricks Academy

1 Like
10

@MattHias Great suggestion :slight_smile:

We discussed the different approaches internally, and it seems that #1 is the most flexible.

We can pass the function name to the bricks/code/echo_function_names filter, and you can perform your logic in the filter and return true or false according to your custom checks, such as regex, etc.

Something like that:

add_filter('bricks/code/echo_function_names', function( $function_name ) {
    // True if the function name starts with "get_"
    return preg_match( '/^get_/', $function_name );
});

An alternative way would be to build the regex check into our code and you could simple pass it as part of the returned array like this.

add_filter('bricks/code/echo_function_names', function( $whitelisted_items ) {
    // True if the function name starts with "get_" (the "@" flags it in Bricks as a regex 
    $whitelisted_items[] = '@^get_';
    return $whitelisted_items;
});

Let us know what you think :pray:

6 Likes
11 
$whitelisted_items[] = '@^get_';

I like it :slight_smile:

12

@thomas, thank you very much for your quick response on this topic!

Having a first look, I like both approaches.

The first approach, returning a simple boolean, would provide a very easy and also flexible way for us developers to implement every thinkable logic, like a string comparison, a regex match, etc.

The second approach, still returning a list of function names, would be compatible to your current implementation, which expects a list of function names as a result.
Provided that you’re passing in the function name in $whitelisted_items also in this approach, it would still allow me to do my own checking algorithm like regex match upfront and check the function name.

From a technical perspective, I couldn’t decide which approach I would prefer.
They both give me full control – provided that you pass in the function name in question for both approaches.

From a compatibility perspective, the second approach (return list) wouldn’t kill the filters already implemented by users of 1.9.7.
(I still assume, you’re passing in the function name also with this approach via $whitelisted_items!)

We must be absolutely clear that it’s in the responsibility of the developer to not just blindly return true, or the passed-in function name. The filter is, however, still a huge security gain compared to no control at all with Bricks <= 1.9.6

13

It would be neat if the default WP functions would be automatically whitelisted. @thomas Would this be a possibility or would it present a security issue?

14

Of course that would be a security issue because WP offers functions to e.g. create, update, delete posts, users, and also files. That’s all an attacker would need to compromise a system.

15

Entirely agree with all of this. Recently loved using the echo feature to return plugin functions and features, it was helpful and fast. Now I have to call each and every echo, save that, refresh, test code, save, etc.

This must be able to be made better.

16

I’m going to show my ignorance here, but how exactly does an attacker enter a function into an echo tag on a site? This presumes they are somehow authenticated into WordPress where they can edit Bricks content?? Or are they injecting it directly in the DB somehow? Or executing their own arbitrary PHP from a file?

A question I’m always asking is once this theoretical attacker is able to edit Bricks code output, aren’t they also able to wreck havoc on anything else they want too? If they can inject data to the DB, a Bricks function whitelist doesn’t mean much. If they can execute their own PHP code of any kind, they have endless ways to wreck the site outside of Bricks.

Under what situation do we have an attacker gaining access to WordPress (or Bricks) such that the ONLY thing they are able to “hack” is creating a Bricks echo tag with their own arbitrary function name that they can somehow also define somewhere, but it only affects Bricks? Can somehow hack themselves directly only into Bricks?

My point is, if an attacker gets access to edit echo tags and define new functions to run, they can probably do anything else they want, so what good is the whitelist at that point?
It’s like putting a lock on my fridge just in case a thief breaks into my house. But what good is that? They can set my house on fire or break or steal anything they want, protecting my fridge is the least of my worries at that point.

I could be way off base, but I’m just trying to understand why we have to go through the whitelist to “block hackers”, when it seems like if the hacker could edit that, then they likely can do just about anything else they want elsewhere in the system! What is this stopping exactly?

17

@vigilante, here’s a simplified explanation of the security issue:
There is a REST/AJAX call that allows the Bricks builder to execute and render {echo} tags to display the results in the preview pane.
Due to an improper and insufficient implementation, the permission/authorization check for this call could be overcome by an unauthenticated attacker, and allowed execution of arbitrary, potentially malicious PHP code. In short, an attacker didn’t need to be logged in nor able to access the builder to inject and execute code.

Besides improvement of the permission/authorization checks, the new echo function name filter allows the owner/developer of the site to (carefully!) define functions that are intended and allowed to be used.
An external attacker can’t know what functions are allowed by the filter, and any attack attempts with unlisted functions will be blocked.

You, as a developer, have to be careful which functions you allow to be used with {echo}. Those should never be any functions that allow write access, and of course never “open gate” functions such as eval.

18

I asked a question in a separate topic, but for some reason no one wanted to answer. Out of curiosity, I’ll ask here too.

I see a lot of posts on the forum about echo and that many people can’t add it correctly because they either don’t want to create and edit function.php or the php plugins don’t work as expected.

Now the question. Why is it necessary to put new users through this pain? Those clients to whom I advertise bricks complain to me.

Why should a site administrator with full rights do all this? This approach does not add security in any way since the administrator can manage files on the server by installing a file manager.

Why can’t the administrator be given the opportunity to create exclusion data through the site’s admin panel?

If the security policy prohibits storing exceptions in the database, we can always write them to a file. We’ll end up with the same effect without having to scare people away.

Who has time, explain to me why this approach is safe, because the administrator already has access to the entire site, including files on the server, through file manager plugins.

Why can’t it be done this way?

2024-03-26_11-08-12
2024-03-26_11-08-12750×375 28.2 KB

19

@clickfusion63, I agree.
While understanding the necessity of a whitelist, I didn’t like the filter approach from the beginning due to the extra efforts and also the implementation via PHP.
It would be easier and more intuitive to have e.g. a text box in the admin panel to add the function names.
It would also be cool to have a red marker, if a function used in an echo statement isn’t whitelisted yet, along with a little button to add the function name to the whitelist directly.

That said, I also do like the flexibility of the filter function if it’s modified in one of the ways described above. That would allow e.g. some pattern to whitelist functions, like “project_get_*”.

A combination of those would be perfect:

  • Marker and button to whitelist function names directly
  • Text box for the easy setups with only a few echo functions
  • The filter for more complex setups and the experienced developers
    You can use either one or the other or even combined: First check function names listed in text box, then send through the optional filter function to modify the whitelist.
2 Likes
20

Hi @clickfusion63,

I understand your concerns regarding the echo workflow and the new additional steps required. Let me explain the rationale behind this approach:

Why use filters to whitelist echo tag functions?:

  • Dynamic nature of echo tags: Echo tags are dynamic and can be used anywhere on the site, making it impractical to apply code signatures to them (which is why we had to come up with a different solution).
  • File access requirement: Filters need file access to modify. This means even if someone has database access, they can’t execute code without file access.
  • Protection against database-only attacks: Many attacks target the database. By requiring file access to whitelist functions, we ensure a higher security standard.
  • Admin rights vs. Code Execution: While admins can manage files, this setup ensures that only those with the right permissions can execute code, adding another layer of security.

Regarding this:

  • Some high-security environments restrict admins from uploading plugins/themes or modifying files.
  • Our approach fits these scenarios, ensuring security even with limited admin rights.

We aim to have Bricks secure by default. However, we recognize the current setup can be frustrating so feel free to share your thoughts about our recommended solutions:

Let me know if you still have any questions!