[reload4j] SQL injection problem in JDBCAppender

[Ceki Gülcü]

Responses inline.

On 1/19/2022 11:22 AM, Vladimir Sitnikov wrote:
> There's a case when users can override JDBCAppender, and override its
> flushBuffer method.
> 
> So removing the class would break "drop-in replacement" 
> I would rather suggest doing the following:
> 1) Throw an exception from JDBCAppender#flushBuffer unless there's
> reload4.appender.jdbc.allow_insecure_sql_replace=true
> That would make the thing secure, and the people would have a migration plan

If JDBCAppender is removed thre would be a single exception thrown at
config time. Throwing an exception within JDBCAppender#flushBuffer would
cause many more exceptions to be thrown.

> 2) Having a secured JDBCApender makes sense a well, even if the users
> would need to adjust the configs.
> It is used a lot even in public GitHub
> repositories: https://github.com/search?l=Java+Properties&q=org.apache.log4j.jdbc.JDBCAppender&type=Code
> <https://github.com/search?l=Java+Properties&q=org.apache.log4j.jdbc.JDBCAppender&type=Code>
> 
> One of the ideas could be treating '%m' as ? (bind placeholder).
> That would automatically cover cases like
> https://github.com/Gitlishitong/bams/blob/806385d0f3d9dd1074f3436c2c70ab1e3555ff40/src/main/resources/log/database.properties#L15
> <https://github.com/Gitlishitong/bams/blob/806385d0f3d9dd1074f3436c2c70ab1e3555ff40/src/main/resources/log/database.properties#L15>

Then there is the case of MDC (%X), NCD (%x) and many other cases which
we did not anticipate.

I propose we solve the security issue first by removing JDBCAppender and
circle back to creating a JDBCAppender based on PreparedStatement.

> Vladimir
> 
-- 
Ceki Gülcü

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