Enforcement of new Poggit rule against SQL injection about bedcoreprotect HOT 10 CLOSED

I understand that you are already using libasynql partially. However, it is still considered not acceptable that you generate your queries like this. Please consider using different queries for your purpose as detected above.

from bedcoreprotect.

Hi, what do you mean with "... you are already using libasynql partially."? The entire plugin is totally based on libasynql.

from bedcoreprotect.

The use of executeXxxRaw shall not be used to inject custom data.

from bedcoreprotect.

Ok, then:

1. What's the purpose of "executeXxxRaw"?
2. How can I use dynamically generated query if libasynql does not allow it? (E.g. multiple select query)

from bedcoreprotect.

The purpose of executeXxxRaw is so that you can generate queries with a dynamic number of subexpressions, e.g. INSERT INTO tbl VALUES (?,?),(?,?),(?,?) etc. It is not to let you inject data (data as in those that may depend on external uncontrollable factors).

The principle of least privilege states that a program should have as little code as possible that could lead to security issues. For example, if you inject $player->getUuid() into a query, if we later on discover a vulnerability where an adversary can insert an arbitrary string as the player uuid, this vulnerability could be additionally exploited to breach database integrity or even result in remote code execution (this may be possible with certain SQLite3 queries like ATTACH). Therefore, rule B8 basically states that all possible strings that can go into a SQL query must either be escaped or originate from some code directly related to the SQL processing (such as your own utility function for generating a repeating sequence of (?,?)s, or libasynql), while your $this->uuid, $x, etc. might not be so in this case.

Security vulnerabilities often result from multiple unrelated commits that work together to open a loophole. Therefore, each module must be strengthened as much as possible.

from bedcoreprotect.

I am not sure what you mean by multiple select query. libasynql already supports arguments in the form (?,?,?,...) using the list: type modifier.

If there is a case where your usage is not covered, you may still use dynamically generated queries, but that must only be used for generating the syntax part of the SQL, not the data part.

It is not possible to escape the string on the main thread, because MySQL parser is charset-sensitive. See this StackOverflow question for more information. Therefore, for MySQL, you simply should not try to escape strings at all; you must pass strings as separate params.

from bedcoreprotect.

Very clear, thanks.

About the plugin dynamic query generation, I use it for:

• builds a query with dynamic WHERE clause, you can see here
• builds multiple select query, I said wrong before but I was meaning this.

I am forced to use this approach because the number of parameters is N and I cannot define the query in a static way.

from bedcoreprotect.

Sure, but use placeholders. You can use them like executeSelectRaw("SELECT ?", [$foo]);.

I would still advise against building a dynamic query. You may want to pass dummy parameters to it and add a OR :boolean parameter, where :boolean is 1 or 0. I believe the database engine should be able to optimize this away.

For the case of log_history, I feel like you're premature optimizing. How large is $this->positions likely to be? Is it really bad to submit one query per insertion?

from bedcoreprotect.

Sure, but use placeholders. You can use them like executeSelectRaw("SELECT ?", [$foo]);.

Do you mean I can build a raw query like this: SELECT field FROM table WHERE ?; where ? could be a = ? or b = ??

The $this->positions in the most of cases contains the number of blocks exploded by entity (TNT), they could be a lot. The worst case is when more TNTs explode generating a lot of query. This is why I had to choose this optimization to speed up the query insertion times.

from bedcoreprotect.

In that case, I guess you really have to build the query like this. But still, use ?, yes.

from bedcoreprotect.