Skip to content

Commit 0e91f2b

Browse files
Copilotbcollamore
Copilot
and
bcollamore
authored
feat: PH2158: AvoidPkcsPaddingWithRsaEncryption security analyzer with CodeFixer (#908)
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com> Co-authored-by: Collamore, Brian <brian.collamore@philips.com>
1 parent b8c121e commit 0e91f2b

File tree

5 files changed

+348
-0
lines changed

5 files changed

+348
-0
lines changed

‎Documentation/Diagnostics/PH2158.md‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
# PH2158: Avoid PKCS#1 v1.5 padding with RSA encryption
2+
3+
| Property | Value |
4+
|--|--|
5+
| Package | [Philips.CodeAnalysis.SecurityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.SecurityAnalyzers) |
6+
| Diagnostic ID | PH2158 |
7+
| Category | [Security](../Security.md) |
8+
| Analyzer | [AvoidPkcsPaddingWithRsaEncryptionAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.SecurityAnalyzers/AvoidPkcsPaddingWithRsaEncryptionAnalyzer.cs)
9+
| CodeFix | Yes |
10+
| Severity | Error |
11+
| Enabled By Default | No |
12+
13+
## Introduction
14+
15+
This rule identifies usage of PKCS#1 v1.5 padding with RSA encryption, which is vulnerable to padding oracle attacks.
16+
17+
## Reason
18+
19+
PKCS#1 v1.5 padding is susceptible to padding oracle attacks that can allow attackers to decrypt encrypted data or forge signatures. OAEP (Optimal Asymmetric Encryption Padding) provides significantly better security through a more robust padding scheme that includes randomness and cryptographic hashing.
20+
21+
## How to fix violations
22+
23+
Replace `RSAEncryptionPadding.Pkcs1` with `RSAEncryptionPadding.OaepSHA256` or other OAEP variants. The CodeFix provider will automatically suggest replacing `.Pkcs1` with `.OaepSHA256`.
24+
25+
## Examples
26+
27+
### Incorrect
28+
29+
```csharp
30+
using System.Security.Cryptography;
31+
32+
class Example
33+
{
34+
void EncryptData(RSA rsa, byte[] data)
35+
{
36+
// This triggers PH2158 - PKCS#1 v1.5 padding is insecure
37+
byte[] encrypted = rsa.Encrypt(data, RSAEncryptionPadding.Pkcs1);
38+
}
39+
40+
void DecryptData(RSA rsa, byte[] encryptedData)
41+
{
42+
// This also triggers PH2158
43+
byte[] decrypted = rsa.Decrypt(encryptedData, RSAEncryptionPadding.Pkcs1);
44+
}
45+
46+
void AssignPadding()
47+
{
48+
// This also triggers PH2158
49+
RSAEncryptionPadding padding = RSAEncryptionPadding.Pkcs1;
50+
}
51+
}
52+
```
53+
54+
### Correct
55+
56+
```csharp
57+
using System.Security.Cryptography;
58+
59+
class Example
60+
{
61+
void EncryptData(RSA rsa, byte[] data)
62+
{
63+
// Use OAEP padding for better security
64+
byte[] encrypted = rsa.Encrypt(data, RSAEncryptionPadding.OaepSHA256);
65+
}
66+
67+
void DecryptData(RSA rsa, byte[] encryptedData)
68+
{
69+
// Use OAEP padding for better security
70+
byte[] decrypted = rsa.Decrypt(encryptedData, RSAEncryptionPadding.OaepSHA256);
71+
}
72+
73+
void AssignPadding()
74+
{
75+
// Use OAEP padding for better security
76+
RSAEncryptionPadding padding = RSAEncryptionPadding.OaepSHA256;
77+
}
78+
}
79+
```
80+
81+
## Configuration
82+
83+
This rule is disabled by default and must be explicitly enabled in your project configuration.
84+
85+
## Suppression
86+
87+
```csharp
88+
[SuppressMessage("Philips.CodeAnalysis.SecurityAnalyzers", "PH2158:Avoid PKCS#1 v1.5 padding with RSA encryption", Justification = "Reviewed.")]
89+
```
90+
91+
## References
92+
93+
- [OAEP Padding](https://en.wikipedia.org/wiki/Optimal_asymmetric_encryption_padding)
94+
- [Padding Oracle Attacks](https://en.wikipedia.org/wiki/Padding_oracle_attack)
95+
- [RSAEncryptionPadding Class](https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rsaencryptionpadding)
96+
- [How much safer is RSA OAEP compared to RSA with PKCS1 v1.5 padding?](https://crypto.stackexchange.com/questions/47436/how-much-safer-is-rsa-oaep-compared-to-rsa-with-pkcs1-v1-5-padding)

‎Philips.CodeAnalysis.Common/DiagnosticId.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,5 +142,6 @@ public enum DiagnosticId
142142
AvoidTodoComments = 2151,
143143
AvoidUnusedToString = 2153,
144144
AvoidUnlicensedPackages = 2155,
145+
AvoidPkcsPaddingWithRsaEncryption = 2158,
145146
}
146147
}

‎Philips.CodeAnalysis.SecurityAnalyzers/AvoidPkcsPaddingWithRsaEncryptionAnalyzer.cs‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information.
2+
3+
using System.Collections.Immutable;
4+
using Microsoft.CodeAnalysis;
5+
using Microsoft.CodeAnalysis.CSharp;
6+
using Microsoft.CodeAnalysis.CSharp.Syntax;
7+
using Microsoft.CodeAnalysis.Diagnostics;
8+
using Philips.CodeAnalysis.Common;
9+
10+
namespace Philips.CodeAnalysis.SecurityAnalyzers
11+
{
12+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
13+
public class AvoidPkcsPaddingWithRsaEncryptionAnalyzer : DiagnosticAnalyzerBase
14+
{
15+
private const string Title = @"Avoid PKCS#1 v1.5 padding with RSA encryption";
16+
public const string MessageFormat = @"RSA should use OAEP padding instead of PKCS#1 v1.5 padding for better security";
17+
private const string Description = @"PKCS#1 v1.5 padding is vulnerable to padding oracle attacks. Use OAEP padding (RSAEncryptionPadding.OaepSHA1, RSAEncryptionPadding.OaepSHA256, etc.) instead for better security.";
18+
private const string Category = Categories.Security;
19+
20+
public static readonly DiagnosticDescriptor Rule = new(
21+
DiagnosticId.AvoidPkcsPaddingWithRsaEncryption.ToId(),
22+
Title, MessageFormat, Category,
23+
DiagnosticSeverity.Error, isEnabledByDefault: false, description: Description);
24+
25+
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
26+
27+
protected override void InitializeCompilation(CompilationStartAnalysisContext context)
28+
{
29+
context.RegisterSyntaxNodeAction(AnalyzeMemberAccess, SyntaxKind.SimpleMemberAccessExpression);
30+
}
31+
32+
private void Analyze(SyntaxNodeAnalysisContext context)
33+
{
34+
if (Helper.ForTests.IsInTestClass(context))
35+
{
36+
return;
37+
}
38+
39+
var memberAccess = (MemberAccessExpressionSyntax)context.Node;
40+
41+
// Check for RSAEncryptionPadding.Pkcs1
42+
if (IsRsaEncryptionPaddingPkcs1(memberAccess))
43+
{
44+
Location location = memberAccess.GetLocation();
45+
var diagnostic = Diagnostic.Create(Rule, location);
46+
context.ReportDiagnostic(diagnostic);
47+
}
48+
}
49+
50+
private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context)
51+
{
52+
Analyze(context);
53+
}
54+
55+
private bool IsRsaEncryptionPaddingPkcs1(MemberAccessExpressionSyntax memberAccess)
56+
{
57+
// Check if it's accessing Pkcs1 member
58+
if (memberAccess.Name.Identifier.ValueText != "Pkcs1")
59+
{
60+
return false;
61+
}
62+
63+
// Fast path: check for fully qualified usage
64+
var expressionText = memberAccess.Expression.ToString();
65+
if (expressionText == "System.Security.Cryptography.RSAEncryptionPadding")
66+
{
67+
return true;
68+
}
69+
70+
// Use NamespaceResolver to check if the expression is RSAEncryptionPadding (for using statements)
71+
NamespaceResolver namespaceResolver = Helper.ForNamespaces.GetUsingAliases(memberAccess);
72+
return namespaceResolver.IsOfType(memberAccess, "System.Security.Cryptography", "RSAEncryptionPadding");
73+
}
74+
}
75+
}

‎Philips.CodeAnalysis.SecurityAnalyzers/AvoidPkcsPaddingWithRsaEncryptionCodeFixProvider.cs‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information.
2+
3+
using System.Collections.Immutable;
4+
using System.Composition;
5+
using System.Threading;
6+
using System.Threading.Tasks;
7+
using Microsoft.CodeAnalysis;
8+
using Microsoft.CodeAnalysis.CodeFixes;
9+
using Microsoft.CodeAnalysis.CSharp;
10+
using Microsoft.CodeAnalysis.CSharp.Syntax;
11+
using Microsoft.CodeAnalysis.Formatting;
12+
using Philips.CodeAnalysis.Common;
13+
14+
namespace Philips.CodeAnalysis.SecurityAnalyzers
15+
{
16+
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AvoidPkcsPaddingWithRsaEncryptionCodeFixProvider)), Shared]
17+
public class AvoidPkcsPaddingWithRsaEncryptionCodeFixProvider : SingleDiagnosticCodeFixProvider<MemberAccessExpressionSyntax>
18+
{
19+
protected override string Title => "Replace with OaepSHA256 padding";
20+
21+
protected override DiagnosticId DiagnosticId => DiagnosticId.AvoidPkcsPaddingWithRsaEncryption;
22+
23+
protected override async Task<Document> ApplyFix(Document document, MemberAccessExpressionSyntax node, ImmutableDictionary<string, string> properties, CancellationToken cancellationToken)
24+
{
25+
SyntaxNode rootNode = await document.GetSyntaxRootAsync(cancellationToken);
26+
27+
// Replace .Pkcs1 with .OaepSHA256
28+
IdentifierNameSyntax newIdentifier = SyntaxFactory.IdentifierName("OaepSHA256");
29+
MemberAccessExpressionSyntax newMemberAccess = node.WithName(newIdentifier);
30+
31+
SyntaxNode newNode = newMemberAccess.WithAdditionalAnnotations(Formatter.Annotation);
32+
rootNode = rootNode.ReplaceNode(node, newNode);
33+
34+
return document.WithSyntaxRoot(rootNode);
35+
}
36+
}
37+
}

‎Philips.CodeAnalysis.Test/Security/AvoidPkcsPaddingWithRsaEncryptionAnalyzerTest.cs‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information.
2+
3+
using System.Collections.Immutable;
4+
using System.Security.Cryptography;
5+
using System.Threading.Tasks;
6+
using Microsoft.CodeAnalysis;
7+
using Microsoft.CodeAnalysis.CodeFixes;
8+
using Microsoft.CodeAnalysis.Diagnostics;
9+
using Microsoft.VisualStudio.TestTools.UnitTesting;
10+
using Philips.CodeAnalysis.Common;
11+
using Philips.CodeAnalysis.SecurityAnalyzers;
12+
using Philips.CodeAnalysis.Test.Helpers;
13+
using Philips.CodeAnalysis.Test.Verifiers;
14+
15+
namespace Philips.CodeAnalysis.Test.Security
16+
{
17+
[TestClass]
18+
public class AvoidPkcsPaddingWithRsaEncryptionAnalyzerTest : CodeFixVerifier
19+
{
20+
protected override DiagnosticAnalyzer GetDiagnosticAnalyzer()
21+
{
22+
return new AvoidPkcsPaddingWithRsaEncryptionAnalyzer();
23+
}
24+
25+
protected override CodeFixProvider GetCodeFixProvider()
26+
{
27+
return new AvoidPkcsPaddingWithRsaEncryptionCodeFixProvider();
28+
}
29+
30+
protected override ImmutableArray<MetadataReference> GetMetadataReferences()
31+
{
32+
var cryptographyReference = typeof(RSA).Assembly.Location;
33+
MetadataReference reference = MetadataReference.CreateFromFile(cryptographyReference);
34+
35+
return base.GetMetadataReferences().Add(reference);
36+
}
37+
38+
private string GetTemplate()
39+
{
40+
return @"
41+
using System;
42+
using System.Security.Cryptography;
43+
using System.Text;
44+
45+
namespace AvoidPkcsPaddingWithRsaEncryptionTest
46+
{{
47+
public class Foo
48+
{{
49+
public void TestMethod()
50+
{{
51+
using RSA rsa = RSA.Create(2048);
52+
string originalData = ""sensitive data"";
53+
byte[] dataToEncrypt = Encoding.UTF8.GetBytes(originalData);
54+
55+
{0}
56+
}}
57+
}}
58+
}}
59+
";
60+
}
61+
62+
[DataTestMethod]
63+
[DataRow(@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, RSAEncryptionPadding.Pkcs1);")]
64+
[DataRow(@"byte[] decrypted = rsa.Decrypt(dataToEncrypt, RSAEncryptionPadding.Pkcs1);")]
65+
[DataRow(@"RSAEncryptionPadding padding = RSAEncryptionPadding.Pkcs1;")]
66+
[DataRow(@"var padding = RSAEncryptionPadding.Pkcs1;")]
67+
[DataRow(@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, System.Security.Cryptography.RSAEncryptionPadding.Pkcs1);")]
68+
[TestCategory(TestDefinitions.UnitTests)]
69+
public async Task Pkcs1PaddingShouldTriggerDiagnosticAsync(string content)
70+
{
71+
var format = GetTemplate();
72+
var testCode = string.Format(format, content);
73+
await VerifyDiagnostic(testCode, DiagnosticId.AvoidPkcsPaddingWithRsaEncryption).ConfigureAwait(false);
74+
}
75+
76+
[DataTestMethod]
77+
[DataRow(@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, RSAEncryptionPadding.OaepSHA1);")]
78+
[DataRow(@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, RSAEncryptionPadding.OaepSHA256);")]
79+
[DataRow(@"byte[] decrypted = rsa.Decrypt(dataToEncrypt, RSAEncryptionPadding.OaepSHA256);")]
80+
[DataRow(@"RSAEncryptionPadding padding = RSAEncryptionPadding.OaepSHA1;")]
81+
[DataRow(@"var padding = RSAEncryptionPadding.OaepSHA256;")]
82+
[DataRow(@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, System.Security.Cryptography.RSAEncryptionPadding.OaepSHA256);")]
83+
[DataRow(@"// This is just a comment about Pkcs1")]
84+
[DataRow(@"string text = ""This mentions Pkcs1 but is not crypto"";")]
85+
[TestCategory(TestDefinitions.UnitTests)]
86+
public async Task SecurePaddingShouldNotTriggerDiagnosticAsync(string content)
87+
{
88+
var format = GetTemplate();
89+
var testCode = string.Format(format, content);
90+
await VerifySuccessfulCompilation(testCode).ConfigureAwait(false);
91+
}
92+
93+
[TestMethod]
94+
[TestCategory(TestDefinitions.UnitTests)]
95+
public async Task DoesNotTriggerDiagnosticInTestCodeAsync()
96+
{
97+
const string template = @"
98+
using System;
99+
using System.Security.Cryptography;
100+
using Microsoft.VisualStudio.TestTools.UnitTesting;
101+
102+
[TestClass]
103+
public class Foo
104+
{
105+
[TestMethod]
106+
public void Test()
107+
{
108+
using RSA rsa = RSA.Create(2048);
109+
byte[] data = new byte[10];
110+
byte[] encrypted = rsa.Encrypt(data, RSAEncryptionPadding.Pkcs1);
111+
}
112+
}
113+
";
114+
await VerifySuccessfulCompilation(template).ConfigureAwait(false);
115+
}
116+
117+
[DataTestMethod]
118+
[DataRow(
119+
@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, RSAEncryptionPadding.Pkcs1);",
120+
@"byte[] encrypted = rsa.Encrypt(dataToEncrypt, RSAEncryptionPadding.OaepSHA256);")]
121+
[DataRow(
122+
@"byte[] decrypted = rsa.Decrypt(dataToEncrypt, RSAEncryptionPadding.Pkcs1);",
123+
@"byte[] decrypted = rsa.Decrypt(dataToEncrypt, RSAEncryptionPadding.OaepSHA256);")]
124+
[DataRow(
125+
@"RSAEncryptionPadding padding = RSAEncryptionPadding.Pkcs1;",
126+
@"RSAEncryptionPadding padding = RSAEncryptionPadding.OaepSHA256;")]
127+
[DataRow(
128+
@"var padding = RSAEncryptionPadding.Pkcs1;",
129+
@"var padding = RSAEncryptionPadding.OaepSHA256;")]
130+
[TestCategory(TestDefinitions.UnitTests)]
131+
public async Task CodeFixShouldReplaceWithOaepSHA256Async(string originalCode, string expectedCode)
132+
{
133+
var originalSource = string.Format(GetTemplate(), originalCode);
134+
var expectedSource = string.Format(GetTemplate(), expectedCode);
135+
136+
await VerifyFix(originalSource, expectedSource).ConfigureAwait(false);
137+
}
138+
}
139+
}

0 commit comments

Comments
 (0)