[reload4j] SQL injection problem in JDBCAppender

[Ceki Gülcü]

On 1/19/2022 1:15 PM, Vladimir Sitnikov wrote:
>>If JDBCAppender is removed thre would be a single exception thrown at
>>config time
> 
> Once again: there are valid cases when people override `flushBuffer` and
> implement secure database calls.
> In other words, they find "buffering" feature useful.

The buffering feature is not at cause. What is at cause is the
unprotected JDBC call which uses a simple Statement instead of a
PreparedStatement.

> If we drop the class altogether, the users would have to
> rewrite/recompile the code which is harder than just replacing log4j.jar
> with reload4j.jar.
> if we keep the class, we could still keep the "drop-in replacement"
> label which would be very good.

 I agree that 100% drop-in compatibility is better than covering 99%.
However, we have to be realistic about the means at our disposal.
> That is why I suggest we keep the class, and we heal SQL injection.

OK. I don't think fixing SQL injection can be done without heavy
surgery. It would be probably be easier and more sane to write a new
JDBCAppender from the ground up.

>>Then there is the case of MDC (%X), NCD (%x) and many other cases which
>>we did not anticipate
> 
> We can hard-code things like "we support PatternLayout only", then we
> know the full set of markers (e.g. everything with %).

But the problem is the use of PatternLayout. We would need to reinvent
JDBC PreparedStamement to overcome the vulnerability.

> Vladimir
> 

-- 
Ceki Gülcü

Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch