4dc2ad8084
Until now ISqlDialect.QuoteIdentifier was the SOLE defence for authored
table/column names: an identifier went from the TagConfig blob straight into a
command text, bracket-quoted. The residual risk was bounded — a hostile name is
quoted into one nonexistent object, the query fails after the connection opens,
and the tag Bad-codes — but "bounded" is not "filtered", and the design promised
a filter. Both ISqlDialect and SqlServerDialect carried a doc paragraph saying so.
SqlCatalogGate + SqlCatalogLoader now resolve every authored identifier against
the live catalog at Initialize, and REPLACE it with the catalog's own spelling.
The identifier text in an emitted poll query is therefore a string this driver
read back out of ListSchemas/ListTables/ListColumns — not operator input. Quoting
becomes the backstop it was documented to be.
Decisions worth knowing before touching this:
- Substitution, not just validation. Matching is exact-ordinal first, then a
UNIQUE case-insensitive hit: SQL Server's default collation is CI so case
variants have always worked, and rejecting them would break valid deployments.
An ambiguous CI match under a case-sensitive collation is refused rather than
guessed — picking one would publish another column's data under the operator's
node, which is worse than rejecting the tag.
- Charset check BEFORE catalog lookup. Each identifier goes through
QuoteIdentifier for its rejection rules first, so a name carrying a control or
Unicode format character is rejected WITHOUT its value being echoed into a log
line (Trojan-Source). A name that passes is safe to render, which is why
catalog-miss messages do name it — an operator hunting a typo has to see what
they wrote. Both halves are pinned by tests.
- A rejected tag keeps its node. It is dropped from the POLLED table but stays in
the AUTHORED table, so it still materializes and reads BadNodeIdUnknown —
§8.1's specified outcome. My first wiring dropped it from both, which deleted
the node instead; an existing test (ReinitializeAsync_recoversFromFaulted)
caught it. A status code can only be published by a node that exists, and a
missing address-space entry is far harder to diagnose than a Bad quality.
- An unreadable catalog FAULTS Initialize; it does not reject every tag. That is
the absence of evidence about the tags, not evidence against them — rejecting
all of them would serve a confidently-empty address space and send the operator
hunting typos that do not exist. Zero visible schemas is treated the same way,
because that is exactly what a missing GRANT looks like. Faulting lands
DriverInstanceActor in Reconnecting with its retry timer running.
- Bounded load: one schema list, one default-schema scalar, then one ListTables
per distinct authored schema and one ListColumns per distinct authored relation
— never a full catalog enumeration, and nothing after Initialize. Every
authored name reaches the catalog queries as a bound @schema/@table parameter,
so building the allow-list cannot itself be an injection vector.
- ISqlDialect gains DefaultSchemaSql ("SELECT SCHEMA_NAME()"). An unqualified
`TagValues` must resolve in whatever schema the SERVER reports; hardcoding dbo
would be a silent lie on any estate that maps service accounts to their own
default schema. It is a query, not a constant, because the answer is
per-connection.
- Accepted v1 limitation: a 3-part db.schema.table (or linked-server name)
addresses a catalog this connection cannot enumerate, so it cannot be
allow-listed and is rejected with a message pointing at the fix — expose the
data through a view in the connected database.
VerifyLivenessAsync's wall-clock pattern is extracted to RunBoundedAsync and
reused, so the new I/O gets the same R2-01 / STAB-14 protection rather than a
second hand-rolled copy. (It also fixes a latent bug in the extracted code: the
OperationCanceledException arm detached the wrong task.)
Tests: 24 pure gate tests + 9 end-to-end driver tests against the real SQLite
catalog + 3 new live tests against the real SQL Server on 10.100.0.35 (21 in that
suite now pass, exercising the real SELECT SCHEMA_NAME() + INFORMATION_SCHEMA
path). Verified falsifiable: bypassing the gate turns exactly the three rejection
assertions red and leaves the rest green.
SqlInjectionRegressionTests is deliberately NOT rewritten to expect
BadNodeIdUnknown. It drives SqlPollReader directly, below the gate, and pins the
quoting backstop on its own — defence in depth is only worth the name if each
layer holds independently. Rewriting those assertions would delete the backstop's
only coverage and leave the gate a single point of failure. Its scope note, which
said the gate does not exist, is updated to say why it stays where it is.
174 lines
9.2 KiB
C#
174 lines
9.2 KiB
C#
using Microsoft.Data.Sqlite;
|
|
using Shouldly;
|
|
using Xunit;
|
|
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
|
using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts;
|
|
|
|
namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests;
|
|
|
|
/// <summary>
|
|
/// Locks the driver's SQL-injection guarantee against a real database (<see cref="SqlitePollFixture"/>):
|
|
/// an authored <em>value</em> is bound as a parameter and can never execute, and an authored
|
|
/// <em>identifier</em> — which cannot be parameterized in SQL — is dialect-quoted so a hostile one becomes
|
|
/// an inert reference to a nonexistent object rather than executable text. If these pass, a malicious tag
|
|
/// cannot alter the database; the seed table is intact after every hostile poll.
|
|
/// <para><b>Scope note — this suite deliberately tests the layer BELOW the catalog gate.</b> Design
|
|
/// §8.1's catalog gate now exists (Gitea #496): <see cref="SqlCatalogGate"/> validates every authored
|
|
/// table/column against the live catalog at Initialize, so in the assembled driver a hostile identifier
|
|
/// is rejected up front and its tag reads <see cref="SqlStatusCodes.BadNodeIdUnknown"/> — proven by
|
|
/// <c>SqlCatalogGateDriverTests</c>.</para>
|
|
/// <para>The tests here construct a <see cref="SqlPollReader"/> <b>directly</b>, with hand-built
|
|
/// definitions that never pass through the gate, and that is the point: defence in depth is only worth
|
|
/// the name if each layer holds on its own. These assertions pin the <em>quoting backstop</em> — that
|
|
/// even with the allow-list bypassed entirely, a hostile identifier becomes an inert reference to a
|
|
/// nonexistent object, the query fails after the connection opened, the tag Bad-codes as a query failure
|
|
/// (<see cref="SqlStatusCodes.BadCommunicationError"/>), and the seed table survives. Rewriting them to
|
|
/// expect <c>BadNodeIdUnknown</c> would delete the only coverage the backstop has and leave the gate as a
|
|
/// single point of failure.</para>
|
|
/// </summary>
|
|
public sealed class SqlInjectionRegressionTests
|
|
{
|
|
private static readonly TimeSpan CommandTimeout = TimeSpan.FromSeconds(10);
|
|
private static readonly TimeSpan OperationTimeout = TimeSpan.FromSeconds(15);
|
|
|
|
/// <summary>The classic payload: were the key concatenated into text, this would drop the table.</summary>
|
|
private const string DropTablePayload = "'; DROP TABLE TagValues; --";
|
|
|
|
[Fact]
|
|
public async Task MaliciousKeyValue_bindsHarmlessly_tableSurvives()
|
|
{
|
|
using var fixture = new SqlitePollFixture();
|
|
var reader = NewReader(fixture, KvTag("Speed", DropTablePayload));
|
|
|
|
var snapshots = await reader.ReadAsync(["Speed"], CancellationToken.None);
|
|
|
|
// Bound, not executed: it is simply a key that matches no row.
|
|
snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.BadNoData);
|
|
// The table is still there, with its seeded rows — the DROP never ran.
|
|
(await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable)).ShouldBeGreaterThan(0);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task MaliciousKeyValue_doesNotEvenDisturbAHealthyNeighbourOnTheSamePoll()
|
|
{
|
|
// A hostile key in one slot must not corrupt a legitimate tag sharing the poll — it binds as its own
|
|
// parameter and matches nothing; the real key still reads.
|
|
using var fixture = new SqlitePollFixture();
|
|
var reader = NewReader(fixture,
|
|
KvTag("Evil", DropTablePayload),
|
|
KvTag("Speed", SqlitePollFixture.PresentKey));
|
|
|
|
var snapshots = await reader.ReadAsync(["Evil", "Speed"], CancellationToken.None);
|
|
|
|
snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.BadNoData);
|
|
snapshots[1].Value.ShouldBe(SqlitePollFixture.PresentValue);
|
|
(await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable)).ShouldBeGreaterThan(0);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task MaliciousTableIdentifier_isInert_neverExecutesAndTheSeedSurvives()
|
|
{
|
|
using var fixture = new SqlitePollFixture();
|
|
var seedRows = await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable);
|
|
|
|
// A table name carrying a statement terminator + DROP. The reader is driven directly, so the
|
|
// catalog gate never sees it and the quoting backstop is on its own: the name is quoted into one
|
|
// nonexistent identifier, the query fails (no such table) with the connection already open, so the
|
|
// tag Bad-codes as a query failure. The DROP never runs. In the assembled driver the gate rejects
|
|
// this first and the tag reads BadNodeIdUnknown instead — see SqlCatalogGateDriverTests.
|
|
var reader = NewReader(fixture,
|
|
new SqlTagDefinition(
|
|
Name: "Evil",
|
|
Model: SqlTagModel.KeyValue,
|
|
Table: "TagValues\"; DROP TABLE TagValues; --",
|
|
KeyColumn: SqlitePollFixture.KeyColumn,
|
|
KeyValue: SqlitePollFixture.PresentKey,
|
|
ValueColumn: SqlitePollFixture.ValueColumn,
|
|
TimestampColumn: SqlitePollFixture.TimestampColumn));
|
|
|
|
var snapshots = await reader.ReadAsync(["Evil"], CancellationToken.None);
|
|
|
|
// Bad-coded as a QUERY failure, not BadNodeIdUnknown — this reader was built without the gate.
|
|
SqlStatusCodes.IsBad(snapshots[0].StatusCode).ShouldBeTrue();
|
|
snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.BadCommunicationError);
|
|
// The load-bearing assertion: the payload did NOT execute. The table and its rows are untouched.
|
|
(await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable)).ShouldBe(seedRows);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task MaliciousColumnIdentifier_isInert_neverExecutesAndTheSeedSurvives()
|
|
{
|
|
using var fixture = new SqlitePollFixture();
|
|
var seedRows = await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable);
|
|
|
|
// The same attack via the value column: quoted, so it becomes a nonexistent column reference; the
|
|
// SELECT fails and the tag Bad-codes. Nothing executes.
|
|
var reader = NewReader(fixture,
|
|
new SqlTagDefinition(
|
|
Name: "Evil",
|
|
Model: SqlTagModel.KeyValue,
|
|
Table: SqlitePollFixture.KeyValueTable,
|
|
KeyColumn: SqlitePollFixture.KeyColumn,
|
|
KeyValue: SqlitePollFixture.PresentKey,
|
|
ValueColumn: "num_value\"; DROP TABLE TagValues; --",
|
|
TimestampColumn: SqlitePollFixture.TimestampColumn));
|
|
|
|
var snapshots = await reader.ReadAsync(["Evil"], CancellationToken.None);
|
|
|
|
SqlStatusCodes.IsBad(snapshots[0].StatusCode).ShouldBeTrue();
|
|
(await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable)).ShouldBe(seedRows);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task AControlCharacterIdentifier_isRejectedByQuoting_beforeItReachesTheDatabase()
|
|
{
|
|
// QuoteIdentifier refuses a NUL/control-character identifier outright (it cannot name a real object).
|
|
// The planner throws ArgumentException, which the reader classes as an authoring fault —
|
|
// BadConfigurationError — rather than a database failure. Still inert: the table survives.
|
|
using var fixture = new SqlitePollFixture();
|
|
var seedRows = await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable);
|
|
|
|
var reader = NewReader(fixture,
|
|
new SqlTagDefinition(
|
|
Name: "Evil",
|
|
Model: SqlTagModel.KeyValue,
|
|
Table: "Tag\0Values",
|
|
KeyColumn: SqlitePollFixture.KeyColumn,
|
|
KeyValue: SqlitePollFixture.PresentKey,
|
|
ValueColumn: SqlitePollFixture.ValueColumn,
|
|
TimestampColumn: SqlitePollFixture.TimestampColumn));
|
|
|
|
var snapshots = await reader.ReadAsync(["Evil"], CancellationToken.None);
|
|
|
|
snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.BadConfigurationError);
|
|
(await RowCountAsync(fixture, SqlitePollFixture.KeyValueTable)).ShouldBe(seedRows);
|
|
}
|
|
|
|
// ---- helpers ----
|
|
|
|
private static SqlPollReader NewReader(SqlitePollFixture fixture, params SqlTagDefinition[] tags)
|
|
{
|
|
var table = tags.ToDictionary(t => t.Name, StringComparer.Ordinal);
|
|
return new SqlPollReader(
|
|
fixture.Factory, fixture.ConnectionString, new SqliteDialect(),
|
|
commandTimeout: CommandTimeout, operationTimeout: OperationTimeout,
|
|
maxConcurrentGroups: 4, nullIsBad: false,
|
|
resolve: rawPath => table.GetValueOrDefault(rawPath));
|
|
}
|
|
|
|
private static SqlTagDefinition KvTag(string name, string keyValue)
|
|
=> new(name, SqlTagModel.KeyValue, SqlitePollFixture.KeyValueTable,
|
|
KeyColumn: SqlitePollFixture.KeyColumn, KeyValue: keyValue,
|
|
ValueColumn: SqlitePollFixture.ValueColumn,
|
|
TimestampColumn: SqlitePollFixture.TimestampColumn);
|
|
|
|
/// <summary>Counts rows in a table over the fixture's own connection — the direct evidence a DROP did not
|
|
/// run. The table name is a test constant, never authored input, so interpolating it here is safe.</summary>
|
|
private static async Task<long> RowCountAsync(SqlitePollFixture fixture, string table)
|
|
{
|
|
await using var command = fixture.Connection.CreateCommand();
|
|
command.CommandText = $"SELECT COUNT(*) FROM \"{table}\"";
|
|
return Convert.ToInt64(await command.ExecuteScalarAsync());
|
|
}
|
|
}
|