Skip to content

Commit bcd2f44

Browse files
jeffrymorrisjeffrymorris
committed
NCBC-4226: Fix SQL++ identifier injection via backtick escaping
Motivation ---------- EscapeIfRequired now doubles internal backticks to prevent injection in Analytics link/dataverse names. Apply escaping consistently in AnalyticsIndexManager for link and dataverse identifiers. Change-Id: I952a5e782c52459cb5f3e3212fc9786445e5f86b
1 parent 1c4f352 commit bcd2f44

5 files changed

Lines changed: 76 additions & 39 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ The Official Couchbase .NET SDK
22

33
[Chat with us on Discord](https://discord.com/invite/sQ5qbPZuTh) | [Couchbase Forums](https://forums.couchbase.com/c/net-sdk/6)
44

5-
* master is 3.4 development branch
5+
* master is 3.9 development branch
66
* release27 is 2.7.X development branch
77
* release13 is 1.3.X development branch
88

@@ -46,6 +46,6 @@ NOTE: Couchbase Server 5.0+ uses Role-Based Access Control (RBAC) for authentica
4646
Please checkout this [README](https://github.com/couchbase/couchbase-net-client/blob/master/src/Couchbase.Stellar.CodeGen/README.md) for how to generate the gRPC proxies and stubs for Protostellar.
4747

4848
## Pull Requests and Submissions ##
49-
Being an Open Source project, the Couchbase SDK depends upon feedback and submissions from the community. If you feel as if you want to submit a bug fix or a feature, please post a Pull Request. The Pull Request will go through a formal code review process and merged after being +2'd by a Couchbase Engineer. In order to accept a submission, Couchbase requires that all contributors sign the Contributor License Agreement (CLA). You can do this by creating an account in [Gerrit](http://review.couchbase.org), our official Code Review system. After you have created your account, login and check the CLA checkbox.
49+
Being an Open Source project, the Couchbase SDK depends upon feedback and submissions from the community. If you feel as if you want to submit a bug fix or a feature, please post a Pull Request. The Pull Request will go through a formal code review process and merged after being +2'd by a Couchbase Engineer. In order to accept a submission, Couchbase requires that all contributors sign the Contributor License Agreement (CLA). When you push a pull-request (PR) a prompt will be displayed which allows you to sign the CLA.
5050

51-
Once the CLA is signed, a Couchbase engineer will push the pull request to Gerrit and one or more Couchbase engineers will review the submission. If it looks good they will then +2 the changeset and merge it with master. In addition, if the submission needs more work, you will need to amend the Changeset with another Patchset. Note that is strongly encouraged to submit a Unit Test with each submission and also include a description of the submission, what changed and what the result is.
51+
Once the CLA is signed, a Couchbase engineer will review your PR. If needed, you may need to amend the PR with additional changes as requested. Note that is strongly encouraged to submit a Unit Test with each submission and also include a description of the submission, what changed and what the result is.

‎src/Couchbase/Core/Utils/StringExtensions.cs‎

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,36 +14,26 @@ public static string ToHexString(this uint opaque)
1414

1515
/// <summary>
1616
/// Adds back ticks to the beginning and end of a string if they do not already exist.
17+
/// Internal backticks are doubled to prevent SQL++ identifier injection.
1718
/// </summary>
1819
/// <param name="value">A value such as a bucket or scope name.</param>
1920
/// <returns>The original value escaped with back ticks.</returns>
2021
public static string EscapeIfRequired(this string value)
2122
{
2223
const char backtick = '`';
2324

24-
if (value.Length == 0)
25+
// Strip any existing enclosing backticks to get the raw identifier,
26+
// then double all internal backticks and wrap.
27+
if (value.Length > 0 && value[0] == backtick)
2528
{
26-
return "``";
29+
value = value.Substring(1);
2730
}
28-
29-
if (value[0] != backtick)
30-
{
31-
if (value[value.Length - 1] != backtick)
32-
{
33-
// hot path is no backticks at all
34-
return $"`{value}`";
35-
}
36-
37-
return backtick + value;
38-
}
39-
40-
if (value[value.Length - 1] != backtick)
31+
if (value.Length > 0 && value[value.Length - 1] == backtick)
4132
{
42-
return value + backtick;
33+
value = value.Substring(0, value.Length - 1);
4334
}
4435

45-
// Already has backticks
46-
return value;
36+
return backtick + value.Replace("`", "``") + backtick;
4737
}
4838

4939
/// <summary>

‎src/Couchbase/Management/Analytics/AnalyticsIndexManager.cs‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
using Couchbase.Core.Exceptions.Analytics;
1212
using Couchbase.Core.IO.HTTP;
1313
using Couchbase.Core.Logging;
14+
using Couchbase.Core.Utils;
1415
using Couchbase.Management.Analytics.Link;
1516
using Couchbase.Core.Diagnostics.Metrics;
1617
using Couchbase.Core.Diagnostics.Tracing;
@@ -163,7 +164,7 @@ public async Task CreateDatasetAsync(string datasetName, string bucketName, Crea
163164
whereStr += conditionValue.StartsWith(" ") ? conditionValue : " " + conditionValue;
164165
}
165166

166-
var statement = $"CREATE DATASET {ignoreStr}{datasetName} ON `{bucketName}`{whereStr}";
167+
var statement = $"CREATE DATASET {ignoreStr}{datasetName} ON {bucketName.EscapeIfRequired()}{whereStr}";
167168

168169
await _client
169170
.QueryAsync<dynamic>(statement, options.CreateAnalyticsOptions())
@@ -305,7 +306,7 @@ public async Task CreateIndexAsync(string datasetName, string indexName, Diction
305306
_redactor.MetaData(datasetName),
306307
_redactor.MetaData(joinedFields));
307308
var ignoreStr = options.IgnoreIfExistsValue ? "IF NOT EXISTS " : string.Empty;
308-
var statement = $"CREATE INDEX `{indexName}` {ignoreStr}ON {datasetName} {joinedFields}";
309+
var statement = $"CREATE INDEX {indexName.EscapeIfRequired()} {ignoreStr}ON {datasetName} {joinedFields}";
309310

310311
await _client
311312
.QueryAsync<dynamic>(statement, options.CreateAnalyticsOptions())
@@ -359,7 +360,7 @@ public async Task DropIndexAsync(string datasetName, string indexName, DropAnaly
359360
}
360361

361362
var ignoreStr = options.IgnoreIfNotExistsValue ? " IF EXISTS" : string.Empty;
362-
var statement = $"DROP INDEX {datasetName}.`{indexName}`{ignoreStr}";
363+
var statement = $"DROP INDEX {datasetName}.{indexName.EscapeIfRequired()}{ignoreStr}";
363364

364365
await _client
365366
.QueryAsync<dynamic>(statement, options.CreateAnalyticsOptions())
@@ -445,7 +446,7 @@ public async Task ConnectLinkAsync(ConnectAnalyticsLinkOptions? options = null)
445446
rootSpan);
446447
try
447448
{
448-
var statement = $"CONNECT LINK {options.LinkNameValue}";
449+
var statement = $"CONNECT LINK {options.LinkNameValue.EscapeIfRequired()}";
449450

450451
await _client
451452
.QueryAsync<dynamic>(statement, options.CreateAnalyticsOptions())
@@ -486,7 +487,7 @@ public async Task DisconnectLinkAsync(DisconnectAnalyticsLinkOptions? options =
486487
rootSpan);
487488
try
488489
{
489-
var statement = $"DISCONNECT LINK {options.LinkNameValue}";
490+
var statement = $"DISCONNECT LINK {options.LinkNameValue.EscapeIfRequired()}";
490491

491492
await _client
492493
.QueryAsync<dynamic>(statement, options.CreateAnalyticsOptions())
@@ -781,18 +782,13 @@ private string UncompoundName(string dataverseName)
781782
throw new InvalidArgumentException("dataverseName is a required parameter");
782783
}
783784

784-
if (!dataverseName.StartsWith("`"))
785-
{
786-
dataverseName = string.Concat("`", dataverseName, "`");
787-
}
788-
789785
if (!dataverseName.Contains('/'))
790786
{
791-
return dataverseName;
787+
return dataverseName.EscapeIfRequired();
792788
}
793789

794790
var pieces = dataverseName.Split('/');
795-
return string.Join("`.`", pieces);
791+
return string.Join(".", pieces.Select(p => p.EscapeIfRequired()));
796792
}
797793

798794
private string DataSetWithDataVerse(string datasetName, string? dataverseName)
@@ -803,8 +799,8 @@ private string DataSetWithDataVerse(string datasetName, string? dataverseName)
803799
}
804800

805801
return string.IsNullOrWhiteSpace(dataverseName) ?
806-
$"`{datasetName}`" :
807-
$"{UncompoundName(dataverseName!)}.`{datasetName}`";
802+
datasetName.EscapeIfRequired() :
803+
$"{UncompoundName(dataverseName!)}.{datasetName.EscapeIfRequired()}";
808804
}
809805
}
810806
}

‎tests/Couchbase.UnitTests/Core/Utils/StringExtensionTests.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ public class StringExtensionTests
1313
[InlineData("default`", "`default`")]
1414
[InlineData("beer-sample", "`beer-sample`")]
1515
[InlineData("`beer-sample`", "`beer-sample`")]
16+
[InlineData("foo`bar", "`foo``bar`")]
17+
[InlineData("`foo`bar`", "`foo``bar`")]
18+
[InlineData("`inject``here`", "`inject````here`")]
1619
public void EscapeIfRequired_Input_ExpectedOutput(string input, string expectedOutput)
1720
{
1821
// Act

‎tests/Couchbase.UnitTests/Management/AnalyticsIndexManagerTests.cs‎

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ public async Task CreatesCorrectConnectLinkQueryWithLinkName()
377377
{
378378
Mock<IAnalyticsClient> mockAnalyticsClient = new Mock<IAnalyticsClient>();
379379
mockAnalyticsClient.Setup(x => x.QueryAsync<object>(
380-
It.Is<string>(s => s.Equals("CONNECT LINK test_link")),
380+
It.Is<string>(s => s.Equals("CONNECT LINK `test_link`")),
381381
It.IsAny<AnalyticsOptions>()))
382382
.ReturnsAsync(new StreamingAnalyticsResult<dynamic>(stream, new DefaultSerializer()));
383383
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
@@ -393,7 +393,7 @@ public async Task CreatesCorrectConnectLinkQueryWithDefaultLinkName()
393393
{
394394
Mock<IAnalyticsClient> mockAnalyticsClient = new Mock<IAnalyticsClient>();
395395
mockAnalyticsClient.Setup(x => x.QueryAsync<object>(
396-
It.Is<string>(s => s.Equals("CONNECT LINK Local")),
396+
It.Is<string>(s => s.Equals("CONNECT LINK `Local`")),
397397
It.IsAny<AnalyticsOptions>()))
398398
.ReturnsAsync(new StreamingAnalyticsResult<dynamic>(stream, new DefaultSerializer()));
399399
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
@@ -409,7 +409,7 @@ public async Task CreatesCorrectDisconnectLinkQueryWithLinkName()
409409
{
410410
Mock<IAnalyticsClient> mockAnalyticsClient = new Mock<IAnalyticsClient>();
411411
mockAnalyticsClient.Setup(x => x.QueryAsync<object>(
412-
It.Is<string>(s => s.Equals("DISCONNECT LINK test_link")),
412+
It.Is<string>(s => s.Equals("DISCONNECT LINK `test_link`")),
413413
It.IsAny<AnalyticsOptions>()))
414414
.ReturnsAsync(new StreamingAnalyticsResult<dynamic>(stream, new DefaultSerializer()));
415415
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
@@ -425,7 +425,7 @@ public async Task CreatesCorrectDisconnectLinkQueryWithDefaultLinkName()
425425
{
426426
Mock<IAnalyticsClient> mockAnalyticsClient = new Mock<IAnalyticsClient>();
427427
mockAnalyticsClient.Setup(x => x.QueryAsync<object>(
428-
It.Is<string>(s => s.Equals("DISCONNECT LINK Local")),
428+
It.Is<string>(s => s.Equals("DISCONNECT LINK `Local`")),
429429
It.IsAny<AnalyticsOptions>()))
430430
.ReturnsAsync(new StreamingAnalyticsResult<dynamic>(stream, new DefaultSerializer()));
431431
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
@@ -667,5 +667,53 @@ public void AzureBlobAnalyticsLink_Validate(string name, string dataverse, strin
667667
Assert.Throws<ArgumentException>(link.ValidateForRequest);
668668
}
669669
}
670+
671+
[Fact]
672+
public async Task ConnectLink_EscapesEmbeddedBackticksInLinkName()
673+
{
674+
using (var stream = GenerateStreamFromString("Here is a stream."))
675+
{
676+
Mock<IAnalyticsClient> mockAnalyticsClient = new Mock<IAnalyticsClient>();
677+
mockAnalyticsClient.Setup(x => x.QueryAsync<object>(
678+
It.Is<string>(s => s.Equals("CONNECT LINK `malicious``link`")),
679+
It.IsAny<AnalyticsOptions>()))
680+
.ReturnsAsync(new StreamingAnalyticsResult<dynamic>(stream, new DefaultSerializer()));
681+
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
682+
await manager.ConnectLinkAsync(new ConnectAnalyticsLinkOptions().LinkName("malicious`link"));
683+
mockAnalyticsClient.VerifyAll();
684+
}
685+
}
686+
687+
[Fact]
688+
public async Task DisconnectLink_EscapesEmbeddedBackticksInLinkName()
689+
{
690+
using (var stream = GenerateStreamFromString("Here is a stream."))
691+
{
692+
Mock<IAnalyticsClient> mockAnalyticsClient = new Mock<IAnalyticsClient>();
693+
mockAnalyticsClient.Setup(x => x.QueryAsync<object>(
694+
It.Is<string>(s => s.Equals("DISCONNECT LINK `malicious``link`")),
695+
It.IsAny<AnalyticsOptions>()))
696+
.ReturnsAsync(new StreamingAnalyticsResult<dynamic>(stream, new DefaultSerializer()));
697+
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
698+
await manager.DisconnectLinkAsync(new DisconnectAnalyticsLinkOptions().LinkName("malicious`link"));
699+
mockAnalyticsClient.VerifyAll();
700+
}
701+
}
702+
703+
[Fact]
704+
public async Task CreateDataverse_EscapesEmbeddedBackticks()
705+
{
706+
using (var stream = GenerateStreamFromString("Here is a stream."))
707+
{
708+
var mockAnalyticsClient = new Mock<IAnalyticsClient>();
709+
mockAnalyticsClient.Setup(x => x.QueryAsync<dynamic>(
710+
It.Is<string>(s => s.Equals("CREATE DATAVERSE `malicious``dataverse`")), It.IsAny<AnalyticsOptions>()))
711+
.ReturnsAsync(new StreamingAnalyticsResult<object>(stream, new DefaultSerializer()));
712+
713+
var manager = new AnalyticsIndexManager(_mockLogger.Object, mockAnalyticsClient.Object, _mockRedactor.Object, _mockProvider.Object, _httpClientFactory);
714+
await manager.CreateDataverseAsync("malicious`dataverse");
715+
mockAnalyticsClient.VerifyAll();
716+
}
717+
}
670718
}
671719
}

0 commit comments

Comments
 (0)