dohertj2/lmxopcua
1
0
Fork 0
Code Issues 16 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(sql): reject Unicode format chars in identifiers; stop overclaiming catalog validation

Browse Source 
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
This commit is contained in:
Joseph Doherty
2026-07-24 14:07:54 -04:00
parent a05cff794c
commit 50f88b938f
3 changed files with 48 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/ISqlDialect.cs
+8 -4
View File
@@ -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.
/// <para><b>This interface is the driver's SQL-injection boundary.</b> Every runtime <em>value</em> is
/// bound as a <see cref="DbParameter"/> 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
/// (<c>INFORMATION_SCHEMA</c>) names and emitted through <see cref="QuoteIdentifier"/>. Any code path that
/// builds SQL by concatenating an authored tag field <em>without</em> going through
/// <see cref="QuoteIdentifier"/> is a defect (design §8.1).</para>
/// parameterized in SQL, so the few that must appear as text are emitted through
/// <see cref="QuoteIdentifier"/>. Any code path that builds SQL by concatenating an authored tag field
/// <em>without</em> going through <see cref="QuoteIdentifier"/> is a defect (design §8.1).</para>
/// <para><b>Not yet implemented:</b> 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 <see cref="QuoteIdentifier"/>
/// straight from the authored <c>TagConfig</c> blob, so <b>quoting is currently the only defence</b>,
/// not a backstop behind an upstream filter. Scrutinise it accordingly.</para>
/// <para>Public because <c>Driver.Sql.Browser</c> consumes it — the catalog SQL <em>is</em> the browse
/// engine, so it is shared rather than duplicated. Implementations own their provider package;
/// <b>no provider-specific type appears in this signature</b> (<see cref="Factory"/> is the abstract
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlServerDialect.cs
+17 -3
View File
@@ -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
/// <remarks>
/// <para><b>Rejects</b> (all as <see cref="ArgumentException"/>): null / empty / all-whitespace; longer
/// than <see cref="MaxIdentifierLength"/>; any control character (which covers embedded NUL, and the
/// C0/C1 ranges that can truncate or corrupt a logged/audited statement).</para>
/// C0/C1 ranges that can truncate or corrupt a logged/audited statement); and any Unicode
/// <see cref="System.Globalization.UnicodeCategory.Format"/> character — bidi overrides, zero-width
/// spaces, soft hyphen, BOM. Those last cannot escape the brackets, but they spoof the <em>rendered</em>
/// 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.</para>
/// <para>The rejection messages deliberately do <b>not</b> echo the offending value — it is untrusted
/// input and this exception's text reaches the driver log.</para>
/// <para>Rejection is a backstop, not the primary defence: an identifier only reaches here after being
/// validated against the live catalog (design §8.1).</para>
/// <para><b>This is currently the only defence, not a backstop.</b> 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 <c>TagConfig</c> table/column arrives unfiltered.</para>
/// </remarks>
/// <param name="ident">The bare, unquoted identifier part.</param>
/// <returns>The bracket-quoted identifier.</returns>
@@ -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), "]");
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs
+23
View File
@@ -25,6 +25,29 @@ public class SqlServerDialectTests
public void QuoteIdentifier_rejectsControlCharsAndNul()
=> Should.Throw<ArgumentException>(() => 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<ArgumentException>(() => 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<ArgumentException>(() => new SqlServerDialect().QuoteIdentifier(ident));
[Theory]
[InlineData("bit", DriverDataType.Boolean)]
[InlineData("int", DriverDataType.Int32)]