From e9ea5f82059ee36d840ca0d076ce23c3cbbbd9c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Jun 2026 20:57:18 +0000 Subject: [PATCH] Neutralize CSV formula injection in recording output (CWE-1236) DisplayName, NodeId, Value and Status originate from the OPC UA server, which is potentially untrusted on a plant network. EscapeCsvField was RFC 4180-correct but wrote fields beginning with '=', '+', '-', '@', tab, or CR verbatim, so a DisplayName like =cmd|'/C calc'!A0 executed as a formula when the CSV was opened in Excel/LibreOffice. Such fields are now prefixed with a single quote ('), which spreadsheets interpret as "treat as text". Exception: fields that parse as a number under InvariantCulture (double.TryParse, NumberStyles.Float) are left untouched - recorded values are routinely negative numbers (-12.5) and prefixing them would corrupt the data column for downstream tools. Neutralization runs before RFC 4180 quoting and applies to every server-supplied field; the timestamp is generated locally in a fixed format and the header line is constant, so neither needs it. Tests: formula DisplayName neutralized, -12.5 value untouched, +SomeTag neutralized, and combined neutralization + RFC 4180 quoting. https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie --- .../Utilities/CsvRecordingManagerTests.cs | 101 ++++++++++++++++++ Utilities/CsvRecordingManager.cs | 41 ++++++- 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/Tests/Opcilloscope.Tests/Utilities/CsvRecordingManagerTests.cs b/Tests/Opcilloscope.Tests/Utilities/CsvRecordingManagerTests.cs index f23f2b7..891a16f 100644 --- a/Tests/Opcilloscope.Tests/Utilities/CsvRecordingManagerTests.cs +++ b/Tests/Opcilloscope.Tests/Utilities/CsvRecordingManagerTests.cs @@ -700,6 +700,107 @@ public void GenerateDefaultRecordingFilename_UsesGregorianCalendar_UnderThaiCult }); } + [Fact] + public void RecordValue_NeutralizesFormulaInjection_InDisplayName() + { + // Arrange - DisplayName originates from the (potentially untrusted) + // OPC UA server. "=cmd|'/C calc'!A0" executes as a formula when the + // CSV is opened in Excel/LibreOffice (CWE-1236). + var filePath = Path.Combine(_testDirectory, "test.csv"); + _manager.StartRecording(filePath); + var node = new MonitoredNode + { + DisplayName = "=cmd|'/C calc'!A0", + NodeId = new NodeId(1234), + Value = "100" + }; + + // Act + _manager.RecordValue(node); + _manager.StopRecording(); // waits for the background writer to drain + + // Assert - the field must be prefixed with a single quote so + // spreadsheets treat it as text. + var content = File.ReadAllText(filePath); + Assert.Contains(",'=cmd|'/C calc'!A0,", content); + Assert.DoesNotContain(",=cmd", content); + } + + [Fact] + public void RecordValue_DoesNotNeutralizeNegativeNumericValue() + { + // Arrange - recorded values are routinely negative numbers; they are + // inert in spreadsheets and prefixing them would corrupt the data + // column for downstream tools. + var filePath = Path.Combine(_testDirectory, "test.csv"); + _manager.StartRecording(filePath); + var node = new MonitoredNode + { + DisplayName = "TestNode", + NodeId = new NodeId(1234), + Value = "-12.5", + RawValue = "-12.5" + }; + + // Act + _manager.RecordValue(node); + _manager.StopRecording(); + + // Assert - written verbatim, no neutralization prefix. + var content = File.ReadAllText(filePath); + Assert.Contains(",-12.5,", content); + Assert.DoesNotContain("'-12.5", content); + } + + [Fact] + public void RecordValue_NeutralizesPlusPrefixedDisplayName() + { + // Arrange - "+SomeTag" starts with a formula trigger and is not a + // valid invariant-culture number, so it must be neutralized. + var filePath = Path.Combine(_testDirectory, "test.csv"); + _manager.StartRecording(filePath); + var node = new MonitoredNode + { + DisplayName = "+SomeTag", + NodeId = new NodeId(1234), + Value = "100" + }; + + // Act + _manager.RecordValue(node); + _manager.StopRecording(); + + // Assert + var content = File.ReadAllText(filePath); + Assert.Contains(",'+SomeTag,", content); + Assert.DoesNotContain(",+SomeTag,", content); + } + + [Fact] + public void RecordValue_CombinesNeutralizationWithRfc4180Quoting() + { + // Arrange - a field that both starts with a formula trigger and + // contains a comma must be neutralized AND wrapped in quotes. + var filePath = Path.Combine(_testDirectory, "test.csv"); + _manager.StartRecording(filePath); + var node = new MonitoredNode + { + DisplayName = "=HYPERLINK(\"http://evil\",\"click\")", + NodeId = new NodeId(1234), + Value = "=1+2,cmd" + }; + + // Act + _manager.RecordValue(node); + _manager.StopRecording(); + + // Assert - neutralizing quote prefix inside the RFC 4180 quoted field, + // with internal quotes doubled. + var content = File.ReadAllText(filePath); + Assert.Contains("\"'=HYPERLINK(\"\"http://evil\"\",\"\"click\"\")\"", content); + Assert.Contains("\"'=1+2,cmd\"", content); + } + [Fact] public void StartRecording_ClearsStaleQueueFromPreviousSession() { diff --git a/Utilities/CsvRecordingManager.cs b/Utilities/CsvRecordingManager.cs index 91235d5..064b072 100644 --- a/Utilities/CsvRecordingManager.cs +++ b/Utilities/CsvRecordingManager.cs @@ -482,7 +482,10 @@ private void WriteRecord(RecordSnapshot item) var timestamp = item.Timestamp?.ToString("yyyy-MM-ddTHH:mm:ss.fff", CultureInfo.InvariantCulture) ?? DateTime.Now.ToString("yyyy-MM-ddTHH:mm:ss.fff", CultureInfo.InvariantCulture); - // Escape values for CSV (handle quotes and commas) + // Escape values for CSV (RFC 4180 quoting plus formula + // injection neutralization for server-supplied fields). + // The timestamp is generated locally in a fixed format, so it + // needs no escaping; the header line is a constant. var displayName = EscapeCsvField(item.DisplayName); var nodeId = EscapeCsvField(item.NodeId); var value = EscapeCsvField(item.Value); @@ -511,6 +514,10 @@ private static string EscapeCsvField(string field) return field; } + // Neutralize spreadsheet formula injection before applying RFC 4180 + // quoting, so the quoting decision sees the final field content. + field = NeutralizeFormulaInjection(field); + // If field contains comma, quote, or newline, wrap in quotes and escape internal quotes if (field.Contains(',') || field.Contains('"') || field.Contains('\n') || field.Contains('\r')) { @@ -520,6 +527,38 @@ private static string EscapeCsvField(string field) return field; } + /// + /// Neutralizes spreadsheet formula injection (CWE-1236). DisplayName, + /// NodeId, Value and Status originate from the OPC UA server, which is + /// potentially untrusted on a plant network; a field such as + /// "=cmd|'/C calc'!A0" executes as a formula when the CSV is opened in + /// Excel/LibreOffice (RFC 4180 quoting alone does not prevent this). + /// Fields starting with a formula trigger character ('=', '+', '-', '@', + /// tab, or CR) are prefixed with a single quote, which spreadsheets + /// interpret as "treat as text". + /// Exception: fields that parse as a number under InvariantCulture are + /// NOT neutralized - recorded values are routinely negative numbers + /// (e.g. "-12.5"), they are inert in spreadsheets, and prefixing them + /// would corrupt the data column for downstream tools. + /// + private static string NeutralizeFormulaInjection(string field) + { + var first = field[0]; + if (first is not ('=' or '+' or '-' or '@' or '\t' or '\r')) + { + return field; + } + + // Valid invariant-culture numbers (covers leading '+'/'-') are safe + // and must round-trip unchanged. + if (double.TryParse(field, NumberStyles.Float, CultureInfo.InvariantCulture, out _)) + { + return field; + } + + return "'" + field; + } + public void Dispose() { StopRecording();