[reload4j] SQL injection problem in JDBCAppender

[Vladimir Sitnikov]

>Those who need that functionality could always add a JDBCAppender of their
own separately.

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

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

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

I am not sure what to do with more complicated cases like INSERT INTO A1
(TITLE3) VALUES ( ' %d  -  %c %-5p %c %x  -  %m%n ' )
https://github.com/0532/print/blob/59f8f7450e2de591e74c0c347fd00fd7dc45357f/src/log4j.properties#L38

We could probably assume people would use ' ....format ...'  approach, and
we could auto-convert those patterns to a bind placeholders, and we could
fail if the conversion fails.
That would make the thing secure by default, and it would support the users.

Vladimir
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.qos.ch/pipermail/reload4j/attachments/20220119/52e117db/attachment.html>