[#931] Bound and escape the values a clear's report reads out of the database - #1008
Open
vharseko wants to merge 1 commit into
Open
[#931] Bound and escape the values a clear's report reads out of the database#1008vharseko wants to merge 1 commit into
vharseko wants to merge 1 commit into
Conversation
…rt reads out of the database Every value those lines carry was read out of the database, and the premise of the lines carrying them is a database written into by something other than this backend. Nothing bounded them: a catalog holding a million rows a clear cannot act on was one log record of a million descriptions, and a value carrying a newline spliced the rest of itself into the server log as further records. forLog() escapes the control characters and the two unicode separators and bounds a value at 200 characters; SkippedRows counts every row passed over and describes the first 20, which bounds the per-row warns of readCatalogRows() by the same cap. Every count these lines state stays whole - what is capped is the naming. The same escape goes on the lines naming what a clear left standing, the stamp of a table being a comment somebody else may write, and on the tree name of a row whose table is gone: TreeName.valueOf() asks for nothing but two slashes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #931
Lands on top of #893, which is where
readCatalogRows()andreportSkippedRows()came from.Problem
A clear accounts for every row of its catalog and for every
opendjtable it left standing, and both accounts are built out of values read back off the database: the key of a catalog row, the table name that row records, the comment a table is stamped with. The premise of those lines is a database written into by something other than this backend — so their input is untrusted by construction, and nothing bounded it.No cap. A catalog holding a million rows this backend cannot act on was a million
logger.warnrecords on the way in, aListof a million descriptions held for the whole of the clear, and one log record of all of them at the end.A value could end the record carrying it. A newline in one of them splices the rest of itself into the server log where it reads as further records — the pattern CodeQL names log injection.
What the values actually are
name— the whole of thekcolumn, decoded as UTF-8, unchecked.treeName—TreeName.valueOf()requires a/at index 0 and another at index >= 2 (TreeName.java:53-60) and asks nothing else: everything between and after, control characters included, is whatever the row holds.tableName— the whole of thevcolumn, in the branch that has just established it is none of this backend's names (isOwnTableName()rejected it).The columns bound almost nothing —
k raw(2000), v blobon oracle,k varbinary(255), v longblobon mysql,k varbinary(max), v imageon sql server,k bytea, v byteaon postgresql — so a single row is enough for a multi-megabyte record, which a cap on the number of rows would not catch.The same shape stands in two lines beside it: the "tables of this backend its catalog does not name" list embeds the stamp of each table, which is a comment somebody else may write, and the "names tree %s, whose table %s is not there" line carries a tree name off the same catalog.
Fix
forLog(String)— renders\n,\rand\tas escapes, every other ISO control character and the two unicode separators as\uXXXX, and bounds the value at 200 characters with a...(+K more characters)tail. A backslash is left alone: it ends no record, and escaping it would spell every escaped comma of a normalized DN twice over.forLog(Collection<String>)— the first 20 values, each through the above, and a count of what the line is not naming.SkippedRows— replaces theList<String>: it counts every row passed over and keeps the first 20 descriptions. Whatadd()answers is whether the row is one of the described, which is what bounds the per-row warns ofreadCatalogRows()by the same cap; one line at the end of the read says how many rows went by without one.Every count these lines state stays whole —
%d row(s),%d table(s)— what is capped is the naming.CachedConnection.redact()is the nearest precedent in the package for a value that goes through something on its way to a log; this is the same shape for the opposite direction, keeping somebody else's records out rather than this backend's credentials.reportSkippedRows()is no longer private: it asks the database nothing and writes to nothing butreportClearLine(), so the bounds of the line it builds are held to by a case that calls it directly.This is not a red CodeQL alert
The repo scans with
security-and-qualityand has nojava/log-injectionalert here, and would not be expected to: aResultSetread is a source only under thedatabasethreat model, which is off by default. The pattern is real; the fix is not chasing a failing check.Tests
ClearReportTestCaseis new and needs no database — the escape and both caps are pure functions, and the line itself is built out of an accumulator a case fills by hand. Keeping it out of the container suites is the point: those skip themselves whole where no docker is reachable, and a bound nothing exercises can be deleted without a test going red.TestCase.testAClearDoesNotLetACatalogRowEndTheLineReportingItis the same thing through a real clear against a real database: a catalog row recording a name with a newline in it, and no line of the report carrying a\nor\rafterwards.ClearReportTestCasePgSqlTestCaseSkipped: 0PgSqlTestCase, withforLog()taken out of the "no table of this backend" branchMySqlTestCase,MsSqlTestCaseandOracleTestCasewere not run: nothing here is engine-specific — the change is string handling and the wording of report lines — and the case above runs in all four suites. Say the word and I will run them.Named rather than folded in
MAX_REPORTED_VALUESbounds the descriptions of passed-over rows and the listings of leftover tables alike. The counts beside them stay exact, but on a database several backends share, the "named by no catalog" listing can now be a sample of a longer list. Two constants, or a larger one, if you would rather.missingTreesanyway. Say the word if it should be bounded like the rest.