Re: [PR] [fix] prevent SQL injection in QuestDB history queries [hertzbeat]

["Duansg (via GitHub)" <gi...@apache.org>]

Duansg commented on PR #4259:
URL: https://github.com/apache/hertzbeat/pull/4259#issuecomment-5232440193

   Thanks for tracking this down — I reproduced the issue and the direction is right. metric and table are interpolated into "%s" with no charset check, a " closes the identifier, and any payload without a . gets past the controller's three-segment split. replace("'", "\\'") is genuinely wrong for QuestDB (ANSI quote doubling, backslash is not an escape). The endpoint is reachable by the guest role (sureness.yml:31), so this is worth fixing.
   
   A few things I'd like to resolve before this lands.
   
   1. This fixes the least-exposed of four storages with the same pattern. TDengine (TdEngineDataStorage:367) and InfluxDB (InfluxdbDataStorage:255) interpolate history — a raw @RequestParam — straight into the SQL, with no equivalent of QuestDB's getDateAdd() parsing. InfluxDB doesn't even have the (broken) quote escaping for instanceValue. IoTDB has the same shape. Merging this as-is means an operator on TDengine or InfluxDB has a larger injection surface than the one we just closed.
   
   Two options: extend this PR to tdengine/influxdb/iotdb, or do the identifier and range validation once in MetricsDataServiceImpl — where all four storages converge — and keep only escapeStringLiteral() here, since quote escaping is the one genuinely dialect-specific piece that can't move up. Either way, could we open a tracking issue for the remaining storages? I'd rather not have this marked "fixed" while three of them stay open.
   
   2. The read-side allowlist is narrower than what the write side can produce. saveData() writes cell.getField().getName() verbatim as a column name, and verifyDefineAppContent() applies no charset validation to field names — we ship heap.memory.used (app-seatunnel.yml) and rocksdb.backup.read.bytes (app-nebula_graph.yml) today. The REST path won't regress because the three-segment split already rejects dotted names, but hertzbeat-ai's MetricsToolsImpl:172 calls the service directly and its fieldParameter is required = false, so it can be null or dotted. As a rule, a read-side validator shouldn't be stricter than the writer — otherwise some stored columns become unreadable by construction. Either constrain column names in saveData() too, or widen the pattern to at least . and /.
   
   3. Prefer normalizing table over validating it. The folding in generateTable() is conditional:
   
   if (instance.contains(".") || instance.contains(":") || instance.contains("["))
   
   An instance like a]b skips the branch entirely, keeps the ], and gets rejected by the new check. Dropping the if and folding unconditionally (or replacing anything outside the charset) makes the table slot non-injectable by construction and removes the need for validateIdentifier(table, ...). Note that a full-charset normalization would change existing table names, so that part needs a compatibility call. Keeping metric as reject-only is correct — it's a column name and must match exactly.
   
   4. Tests. The module already has TdEngineDataStorageTest, GreptimeDbDataStorageTest, VictoriaMetricsDataStorageTest. Making the two new methods package-private and adding four cases would lock this in: an identifier that closes the quoting is rejected; what generateTable actually produces still passes (127.0.0.1:8080, [::1]:8080, hyphenated hostnames); it's → it''s; null metric.
   
   Minor:
   - QUERY_HISTORY_SQL_WITH_INSTANCE (line 70) has no references anywhere and carries the same unescaped '%s' — worth deleting while we're here.
   - The IDENTIFIER constant sits between methods around line 370 while the other constants are at the top of the class. Checkstyle has no DeclarationOrder module so CI is fine, just a readability nit.
   - Unrelated, possibly worth its own issue: getDateAdd() only handles d/h/m/s, but the chart buttons send 1W/4W/12W, which throws on QuestDB.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@hertzbeat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@hertzbeat.apache.org
For additional commands, e-mail: notifications-help@hertzbeat.apache.org