Sicherheitsluecken S1, S2 und S3 schliessen
Neues Testprojekt tests/ClawdDotNet.Tools.Tests. Die Angriffsfaelle aus der Bestandsaufnahme bleiben darin dauerhaft als Testfaelle dokumentiert — zusammen mit Gegenproben, damit die Fixes nicht zu streng werden und legitime Nutzung blockieren. S3 — DirectAPI gab die abgerufene URL als Quelle an das Modell zurueck, samt API-Schluessel im Query-String. Der Schluessel landete damit im Konversationskontext, wurde bei jedem Folgeschritt erneut gesendet, in ChatContext.json geschrieben und in die Logs uebernommen. UrlSanitizer maskiert sensible Query-Parameter; auch die Fehlermeldungen sind betroffen und werden bereinigt. S2 — Die Kanal-/Video-Angabe wurde ungeprueft in eine Argument-Zeichenkette fuer yt-dlp interpoliert. UseShellExecute=false verhindert Shell-Metazeichen, nicht aber Options-Injection: yt-dlp kennt die Option --exec, die beliebige Befehle ausfuehrt. Kritisch, weil der Agent untrusted Inhalte verarbeitet — eine Prompt-Injection darin konnte ihn dazu bringen, genau so einen Wert zu setzen. YouTubeUrl validiert Handles und URLs gegen die zulaessigen YouTube-Hosts und lehnt alles ab, was mit einem Bindestrich beginnt. Die Argumente gehen jetzt einzeln ueber ProcessStartInfo.ArgumentList, die Adresse steht hinter dem Optionsende-Trenner. Der ffmpeg-Aufruf wurde ebenso umgestellt. S1 — Die Tabellen-Whitelist suchte den erlaubten Namen als Teilzeichenkette irgendwo im Statement, auch in Kommentaren. Bei einer Freigabe fuer prices genuegte deshalb ein DELETE auf users mit einem Kommentar, der prices enthielt, um eine beliebige Tabelle zu loeschen. Umgekehrt galten harmlose Abfragen, die ein Schluesselwort nur als Wert enthielten, faelschlich als Schreibzugriff. SqlGuard entfernt zuerst Kommentare und String-Literale, lehnt mehrere Statements ab, bestimmt die Operation am ersten Schluesselwort und extrahiert Tabellennamen gezielt hinter FROM/JOIN/INTO/UPDATE/TABLE — inklusive kommagetrennter Listen mit Aliassen. JEDE referenzierte Tabelle muss freigegeben sein, nicht irgendeine. Ohne Whitelist wird nichts durchgelassen. Die MongoDB-Pruefung vergleicht den Collection-Namen jetzt exakt statt per Teilzeichenkette. 178 Tests gruen (91 Core, 87 Tools). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Richard
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
69b5704add
commit
e5067cae70
@@ -95,22 +95,22 @@ public sealed class DatabaseTool : IAgentTool
|
||||
|
||||
var accessLevel = GetAccessLevel(context);
|
||||
|
||||
// Sicherheitsprüfungen
|
||||
if (IsAdminAttempt(sql))
|
||||
{
|
||||
if (accessLevel != DatabaseAccessLevel.Admin)
|
||||
return ToolResult.Fail("Sicherheitsfehler: Strukturänderungen (DDL) sind für diesen Agenten nicht erlaubt.");
|
||||
}
|
||||
else if (IsWriteAttempt(sql))
|
||||
{
|
||||
if (accessLevel == DatabaseAccessLevel.ReadOnly)
|
||||
return ToolResult.Fail("Sicherheitsfehler: Schreibzugriff ist für diesen Agenten deaktiviert (ReadOnly).");
|
||||
}
|
||||
// Statement analysieren: Kommentare, Mehrfach-Statements und String-Literale
|
||||
// werden dabei behandelt, bevor Operation und Tabellen bestimmt werden.
|
||||
var inspection = SqlGuard.Inspect(sql, GetAllowedTables(context));
|
||||
|
||||
// Tabellen-Whitelist-Prüfung
|
||||
if (!IsTableAllowed(sql, context))
|
||||
if (!inspection.IsValid)
|
||||
return ToolResult.Fail($"Sicherheitsfehler: {inspection.Error}");
|
||||
|
||||
switch (inspection.Operation)
|
||||
{
|
||||
return ToolResult.Fail("Sicherheitsfehler: Zugriff auf eine oder mehrere Tabellen im Statement ist nicht erlaubt.");
|
||||
case SqlOperation.Schema when accessLevel != DatabaseAccessLevel.Admin:
|
||||
return ToolResult.Fail(
|
||||
"Sicherheitsfehler: Strukturänderungen (DDL) sind für diesen Agenten nicht erlaubt.");
|
||||
|
||||
case SqlOperation.Write when accessLevel == DatabaseAccessLevel.ReadOnly:
|
||||
return ToolResult.Fail(
|
||||
"Sicherheitsfehler: Schreibzugriff ist für diesen Agenten deaktiviert (ReadOnly).");
|
||||
}
|
||||
|
||||
using DbConnection conn = type switch
|
||||
@@ -152,7 +152,7 @@ public sealed class DatabaseTool : IAgentTool
|
||||
var collectionName = input.TryGetProperty("collection", out var c) ? c.GetString() : null;
|
||||
if (string.IsNullOrWhiteSpace(collectionName)) return ToolResult.Fail("'collection' ist erforderlich.");
|
||||
|
||||
if (!IsTableAllowed(collectionName, context))
|
||||
if (!IsCollectionAllowed(collectionName, context))
|
||||
{
|
||||
return ToolResult.Fail($"Sicherheitsfehler: Zugriff auf Collection '{collectionName}' ist nicht erlaubt.");
|
||||
}
|
||||
@@ -186,18 +186,6 @@ public sealed class DatabaseTool : IAgentTool
|
||||
}
|
||||
}
|
||||
|
||||
private bool IsWriteAttempt(string sql)
|
||||
{
|
||||
var lower = sql.ToLowerInvariant();
|
||||
return lower.Contains("insert") || lower.Contains("update") || lower.Contains("delete");
|
||||
}
|
||||
|
||||
private bool IsAdminAttempt(string sql)
|
||||
{
|
||||
var lower = sql.ToLowerInvariant();
|
||||
return lower.Contains("drop") || lower.Contains("alter") || lower.Contains("create") || lower.Contains("truncate");
|
||||
}
|
||||
|
||||
private DatabaseAccessLevel GetAccessLevel(AgentToolContext context)
|
||||
{
|
||||
if (context.ToolConfig.TryGetValue("accessLevel", out var val) && val != null)
|
||||
@@ -215,16 +203,24 @@ public sealed class DatabaseTool : IAgentTool
|
||||
return DatabaseAccessLevel.ReadOnly;
|
||||
}
|
||||
|
||||
private bool IsTableAllowed(string input, AgentToolContext context)
|
||||
internal static List<string> GetAllowedTables(AgentToolContext context)
|
||||
{
|
||||
if (!context.ToolConfig.TryGetValue("allowedTables", out var val) || val is not JsonElement je)
|
||||
if (!context.ToolConfig.TryGetValue("allowedTables", out var val) || val is not JsonElement je
|
||||
|| je.ValueKind != JsonValueKind.Array)
|
||||
{
|
||||
return false;
|
||||
return [];
|
||||
}
|
||||
|
||||
var allowed = je.EnumerateArray().Select(x => x.GetString()?.ToLowerInvariant()).ToList();
|
||||
var inputLower = input.ToLowerInvariant();
|
||||
|
||||
return allowed.Any(t => t != null && inputLower.Contains(t));
|
||||
return je.EnumerateArray()
|
||||
.Select(x => x.GetString())
|
||||
.Where(s => !string.IsNullOrWhiteSpace(s))
|
||||
.ToList()!;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// MongoDB kennt kein SQL — hier genügt der exakte Vergleich des Collection-Namens.
|
||||
/// Bisher wurde auch hier per Teilzeichenkette geprüft.
|
||||
/// </summary>
|
||||
private static bool IsCollectionAllowed(string collection, AgentToolContext context)
|
||||
=> GetAllowedTables(context).Contains(collection, StringComparer.OrdinalIgnoreCase);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,251 @@
|
||||
using System.Text;
|
||||
using System.Text.RegularExpressions;
|
||||
|
||||
namespace ClawdDotNet.Tools.Database;
|
||||
|
||||
public enum SqlOperation
|
||||
{
|
||||
Unknown,
|
||||
Read, // SELECT, SHOW, DESCRIBE, EXPLAIN, WITH
|
||||
Write, // INSERT, UPDATE, DELETE, MERGE, REPLACE
|
||||
Schema // CREATE, DROP, ALTER, TRUNCATE, GRANT, …
|
||||
}
|
||||
|
||||
public sealed record SqlInspection(
|
||||
bool IsValid,
|
||||
string? Error,
|
||||
SqlOperation Operation,
|
||||
IReadOnlyList<string> Tables);
|
||||
|
||||
/// <summary>
|
||||
/// Prüft LLM-erzeugtes SQL, bevor es ausgeführt wird.
|
||||
///
|
||||
/// Die frühere Prüfung suchte den erlaubten Tabellennamen als Teilzeichenkette
|
||||
/// irgendwo im Statement — auch in einem Kommentar oder String-Literal. Bei
|
||||
/// allowedTables ["prices"] und ReadWrite genügte deshalb
|
||||
///
|
||||
/// DELETE FROM users -- prices
|
||||
///
|
||||
/// um eine beliebige andere Tabelle zu löschen. Ebenso wurde die Operation über
|
||||
/// Teilzeichenketten bestimmt, wodurch harmlose Abfragen wie
|
||||
/// <c>SELECT … WHERE note='update'</c> fälschlich als Schreibzugriff galten.
|
||||
///
|
||||
/// Diese Klasse geht anders vor:
|
||||
/// 1. Kommentare und String-Literale werden entfernt, bevor irgendetwas geprüft wird.
|
||||
/// 2. Mehrere Statements in einem Aufruf werden abgelehnt.
|
||||
/// 3. Die Operation ergibt sich aus dem ersten Schlüsselwort, nicht aus Vorkommen.
|
||||
/// 4. Tabellennamen werden gezielt hinter FROM/JOIN/INTO/UPDATE/… extrahiert;
|
||||
/// JEDER davon muss auf der Whitelist stehen.
|
||||
/// </summary>
|
||||
public static class SqlGuard
|
||||
{
|
||||
private static readonly HashSet<string> ReadKeywords =
|
||||
new(StringComparer.OrdinalIgnoreCase) { "SELECT", "SHOW", "DESCRIBE", "DESC", "EXPLAIN", "WITH" };
|
||||
|
||||
private static readonly HashSet<string> WriteKeywords =
|
||||
new(StringComparer.OrdinalIgnoreCase) { "INSERT", "UPDATE", "DELETE", "MERGE", "REPLACE", "UPSERT" };
|
||||
|
||||
private static readonly HashSet<string> SchemaKeywords =
|
||||
new(StringComparer.OrdinalIgnoreCase)
|
||||
{
|
||||
"CREATE", "DROP", "ALTER", "TRUNCATE", "GRANT", "REVOKE",
|
||||
"RENAME", "ATTACH", "DETACH", "PRAGMA", "SET", "CALL", "EXEC", "EXECUTE"
|
||||
};
|
||||
|
||||
/// <summary>Schlüsselwörter, nach denen eine Tabellenliste folgt.</summary>
|
||||
private static readonly Regex TableKeyword = new(
|
||||
@"\b(?:FROM|JOIN|INTO|UPDATE|TABLE)\b",
|
||||
RegexOptions.IgnoreCase | RegexOptions.Compiled);
|
||||
|
||||
/// <summary>Beendet eine Tabellenliste.</summary>
|
||||
private static readonly Regex ListTerminator = new(
|
||||
@"\b(?:WHERE|GROUP|ORDER|HAVING|LIMIT|ON|USING|SET|VALUES|UNION|EXCEPT|INTERSECT|" +
|
||||
@"JOIN|INNER|LEFT|RIGHT|FULL|CROSS|OUTER|NATURAL|SELECT|RETURNING|WINDOW|FETCH|OFFSET|" +
|
||||
@"ADD|DROP|RENAME|MODIFY|ALTER)\b",
|
||||
RegexOptions.IgnoreCase | RegexOptions.Compiled);
|
||||
|
||||
/// <summary>Erster Bezeichner eines Listeneintrags — ein evtl. folgender Alias wird ignoriert.</summary>
|
||||
private static readonly Regex LeadingIdentifier = new(
|
||||
@"^[\s`\[""]*(?<table>[A-Za-z_][A-Za-z0-9_$]*(?:\.[A-Za-z_][A-Za-z0-9_$]*)?)",
|
||||
RegexOptions.Compiled);
|
||||
|
||||
public static SqlInspection Inspect(string? sql, IReadOnlyCollection<string> allowedTables)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(sql))
|
||||
return Invalid("Leeres SQL-Statement.");
|
||||
|
||||
// Kommentare sind für maschinell erzeugtes SQL ohne Nutzen, aber das ideale
|
||||
// Versteck, um eine erlaubte Tabelle vorzutäuschen.
|
||||
if (ContainsComment(sql))
|
||||
return Invalid("SQL-Kommentare sind nicht erlaubt.");
|
||||
|
||||
var normalized = RemoveStringLiterals(sql);
|
||||
|
||||
if (HasMultipleStatements(normalized))
|
||||
return Invalid("Mehrere Statements in einem Aufruf sind nicht erlaubt.");
|
||||
|
||||
var operation = DetermineOperation(normalized);
|
||||
if (operation == SqlOperation.Unknown)
|
||||
return Invalid("Statement-Art konnte nicht bestimmt werden.");
|
||||
|
||||
var tables = ExtractTables(normalized);
|
||||
if (tables.Count == 0)
|
||||
return new SqlInspection(false, "Keine Tabelle im Statement erkannt.", operation, tables);
|
||||
|
||||
// Fail closed: ohne Whitelist wird nichts durchgelassen.
|
||||
if (allowedTables.Count == 0)
|
||||
return new SqlInspection(false, "Keine Tabellen freigegeben (allowedTables fehlt).", operation, tables);
|
||||
|
||||
var allowed = new HashSet<string>(allowedTables, StringComparer.OrdinalIgnoreCase);
|
||||
var denied = tables.Where(t => !allowed.Contains(StripSchema(t))).ToList();
|
||||
|
||||
if (denied.Count > 0)
|
||||
{
|
||||
return new SqlInspection(false,
|
||||
$"Zugriff auf nicht freigegebene Tabelle(n): {string.Join(", ", denied)}. " +
|
||||
$"Erlaubt sind: {string.Join(", ", allowedTables)}.",
|
||||
operation, tables);
|
||||
}
|
||||
|
||||
return new SqlInspection(true, null, operation, tables);
|
||||
}
|
||||
|
||||
// ─── Bausteine ───
|
||||
|
||||
private static SqlInspection Invalid(string error)
|
||||
=> new(false, error, SqlOperation.Unknown, []);
|
||||
|
||||
private static bool ContainsComment(string sql)
|
||||
=> sql.Contains("--", StringComparison.Ordinal)
|
||||
|| sql.Contains("/*", StringComparison.Ordinal)
|
||||
|| sql.Contains('#');
|
||||
|
||||
/// <summary>
|
||||
/// Ersetzt den Inhalt von String-Literalen durch Leerraum, damit dort weder
|
||||
/// Schlüsselwörter noch Tabellennamen erkannt werden.
|
||||
/// </summary>
|
||||
private static string RemoveStringLiterals(string sql)
|
||||
{
|
||||
var sb = new StringBuilder(sql.Length);
|
||||
var inSingle = false;
|
||||
var inDouble = false;
|
||||
|
||||
for (var i = 0; i < sql.Length; i++)
|
||||
{
|
||||
var c = sql[i];
|
||||
|
||||
if (inSingle)
|
||||
{
|
||||
if (c == '\'')
|
||||
{
|
||||
// Verdoppeltes Apostroph ist ein escaptes Zeichen, kein Ende.
|
||||
if (i + 1 < sql.Length && sql[i + 1] == '\'') { i++; continue; }
|
||||
inSingle = false;
|
||||
sb.Append(' ');
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
if (inDouble)
|
||||
{
|
||||
if (c == '"')
|
||||
{
|
||||
if (i + 1 < sql.Length && sql[i + 1] == '"') { i++; continue; }
|
||||
inDouble = false;
|
||||
sb.Append(' ');
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
switch (c)
|
||||
{
|
||||
case '\'':
|
||||
inSingle = true;
|
||||
sb.Append(' ');
|
||||
break;
|
||||
case '"':
|
||||
inDouble = true;
|
||||
sb.Append(' ');
|
||||
break;
|
||||
default:
|
||||
sb.Append(c);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
return sb.ToString();
|
||||
}
|
||||
|
||||
private static bool HasMultipleStatements(string normalized)
|
||||
{
|
||||
var trimmed = normalized.TrimEnd();
|
||||
var semicolon = trimmed.IndexOf(';');
|
||||
|
||||
// Ein abschließendes Semikolon ist in Ordnung, alles danach nicht.
|
||||
return semicolon >= 0 && semicolon < trimmed.Length - 1;
|
||||
}
|
||||
|
||||
private static SqlOperation DetermineOperation(string normalized)
|
||||
{
|
||||
var firstWord = normalized
|
||||
.Split([' ', '\t', '\r', '\n', '(', ';'], StringSplitOptions.RemoveEmptyEntries)
|
||||
.FirstOrDefault();
|
||||
|
||||
if (firstWord is null)
|
||||
return SqlOperation.Unknown;
|
||||
|
||||
if (SchemaKeywords.Contains(firstWord)) return SqlOperation.Schema;
|
||||
if (WriteKeywords.Contains(firstWord)) return SqlOperation.Write;
|
||||
if (ReadKeywords.Contains(firstWord)) return SqlOperation.Read;
|
||||
|
||||
return SqlOperation.Unknown;
|
||||
}
|
||||
|
||||
private static List<string> ExtractTables(string normalized)
|
||||
{
|
||||
var tables = new List<string>();
|
||||
|
||||
foreach (Match keyword in TableKeyword.Matches(normalized))
|
||||
{
|
||||
var rest = normalized[(keyword.Index + keyword.Length)..];
|
||||
tables.AddRange(ParseTableList(rest));
|
||||
}
|
||||
|
||||
return tables.Distinct(StringComparer.OrdinalIgnoreCase).ToList();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Liest die auf ein Schlüsselwort folgende Tabellenliste — auch mehrere durch Komma
|
||||
/// getrennte Einträge samt Aliassen ("FROM prices p, users u").
|
||||
/// </summary>
|
||||
private static IEnumerable<string> ParseTableList(string rest)
|
||||
{
|
||||
// Die Liste endet am nächsten Schlüsselwort oder an einer öffnenden Klammer
|
||||
// (Spaltenliste bei INSERT, Unterabfrage bei FROM).
|
||||
var end = rest.Length;
|
||||
|
||||
var terminator = ListTerminator.Match(rest);
|
||||
if (terminator.Success)
|
||||
end = Math.Min(end, terminator.Index);
|
||||
|
||||
var parenthesis = rest.IndexOf('(');
|
||||
if (parenthesis >= 0)
|
||||
end = Math.Min(end, parenthesis);
|
||||
|
||||
var segment = rest[..end];
|
||||
|
||||
foreach (var entry in segment.Split(','))
|
||||
{
|
||||
var match = LeadingIdentifier.Match(entry);
|
||||
if (match.Success)
|
||||
yield return match.Groups["table"].Value;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>"schema.tabelle" → "tabelle" (die Whitelist listet Tabellennamen).</summary>
|
||||
private static string StripSchema(string table)
|
||||
{
|
||||
var dot = table.LastIndexOf('.');
|
||||
return dot >= 0 ? table[(dot + 1)..] : table;
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user