From 90bba2a1f9118d7962179d93392cd20c52bdf4cd Mon Sep 17 00:00:00 2001 From: guoqiang Date: Fri, 7 Aug 2026 17:10:31 +0800 Subject: [PATCH] [fix](audit) escape 0x1F/0x1E in audit_log stream load to prevent row forgery The audit plugin frames its stream-load payload for __internal_schema.audit_log with 0x1F (column separator) and 0x1E (row delimiter), but the string columns in AuditLoader.fillLogBuffer were appended without escaping. A statement carrying raw 0x1F/0x1E bytes -- e.g. inside a block comment or string literal, which the lexer accepts -- could therefore close its own audit row early and have the trailing bytes parsed as an additional, fully attacker-controlled row, forging or misattributing rows in the audit table (CWE-117 log injection). - Add sanitizeField(), which replaces the two framing bytes (0x1F, 0x1E) with a space. Only these two bytes are structural, so all other content -- including newlines and tabs already present in SQL text -- is preserved unchanged. - Route every string column in fillLogBuffer through appendField() / appendLastField() so none can bypass the sanitizer and new string columns are covered automatically. Note that planTimesMs, getMetaTimesMs and scheduleTimesMs are String columns despite the Ms suffix. Numeric and boolean columns are appended directly since they can never contain these bytes. - Add unit tests asserting that injected delimiters (in stmt, user, db and planTimesMs) cannot add rows or columns, and that ordinary statements pass through unchanged. The text-file audit sink (AuditLogBuilder, fe.audit.log) uses a |key=value format and is unaffected. --- .../doris/plugin/audit/AuditLoader.java | 82 ++++++++++++++----- .../doris/plugin/audit/AuditLoaderTest.java | 63 ++++++++++++++ 2 files changed, 124 insertions(+), 21 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java b/fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java index 6a09fdfa1fc536..bbc86646635e6c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java +++ b/fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java @@ -149,22 +149,22 @@ private void fillLogBuffer(AuditEvent event, StringBuilder logBuffer) { // should be same order as InternalSchema.AUDIT_SCHEMA // uuid and time - logBuffer.append(event.queryId).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.queryId); logBuffer.append(TimeUtils.longToTimeStringWithms(event.timestamp)).append(AUDIT_TABLE_COL_SEPARATOR); // cs info - logBuffer.append(event.clientIp).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.user).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.feIp).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.clientIp); + appendField(logBuffer, event.user); + appendField(logBuffer, event.feIp); // default ctl and db - logBuffer.append(event.ctl).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.db).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.ctl); + appendField(logBuffer, event.db); // query state - logBuffer.append(event.state).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.state); logBuffer.append(event.errorCode).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.errorMessage).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.errorMessage); // execution info logBuffer.append(event.queryTime).append(AUDIT_TABLE_COL_SEPARATOR); @@ -183,40 +183,80 @@ private void fillLogBuffer(AuditEvent event, StringBuilder logBuffer) { // plan info logBuffer.append(event.parseTimeMs).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.planTimesMs).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.getMetaTimesMs).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.scheduleTimesMs).append(AUDIT_TABLE_COL_SEPARATOR); + // planTimesMs / getMetaTimesMs / scheduleTimesMs are String columns (formatted timing + // breakdowns), not numbers, so they must be sanitized too. + appendField(logBuffer, event.planTimesMs); + appendField(logBuffer, event.getMetaTimesMs); + appendField(logBuffer, event.scheduleTimesMs); logBuffer.append(event.hitSqlCache ? 1 : 0).append(AUDIT_TABLE_COL_SEPARATOR); logBuffer.append(event.isHandledInFe ? 1 : 0).append(AUDIT_TABLE_COL_SEPARATOR); // queried tables, views and m-views - logBuffer.append(event.queriedTablesAndViews).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.chosenMViews).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.queriedTablesAndViews); + appendField(logBuffer, event.chosenMViews); // variable and configs - logBuffer.append(event.changedVariables).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.sqlMode).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.changedVariables); + appendField(logBuffer, event.sqlMode); // type and digest - logBuffer.append(event.stmtType).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.stmtType); logBuffer.append(event.stmtId).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.sqlHash).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.sqlDigest).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.sqlHash); + appendField(logBuffer, event.sqlDigest); logBuffer.append(event.isQuery ? 1 : 0).append(AUDIT_TABLE_COL_SEPARATOR); logBuffer.append(event.isNereids ? 1 : 0).append(AUDIT_TABLE_COL_SEPARATOR); logBuffer.append(event.isInternal ? 1 : 0).append(AUDIT_TABLE_COL_SEPARATOR); // resource - logBuffer.append(event.workloadGroup).append(AUDIT_TABLE_COL_SEPARATOR); - logBuffer.append(event.cloudClusterName).append(AUDIT_TABLE_COL_SEPARATOR); + appendField(logBuffer, event.workloadGroup); + appendField(logBuffer, event.cloudClusterName); // already trim the query in org.apache.doris.qe.AuditLogHelper#logAuditLog String stmt = event.stmt; if (LOG.isDebugEnabled()) { LOG.debug("receive audit event with stmt: {}", stmt); } - logBuffer.append(stmt).append(AUDIT_TABLE_LINE_DELIMITER); + // stmt is the last (and only free-text) column; sanitize it too so a statement carrying + // raw 0x1F/0x1E cannot truncate its own row and forge a following one. + appendLastField(logBuffer, stmt); + } + + /** + * Append one string column to the delimiter-framed audit stream-load payload, followed by the + * column separator. The value is sanitized first so that user-controlled text (SQL statement, + * identifiers, session-variable values, error messages, ...) cannot embed the column separator + * (0x1F) or row delimiter (0x1E) and thereby forge, truncate, or misattribute audit rows in the + * internal {@code audit_log} table (O07 / CWE-117 log injection). Numeric and boolean columns + * are appended directly since they can never contain these bytes. + */ + private static void appendField(StringBuilder logBuffer, String value) { + logBuffer.append(sanitizeField(value)).append(AUDIT_TABLE_COL_SEPARATOR); + } + + /** + * Append the final string column of a row: sanitize the value (same reason as {@link + * #appendField}) and terminate the row with the line delimiter. Every string column is written + * through {@code appendField}/{@code appendLastField} so none can bypass the sanitizer. + */ + private static void appendLastField(StringBuilder logBuffer, String value) { + logBuffer.append(sanitizeField(value)).append(AUDIT_TABLE_LINE_DELIMITER); + } + + /** + * Replace the audit framing bytes (column separator 0x1F and row delimiter 0x1E) with a space so + * field content cannot alter row/column framing. Only these two bytes are structural, so other + * characters (including newlines and tabs already present in SQL text) are preserved as-is. + */ + private static String sanitizeField(String value) { + if (value == null || value.isEmpty()) { + return value; + } + if (value.indexOf(AUDIT_TABLE_COL_SEPARATOR) < 0 && value.indexOf(AUDIT_TABLE_LINE_DELIMITER) < 0) { + return value; + } + return value.replace(AUDIT_TABLE_COL_SEPARATOR, ' ').replace(AUDIT_TABLE_LINE_DELIMITER, ' '); } // public for external call. diff --git a/fe/fe-core/src/test/java/org/apache/doris/plugin/audit/AuditLoaderTest.java b/fe/fe-core/src/test/java/org/apache/doris/plugin/audit/AuditLoaderTest.java index 63bab9c3f95920..2da3c3dfa1abd1 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/plugin/audit/AuditLoaderTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/plugin/audit/AuditLoaderTest.java @@ -79,4 +79,67 @@ private String getAuditLogBuffer(AuditLoader auditLoader) { StringBuilder buffer = Deencapsulation.getField(auditLoader, "auditLogBuffer"); return buffer.toString(); } + + // O07: raw 0x1F/0x1E in user-controlled fields must not be able to add/remove columns or rows. + // A statement carrying the framing bytes (e.g. inside a block comment) must still produce exactly + // one row with the same column count as a clean statement -- otherwise the attacker forges a row. + @Test + public void testDelimiterInjectionDoesNotAlterFraming() { + AuditLoader auditLoader = new AuditLoader(); + char col = AuditLoader.AUDIT_TABLE_COL_SEPARATOR; + char line = AuditLoader.AUDIT_TABLE_LINE_DELIMITER; + + StringBuilder clean = new StringBuilder(); + Deencapsulation.invoke(auditLoader, "fillLogBuffer", + new AuditEvent.AuditEventBuilder() + .setUser("alice").setDb("mydb").setStmt("select 1").build(), + clean); + + // The forged payload tries to close its own row and inject a fully attacker-controlled one. + // Inject into stmt, user, db AND planTimesMs -- planTimesMs is a String column that is easy + // to overlook (its name suggests a number), so exercising it guards against a column + // silently bypassing the sanitizer. + String evilStmt = "select 1 /*" + line + "deadbeef" + col + "2026-01-01 00:00:00.000" + + col + "10.0.0.9" + col + "root" + col + "DROP TABLE finance.ledger*/"; + StringBuilder evil = new StringBuilder(); + Deencapsulation.invoke(auditLoader, "fillLogBuffer", + new AuditEvent.AuditEventBuilder() + .setUser("al" + col + "ice").setDb("my" + line + "db") + .setPlanTimesMs("plan:" + col + "1ms" + line + "forged") + .setStmt(evilStmt).build(), + evil); + + // Exactly one row, and the same number of columns as the clean event. + Assert.assertEquals("injected 0x1E must not add rows", + count(clean, line), count(evil, line)); + Assert.assertEquals("one row per event", 1, count(evil, line)); + Assert.assertEquals("injected 0x1F must not add columns", + count(clean, col), count(evil, col)); + // The forged tokens survive only as inert text, never as framing bytes. + Assert.assertTrue(evil.toString().contains("DROP TABLE finance.ledger")); + } + + // The sanitizer must be a no-op for ordinary statements: no data loss, no mutation. + @Test + public void testCleanStatementIsPreserved() { + AuditLoader auditLoader = new AuditLoader(); + StringBuilder buffer = new StringBuilder(); + Deencapsulation.invoke(auditLoader, "fillLogBuffer", + new AuditEvent.AuditEventBuilder() + .setUser("bob").setDb("sales") + .setStmt("select * from t where a = 1 and b = 'x'").build(), + buffer); + Assert.assertTrue(buffer.toString().contains("select * from t where a = 1 and b = 'x'")); + Assert.assertEquals(1, count(buffer, AuditLoader.AUDIT_TABLE_LINE_DELIMITER)); + } + + private static int count(CharSequence s, char c) { + int n = 0; + for (int i = 0; i < s.length(); i++) { + if (s.charAt(i) == c) { + n++; + } + } + return n; + } }