From 50f88b938fc058606a638cec6babeb247edd037d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 24 Jul 2026 14:07:54 -0400 Subject: [PATCH] fix(sql): reject Unicode format chars in identifiers; stop overclaiming catalog validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the code review of the dialect seam: - QuoteIdentifier used char.IsControl, which only covers Unicode category Cc. Bidi overrides, zero-width spaces, soft hyphen and BOM are category Cf and passed through untouched. They cannot escape the brackets, so this is not an injection bypass — but the method rejects control characters precisely to protect the integrity of logged/audited statements, and Cf characters spoof rendered text while comparing byte-different from the real catalog name (Trojan Source, CVE-2021-42574). Same rule, same rationale, wider category. - The XML docs on both files asserted that identifiers 'are sourced only from catalog-validated (INFORMATION_SCHEMA) names' and that rejection is 'a backstop, not the primary defence'. Neither is true yet: design §8.1 does specify that gate, but nothing implements it and no task in the plan schedules one, so an authored TagConfig table/column reaches QuoteIdentifier unfiltered. The docs now say so, because under-scrutinising this method on the strength of a filter that does not exist is exactly the failure mode. - The control-character test only pinned NUL, so a regression to a Contains('\0') check would still have passed. Pinned the whole Cc category plus the new Cf rule; verified load-bearing by deleting the guard and observing exactly the 5 new cases go red. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW --- .../ISqlDialect.cs | 12 ++++++---- .../SqlServerDialect.cs | 20 +++++++++++++--- .../SqlServerDialectTests.cs | 23 +++++++++++++++++++ 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/ISqlDialect.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/ISqlDialect.cs index f6b37df0..9b15d22e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/ISqlDialect.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/ISqlDialect.cs @@ -11,10 +11,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// Those live here, so no dialect assumption leaks into the reader, the poll planner, or the schema browser. /// This interface is the driver's SQL-injection boundary. Every runtime value is /// bound as a and never reaches a command text. Identifiers cannot be -/// parameterized in SQL, so the few that must appear as text are sourced only from catalog-validated -/// (INFORMATION_SCHEMA) names and emitted through . Any code path that -/// builds SQL by concatenating an authored tag field without going through -/// is a defect (design §8.1). +/// parameterized in SQL, so the few that must appear as text are emitted through +/// . Any code path that builds SQL by concatenating an authored tag field +/// without going through is a defect (design §8.1). +/// Not yet implemented: design §8.1 also specifies that an authored table/column is +/// validated against the live catalog before it is ever quoted into text, so that an unknown identifier +/// rejects the tag. No such gate exists yet — an identifier reaches +/// straight from the authored TagConfig blob, so quoting is currently the only defence, +/// not a backstop behind an upstream filter. Scrutinise it accordingly. /// Public because Driver.Sql.Browser consumes it — the catalog SQL is the browse /// engine, so it is shared rather than duplicated. Implementations own their provider package; /// no provider-specific type appears in this signature ( is the abstract diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlServerDialect.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlServerDialect.cs index 2e76821b..e666856f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlServerDialect.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlServerDialect.cs @@ -1,4 +1,5 @@ using System.Data.Common; +using System.Globalization; using Microsoft.Data.SqlClient; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; @@ -61,11 +62,17 @@ public sealed class SqlServerDialect : ISqlDialect /// /// Rejects (all as ): null / empty / all-whitespace; longer /// than ; any control character (which covers embedded NUL, and the - /// C0/C1 ranges that can truncate or corrupt a logged/audited statement). + /// C0/C1 ranges that can truncate or corrupt a logged/audited statement); and any Unicode + /// character — bidi overrides, zero-width + /// spaces, soft hyphen, BOM. Those last cannot escape the brackets, but they spoof the rendered + /// text of a log line or an AdminUI label while comparing byte-different from the real catalog name + /// (the Trojan-Source class, CVE-2021-42574) — the same log/audit-integrity concern that motivates the + /// control-character rule. /// The rejection messages deliberately do not echo the offending value — it is untrusted /// input and this exception's text reaches the driver log. - /// Rejection is a backstop, not the primary defence: an identifier only reaches here after being - /// validated against the live catalog (design §8.1). + /// This is currently the only defence, not a backstop. Design §8.1 specifies that an + /// identifier is validated against the live catalog before reaching here; that gate is not implemented + /// yet, so an authored TagConfig table/column arrives unfiltered. /// /// The bare, unquoted identifier part. /// The bracket-quoted identifier. @@ -86,6 +93,13 @@ public sealed class SqlServerDialect : ISqlDialect if (char.IsControl(ch)) throw new ArgumentException( "A SQL identifier may not contain control characters (including NUL).", nameof(ident)); + + // Cf is a *separate* category from Cc, so char.IsControl misses every bidi override and + // zero-width character. They are inert inside brackets but spoof rendered log/UI text. + if (CharUnicodeInfo.GetUnicodeCategory(ch) == UnicodeCategory.Format) + throw new ArgumentException( + "A SQL identifier may not contain Unicode format characters " + + "(bidi overrides, zero-width spaces, soft hyphen, BOM).", nameof(ident)); } return string.Concat("[", ident.Replace("]", "]]", StringComparison.Ordinal), "]"); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs index af0e4c0f..e10995bc 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs @@ -25,6 +25,29 @@ public class SqlServerDialectTests public void QuoteIdentifier_rejectsControlCharsAndNul() => Should.Throw(() => new SqlServerDialect().QuoteIdentifier("x\0y")); + // NUL alone would still pass against a `Contains('\0')` regression, so pin the whole Cc category. + [Theory] + [InlineData("x\u0001y")] // C0 SOH + [InlineData("x\u001By")] // C0 ESC + [InlineData("x\u007Fy")] // DEL + [InlineData("x\u0085y")] // C1 NEL + [InlineData("x\ty")] + [InlineData("x\ny")] + public void QuoteIdentifier_rejectsEveryControlCharacter_notJustNul(string ident) + => Should.Throw(() => new SqlServerDialect().QuoteIdentifier(ident)); + + // Unicode Format (Cf) is a distinct category from Control (Cc): char.IsControl returns false for all + // of these. They cannot escape the brackets, but they spoof rendered log/AdminUI text while comparing + // byte-different from the real catalog name (Trojan Source, CVE-2021-42574). + [Theory] + [InlineData("Sp\u200Beed")] // zero-width space + [InlineData("\u202Ediop.selbat")] // right-to-left override + [InlineData("x\u2066y\u2069")] // bidi isolates + [InlineData("x\uFEFFy")] // BOM / zero-width no-break space + [InlineData("x\u00ADy")] // soft hyphen + public void QuoteIdentifier_rejectsUnicodeFormatCharacters(string ident) + => Should.Throw(() => new SqlServerDialect().QuoteIdentifier(ident)); + [Theory] [InlineData("bit", DriverDataType.Boolean)] [InlineData("int", DriverDataType.Int32)]