Hi All, JDBCAppender uses simple strings instead of java.sql.Statement to talk to the database. This creates a vulnerability point for SQL injection attacks. Fixing this vulnerability in JDBCAppender (a rarely used component) in a backward compatible way would be a lot of work for very little or no benefit. S such, I propose to remove JDBCAppender from reload4j with no replacement. Any objections? -- Ceki Gülcü Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch
Hi, I think that's a sensible suggestion. Those who need that functionality could always add a JDBCAppender of their own separately. /Robert -- _______________________________________ Robert Olofsson, Sweden http://www.unlogic.se On January 19, 2022 9:38:04 AM GMT+01:00, "Ceki Gülcü" <ceki@qos.ch> wrote:
Hi All,
JDBCAppender uses simple strings instead of java.sql.Statement to talk to the database. This creates a vulnerability point for SQL injection attacks.
Fixing this vulnerability in JDBCAppender (a rarely used component) in a backward compatible way would be a lot of work for very little or no benefit.
S such, I propose to remove JDBCAppender from reload4j with no replacement.
Any objections?
-- Ceki Gülcü
Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch _______________________________________________ reload4j mailing list reload4j@qos.ch http://mailman.qos.ch/cgi-bin/mailman/listinfo/reload4j
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.JDBCAppe... One of the ideas could be treating '%m' as ? (bind placeholder). That would automatically cover cases like https://github.com/Gitlishitong/bams/blob/806385d0f3d9dd1074f3436c2c70ab1e35... 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/... 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
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
How do you prevent SQL injection in the first place? -- Ceki Gülcü Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch
You would need to use PreparedStatements and this break backward compatibility unless am I missing something. On 1/19/2022 12:53 PM, Ceki Gülcü wrote:
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
How do you prevent SQL injection in the first place?
-- Ceki Gülcü Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch
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.JDBCAppe... <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/806385d0f3d9dd1074f3436c2c70ab1e35... <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
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. 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. That is why I suggest we keep the class, and we heal SQL injection.
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 %). Vladimir
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
Let me file a PR with what I mean. I think we can support many of the current usages in a drop-in replacement fashion, while advising them to migrate to more robust approaches.
It would be probably be easier and more sane to write a new JDBCAppender from the ground up.
A new JDBCAppender makes sense indeed. That, however, is another story. Vladimir
Here's a PR with what I suggest: https://github.com/qos-ch/reload4j/pull/26 JdbcPatternParserTest shows how it parses the current pattern into "text for the prepared statement" and "arguments for it" in JdbcPatternParserTest. I believe it fixes the CVE, and it keeps the code compatible with previous usages. Vladimir
participants (3)
-
Ceki Gülcü -
Robert Olofsson -
Vladimir Sitnikov