Skip to content

Commit 7e03409

Browse files
danfiedler-msftdanfiedler-msft
authored
Add content for rule guidance containing "TO DO"s. (#617)
* Add guidance for weak random rule * Add guidance for outdated TLS protocol * Add guidance for XXE rule * Add guidance for weak cipher mode rule * Point disabled cert validation rules at complete guidance * Add guidance for DPAPI entropy rule * Use existing HTTPS guidance for Ruby rule * Add guidance for strncat rule * Add guidance for strncpy rule * Add guidance for 3DES rule * Add guidance for C gets rule * Add guidance for C strcat rule * Add guidance for C strcpy rule * Add guidance for C malloc rule * Add guidance for banned C function rule * Add guidance for InitializeSecurityContext rule * Add guidance for PowerShell restricted function rule * Add guidance for NOT implementing MD5/SHA1 rule * Add guidance for objective-c format string rule * Add guidance for memcpy rule * Point C++ TLS version rule to existing guidance * Point .NET outdated SSL rule to general guidance * Add guidance for seeding RNG with time rule * Add guidance for mcrypt rules * Add guidance for debug rule * Add guidance for iOS uniqueIdentifier rule * Add guidance for obj-c xss rule * Add guidance for eval XSS rule * Add guidance for hardcoded secret rule * Add guidance for C FILE copy rule * Add guidance for PHP file include rule * Add guidance for ASPNET Controller rule * Add guidance for iOS NSUserDefaults rule * Add guidance for hashing time rule * Remove optional encryption rule (applies to unknown tech?) * Add test condition that guidance must have content * Update changelog for guidance changes
1 parent 3e6a87e commit 7e03409

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

60 files changed

+812
-407
lines changed

‎Changelog.md

+4
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
44
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
55
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
66

7+
## [1.0.38] - 2024-6-05
8+
### Incomplete Guidance
9+
Expanded content for rule guidance containing "TO DO"s.
10+
711
## [1.0.37] - 2024-5-23
812
### Missing Guidance
913
Added guidance for several rules such as weak hash algorithm, disabling certificate validation, and TLS client configuration.

‎DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs

+6-3
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,6 @@ public void Rule_guidance_file_should_be_specified_and_exist(DevSkimRule rule)
148148
Assert.IsTrue(File.Exists(guidanceFile), $"Guidance file {guidanceFile} does not exist.");
149149
}
150150

151-
[Ignore] // TODO: temporary to get missing guidance in.
152151
[TestMethod]
153152
[DynamicData(nameof(DefaultRules))]
154153
public void Rule_guidance_should_be_complete(DevSkimRule rule)
@@ -170,8 +169,12 @@ public void Rule_guidance_should_be_complete(DevSkimRule rule)
170169
}
171170

172171
string guidance = File.ReadAllText(guidanceFile);
173-
if(guidance.Contains("TODO", StringComparison.OrdinalIgnoreCase)
174-
|| guidance.Contains("TO DO", StringComparison.OrdinalIgnoreCase))
172+
bool hasContent = !string.IsNullOrEmpty(guidance);
173+
Assert.IsTrue(hasContent, $"Guidance file {guidanceFile} is empty.");
174+
175+
if (rule.Id != "DS176209" && // "Suspicious comment" - a TODO comment
176+
(guidance.Contains("TODO", StringComparison.OrdinalIgnoreCase)
177+
|| guidance.Contains("TO DO", StringComparison.OrdinalIgnoreCase)))
175178
{
176179
Assert.Fail($"Guidance file {guidanceFile} contains TODO.");
177180
}

‎guidance/DS101155.md

-11
This file was deleted.

‎guidance/DS101159.md

+19-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,23 @@
1-
## Initializing Security Context
1+
# Initializing Security Context
22

3-
### Summary
4-
SecurityContext initialization, look here for cryptography functions.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
The `InitializeSecurityContext` function was detected; its usage and surrounding code should be reviewed for security issues.
86

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
7+
## Details
118

9+
The `InitializeSecurityContext` function is used to build a security context between the client application and a remote peer.
10+
A security context contains security data relevant to the connection (such as user identifiers or session key.)
11+
12+
Proper initialization, usage, and cleanup of security contexts can be a complex process and mistakes can lead to security issues.
13+
See the [InitializeSecurityContext documentation](https://learn.microsoft.com/en-us/windows/win32/secauthn/initializesecuritycontext--general#remarks)
14+
for more guidance. In particular, use of cryptography in this area should be carefully reviewed for potential problems.
15+
16+
## Severity Considerations
17+
18+
Code including usage of the `InitializeSecurityContext` function should be very carefully reviewed
19+
by a security expert or cryptographer to identify security issues and understand the risks involved.
20+
21+
## References
22+
23+
* [Microsoft Learn: InitializeSecurityContext function](https://learn.microsoft.com/en-us/windows/win32/secauthn/initializesecuritycontext--general)

‎guidance/DS104456.md

+31-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,35 @@
1-
## Use of restricted functions.
1+
# Use of Restricted Functions
22

3-
### Summary
4-
Use of restricted functions.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* A restricted PowerShell function was detected.
6+
* Remove or replace usage of the restricted function if possible.
87

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
8+
## Details
119

10+
Use of several different PowerShell functions open up applications to classes of vulnerabilities that would otherwise not normally occur.
11+
We refer to these functions as restricted functions.
12+
For example, `System.Runtime.InteropServices.Marshal` functions could be used to allocate memory in a way that an attacker could use to corrupt memory and potentially execute code.
13+
14+
## Solution
15+
16+
Replace usage of the restricted function with a conventional function if possible (for example, replace `Invoke-Command` or `Invoke-Expression` with `Start-Process`).
17+
18+
## Severity Considerations
19+
20+
Usage of restricted PowerShell functions should be very carefully reviewed
21+
by a security expert to identify security issues and understand the risks involved.
22+
23+
## Restricted Function List
24+
25+
* `GetDelegateForFunctionPointer`
26+
* `System.Runtime.InteropServices.Marshal`
27+
* `WriteByte`
28+
* `Microsoft.Win32.UnsafeNativeMethods`
29+
* `PtrToStructure`
30+
* `StructureToPtr`
31+
* `NtCreateThreadEx`
32+
* `CreateRemoteThread`
33+
* `Invoke-Command`
34+
* `Invoke-Expression`
35+
* `VirtualProtect`

‎guidance/DS106864.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ The DES cipher was found, which is widely considered to be broken.
66

77
## Details
88

9-
The [Data Encryption Standard](https://en.wikipedia.org/wiki/Data_Encryption_Standard), or DES,
9+
The [Data Encryption Standard](https://en.wikipedia.org/wiki/Data_Encryption_Standard), or DES,
1010
is a symmetric block cipher created in the 1970s. It has since been broken and should not be used
1111
anywhere.
1212

13-
In general, the [Advanced Encryption Standard](https://en.wikipedia.org/wiki/Advanced_Encryption_Standard),
13+
In general, the [Advanced Encryption Standard](https://en.wikipedia.org/wiki/Advanced_Encryption_Standard),
1414
or AES, algorithm, is preferred for all cases where symmetric encryption is needed.
1515

1616
### Solution

‎guidance/DS108330.md

+23-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,27 @@
1-
## Banned C function detected (strncat)
1+
# Banned C function detected (strncat)
22

3-
### Summary
4-
strncat adds the null terminator at character 'n + 1', rather than at the nth character. this frequently leads to the null terminator being added in the memory adjacent to the destination buffer, rather than in the destination buffer.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* Use of the `strncat` function to concatenate strings can lead to a buffer overrun vulnerability.
6+
* Resolve this issue by using secure versions such as `strncat_s` to help prevent buffer overruns.
87

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
8+
## Details
119

10+
The `strncat` function does not check for sufficient space in the destination string which is a potential cause of buffer overruns.
11+
The `strncat` function appends characters to a string followed by the null terminator at character 'n + 1', rather than at the nth character.
12+
This behavior frequently leads to the null terminator being added in the memory adjacent to the destination buffer, rather than in the destination buffer.
13+
14+
## Solution
15+
16+
Use secure versions such as `strncat_s` to help prevent buffer overruns.
17+
18+
## Severity Considerations
19+
20+
In the worst case, a buffer overrun vulnerability can provide an attacker the ability to execute arbitrary code leading to complete system compromise.
21+
22+
## References
23+
24+
* [CodeQL: Potentially unsafe call to strncat](https://codeql.github.com/codeql-query-help/cpp/cpp-unsafe-strncat/)
25+
* [Microsoft Learn: strncat_s](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strncat-s-strncat-s-l-wcsncat-s-wcsncat-s-l-mbsncat-s-mbsncat-s-l?view=msvc-170)
26+
* [Avoiding Buffer Overruns](https://learn.microsoft.com/en-us/windows/win32/SecBP/avoiding-buffer-overruns)
27+
* [OWASP: Buffer Overflow](https://owasp.org/www-community/vulnerabilities/Buffer_Overflow)

‎guidance/DS109501.md

+21-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,25 @@
1-
## Do not use the 3DES symmetric block cipher.
1+
# Do not use the 3DES symmetric block cipher
22

3-
### Summary
4-
The 3DES cipher was found, which is only secure if three independent keys are used.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* The 3DES cipher was found which does not provide sufficient security.
6+
* Replace usage of 3DES with AES if possible.
87

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
8+
## Details
119

10+
The [Data Encryption Standard](https://en.wikipedia.org/wiki/Triple_DES), or 3DES, is a symmetric block cipher created in the 1970s.
11+
TripleDES is broken due to its inadequate key size and [CVE-2016-2183](https://nvd.nist.gov/vuln/detail/CVE-2016-2183) and should not be used anywhere.
12+
13+
## Severity Considerations
14+
15+
Any use of the 3DES algorithm should be very carefully reviewed by a security expert or cryptographer to understand the risks involved.
16+
17+
## Solution
18+
19+
### .NET
20+
21+
Use the following method: `System.Security.Cryptography.Aes.Create()`
22+
23+
## References
24+
25+
* [CodeQL: Use of a broken or risky cryptographic algorithm](https://codeql.github.com/codeql-query-help/cpp/cpp-weak-cryptographic-algorithm/)

‎guidance/DS109733.md

+26-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,30 @@
1-
## Source implementation of a weak/broken cryptography hash function
1+
# Source Implementation of a Weak/Broken Cryptographic Hash Function
22

3-
### Summary
4-
An implementation of a weak/broken hash function was found in source code.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* An implementation of a weak/broken hash function such as MD-5 or SHA-1 was found in source code.
6+
* Remove the implementation of the weak/broken hash function.
7+
* Replace the use of insecure hashing algorithms with more secure alternatives such as an algorithm from the SHA-2 family (SHA256, SHA384, and SHA512).
88

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
9+
## Details
1110

11+
### Custom Cryptographic Implementation
12+
13+
Correct and secure implementation of cryptographic algorithms is very complex and difficult.
14+
Developers should always use well-vetted libraries for cryptographic operations rather than producing their own implementations of those functions.
15+
16+
### Weak Hash Algorithms
17+
18+
Hash collisions are computationally feasible for older, weak hash algorithms such as MD2, MD4, MD5, and SHA-1.
19+
A hash collision allows an attacker to substitute an alternative input that results in the same hash value.
20+
Collision attacks allow attackers to undermine the security of systems using an insecure hash algorithm (e.g., by forging digital signatures, concealing data tampering, or cracking passwords).
21+
22+
## Severity Considerations
23+
24+
Developers should almost never implement their own versions of cryptographic operations. Furthermore, weak hash algorithms should not be used, especially for security purposes.
25+
26+
## Solution
27+
28+
### .NET
29+
30+
Replace usages of insecure hash algorithms with `System.Security.Cryptography.SHA512Cng`, `System.Security.Cryptography.SHA384Cng`, or `System.Security.Cryptography.SHA256Cng`.

‎guidance/DS111237.md

+22-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,26 @@
1-
## Banned C function detected (strncpy)
1+
# Banned C function detected (strncpy)
22

3-
### Summary
4-
strncpy is dangerous, as if the source contains 'n' or more characters, it will not null terminate the destination.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* Use of the `strncpy` function to copy strings can lead to a buffer overrun vulnerability.
6+
* Resolve this issue by using secure versions such as `strncpy_s` to help prevent buffer overruns.
87

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
8+
## Details
119

10+
The `strncpy` function does NOT check for sufficient space in the destination buffer which may lead to a buffer overrun vulnerability.
11+
The `strncpy` function is dangerous, as if the source contains 'n' or more characters, it will not null terminate the destination.
12+
13+
## Solution
14+
15+
Use secure versions such as `strncpy_s` to help prevent buffer overruns.
16+
17+
## Severity Considerations
18+
19+
In the worst case, a buffer overrun vulnerability can provide an attacker the ability to execute arbitrary code leading to complete system compromise.
20+
21+
## References
22+
23+
* [CodeQL: Potentially unsafe call to strncpy](https://codeql.github.com/codeql-query-help/cpp/cpp-bad-strncpy-size/)
24+
* [Microsoft Learn: strncpy_s](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l?view=msvc-170)
25+
* [Avoiding Buffer Overruns](https://learn.microsoft.com/en-us/windows/win32/SecBP/avoiding-buffer-overruns)
26+
* [OWASP: Buffer Overflow](https://owasp.org/www-community/vulnerabilities/Buffer_Overflow)

‎guidance/DS112266.md

+20-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,24 @@
1-
## ProtectedData used without additional entropy
1+
# ProtectedData used without additional entropy
22

3-
### Summary
4-
The ProtectedData class should be used with additional entropy to reduce the risk of other application calling DPAPI to access the data.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
The DPAPI `ProtectedData.Protect` method should be used with additional entropy to reduce the risk of other applications accessing the data.
86

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
7+
## Details
118

9+
The `ProtectedData.Protect` method's `optionalEntropy` parameter is an "optional additional byte array used to increase the complexity of the encryption."
10+
Depending on the value of the `scope` parameter, user data encrypted without entropy can be accessed:
11+
12+
* **DataProtectionScope.CurrentUser**: by other threads or applications running under the current user.
13+
* **DataProtectionScope.LocalMachine**: by any process running on the computer.
14+
15+
The `optionalEntropy` parameter should be set to a non-null value unique to your application.
16+
17+
## Severity Considerations
18+
19+
The severity of this finding should be increased if the underlying security
20+
context is highly sensitive or if the environment hosting the application has low trust.
21+
22+
## References
23+
24+
* [Microsoft Learn: How to: Use Data Protection](https://learn.microsoft.com/en-us/dotnet/standard/security/how-to-use-data-protection)

‎guidance/DS112835.md

+5
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,15 @@ Software with hardcoded cryptographic algorithms may require code changes in the
2222
* When `Switch.System.Net.DontEnableSchUseStrongCrypto` is `false`, the application will use more secure network protocols. Do not set `DontEnableSchUseStrongCrypto` to `true`.
2323
* When application code sets a value for `System.Net.ServicePointManager.SecurityProtocol`, the application will override the TLS protocol chosen by the operating system. This makes the application less crypto-agile and may make the application less secure. Do not set a value for `System.Net.ServicePointManager.SecurityProtocol` in application code.
2424

25+
#### C++ (Schannel)
26+
27+
* Ensure that you do not set `grbitEnabledProtocols` to anything but `0`. Setting this field to zero instructs the operating system to use its configured TLS version.
28+
2529
## References
2630

2731
* [TLS Best Practices with .NET Framework](https://learn.microsoft.com/en-us/dotnet/framework/network-programming/tls)
2832
* [Security Briefs - Cryptographic Agility](https://learn.microsoft.com/en-us/archive/msdn-magazine/2009/august/cryptographic-agility)
2933
* [Microsoft SDL Cryptographic Recommendations](http://download.microsoft.com/download/6/3/A/63AFA3DF-BB84-4B38-8704-B27605B99DA7/Microsoft%20SDL%20Cryptographic%20Recommendations.pdf)
3034
* [SSL Labs: SSL and TLS Deployment Best Practices](https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices)
3135
* [OWASP: Transport Layer Protection Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Security_Cheat_Sheet.html)
36+
* [Microsoft Learn: SCHANNEL_CRED structure](https://learn.microsoft.com/en-us/windows/win32/api/schannel/ns-schannel-schannel_cred)

‎guidance/DS113286.md

+22-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,26 @@
1-
## Do not include user-input directly in format strings
1+
# Do Not Include User Input Directly in Format Strings
22

3-
### Summary
4-
Do not create NSString objects using a user-provided format string, as this could lead to a security vulnerability. https://www.securecoding.cert.org/confluence/display/c/FIO30-C.+Exclude+user+input+from+format+strings
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* Do not create NSString objects using a user-provided format string, as this could lead to a vulnerability.
6+
* Either remove creation of the NSString object entirely or remove the user-provided format string from the NSString object.
87

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
8+
## Details
119

10+
If an attacker is able to control a format string then the attacker may be able to execute arbitrary code,
11+
read data in memory, or crash the application. This is known as an uncontrolled format string vulnerability.
12+
Format string vulnerabilities commonly occur when a developer intends to output user-controlled data (to a screen, buffer, or file)
13+
but mistakenly outputs a buffer directly or treats user data as a format string.
14+
15+
## Solution
16+
17+
Either remove creation of the NSString object entirely or remove the user-provided format string from the NSString object.
18+
19+
## Severity Considerations
20+
21+
In the worst case, a format string vulnerability can provide an attacker the ability to execute arbitrary code leading to complete system compromise.
22+
23+
## References
24+
25+
* [SEI CERT C Coding Standard: FIO30-C. Exclude user input from format strings](https://www.securecoding.cert.org/confluence/display/c/FIO30-C.+Exclude+user+input+from+format+strings)
26+
* [CWE-134: Use of Externally-Controlled Format String](https://cwe.mitre.org/data/definitions/134.html)

‎guidance/DS121708.md

+20-7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,24 @@
1-
## Problematic C function detected (memcpy)
1+
# Problematic C Function Detected (memcpy)
22

3-
### Summary
4-
There are a number of conditions in which memcpy can introduce a vulnerability (mismatched buffer sizes, null pointers, etc.). More secure alternatives perform additional validation of the source and destination buffer.
3+
## Summary
54

6-
### Details
7-
TO DO - put more details of problem and solution here
5+
* Use of the `memcpy` function can lead to a buffer overrun vulnerability.
6+
* Resolve this issue by using secure versions such as `memcpy_s` to help prevent buffer overruns.
87

9-
### Severity Considerations
10-
TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem?
8+
## Details
119

10+
There are a number of conditions in which `memcpy` can introduce a vulnerability (mismatched buffer sizes, null pointers, etc.). More secure alternatives perform additional validation of the source and destination buffer.
11+
12+
## Solution
13+
14+
Usages of `memcpy` should be replaced with more secure versions such as `memcpy_s` to help prevent buffer overruns.
15+
16+
## Severity Considerations
17+
18+
In the worst case, a buffer overrun vulnerability can provide an attacker the ability to execute arbitrary code leading to complete system compromise.
19+
20+
## References
21+
22+
* [Microsoft Learn: memcpy_s](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/memcpy-s-wmemcpy-s?view=msvc-170)
23+
* [Avoiding Buffer Overruns](https://learn.microsoft.com/en-us/windows/win32/SecBP/avoiding-buffer-overruns)
24+
* [OWASP: Buffer Overflow](https://owasp.org/www-community/vulnerabilities/Buffer_Overflow)

0 commit comments

Comments
 (0)