[reload4j] the jdbc_cve branch

[Vladimir Sitnikov]

>Not silently as we would have amply publicized this.

I strongly suggest keeping the current APIs if possible (keep old fields,
types, etc).
It does not help much if we drop JDBCAppender#removes field, and the
removal definitely breaks apps.
So why remove it?

I intentionally avoided making extra cleanup changes in PR26 so we can
agree on the approach and merge it.
I assumed the cleanup can go afterward if needed.

Frankly speaking, I'm inclined that there might be a good case for several
JDBC appenders.

For instance:
-jdbc-tableappender.jar
The users would be able to configure table name and column names.
The appender would write to the table and that would be it.

-jdbc-freesql.jar
The user configures SQL, and the appender uses it via prepared statement.
This is less secure than -jdbc-tableappender, because an attacker would be
able
to execute an arbitrary SQL if they can edit the configuration file.

The same thing might apply to other appenders (and logback? ;) ).
If that is something appealing to you, then imagine we can move the current
JDBCAppender to
reload4j-jdbc-legacy.jar and we could let it die there. We don't need to
refactor and modernize it then.

>cure mode might be quite hard to defend in the current
>climate, even as an opt-in feature.

Frankly speaking, I assumed that there might be systems that
use the current JDBCAppender in a secure manner (e.g. their format message
does not include user-provided input),
so I assumed it might be not that bad to just leave the fallback.

On the other hand, if their format string is not supported by the new
parser, adding quotes should be enough,
so it might be just fine to drop insecure mode.

There's a possibility that people used "table rotation" via insert into
logs%d{yyyy}(...).
This case is not supported by my fix, and I think it would be hard to
support in a secure manner.

>You must construct the expected result by hand. This implies that you
>know what you are doing when you write the test. That is kind of the point

Frankly speaking, I think the added ParserState boilerplate (equals,
hashcode, etc) negates all that.
getCopyOfpatternStringRepresentationList looks crazy (naming is hard), so I
would prefer simpler designs,
especially for an internal (package-private) class like JdbcPatternParser.
Then, JdbcPatternParser#toString() assertion ensured the object would look
sane in the debugger.

---

>I was not aware of that. It must be the mixed tab mode which is the
>default. Would "only spaces" be better?

That is why suggested .editorconfig and its indent_style = space.
Reviewing changes is harder when the code layout is broken,
and it is often broken when tabs and spaces are mixed in the same file.

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