Code Review

Code Review Instructions for DSInternals

These instructions guide code reviews for the DSInternals repository, which contains C#, C++/CLI, and PowerShell code for Active Directory security auditing, offline database manipulation, and password management. Focus on higher-level concerns that require expert judgment rather than stylistic or syntactic issues handled by automated tooling.

If there are no code changes to review, perform a review of the entire codebase based on these guidelines to identify potential improvements or issues. Do not rely solely on targeted searches for specific artefacts. Systematically read all source files to ensure comprehensive coverage. For each file read, apply all the review priorities documented below. Do not skip files even if they appear simple—security issues often hide in seemingly innocuous code.

Review Priorities

1. Security

Critical Security Concerns (Especially Important for AD Security Tooling):

• Credential Handling: Ensure passwords, hashes (NT, LM, Kerberos keys), and secrets are securely handled, never logged, and properly cleared from memory when no longer needed
  • Use SecureString where appropriate for password input
  • Clear sensitive byte arrays with Array.Clear() or CryptographicOperations.ZeroMemory() in finally blocks
  • Pin sensitive data in memory during cryptographic operations to prevent GC relocation
• Cryptographic Operations: Verify proper use of cryptographic APIs for password hashing (MD4, MD5, SHA-1, PBKDF2), key derivation, and encryption/decryption operations
  • Use RandomNumberGenerator for cryptographically secure random bytes, never System.Random
  • Ensure proper IV/nonce generation (unique per encryption operation)
  • Verify constant-time comparison for secret data using CryptographicOperations.FixedTimeEquals() to prevent timing attacks
• Buffer Overflows (C++/CLI): In native C++/CLI code, check for proper bounds checking, safe memory operations, and correct use of RPC marshaling
• Input Validation & Sanitization: Ensure all external inputs (Active Directory data, ntds.dit database content, network data from MS-DRSR/MS-SAMR protocols) are properly validated
  • Validate string lengths before buffer operations
  • Check array bounds before indexing
  • Validate enum values are within expected ranges
• Path Traversal: Check for potential path traversal vulnerabilities when handling file paths (especially for ntds.dit database files)
  • Use Path.GetFullPath() and validate paths are within expected directories
• Injection Vulnerabilities: Check for potential LDAP injection, command injection, or code injection risks
• Authentication & Authorization: Ensure proper handling of Kerberos, NTLM, and other AD authentication mechanisms
• Information Disclosure: Watch for accidental logging or exposure of sensitive data (password hashes, DPAPI keys, BitLocker recovery keys, certificates)
  • Ensure ToString() overrides don't expose sensitive fields
  • Check exception messages don't leak sensitive information
• Deserialization: Ensure safe deserialization practices, especially when parsing AD replication data or database records
• Race Conditions: Identify potential TOCTOU vulnerabilities, especially when accessing ntds.dit database files or AD replication streams
• Exception Handling for Crypto: Catch and handle CryptographicException appropriately; avoid leaking information about why decryption failed

2. Performance

Performance Considerations:

• Algorithmic Complexity: Identify inefficient algorithms, especially when processing large AD databases with millions of objects
• Memory Allocations (C#): Watch for excessive allocations in hot paths, consider Span<T>, stackalloc, or object pooling where appropriate
• Memory Management (C++/CLI): Ensure proper use of native/managed memory boundaries, avoid unnecessary marshaling overhead
• Boxing: Identify unnecessary boxing of value types (SIDs, GUIDs, timestamps)
• String Operations: Check for string concatenation in loops (use StringBuilder), especially when building distinguished names or LDAP filters
• ESE/JET Database Access: Ensure efficient cursor operations, proper index usage, and minimal database round-trips when querying ntds.dit
• RPC Call Efficiency: Optimize MS-DRSR/MS-SAMR RPC calls to minimize network round-trips
• Collection Choices: Verify appropriate collection types for access patterns (especially when handling SID histories, group memberships)
• Lazy Initialization: Check for opportunities to defer expensive operations (schema loading, database connections)
• Native Interop: Ensure P/Invoke and C++/CLI interop calls are efficient with minimal marshaling overhead
• Async/Await Best Practices:
  • Use ConfigureAwait(false) in library code (DSInternals Framework) to avoid deadlocks and improve performance
  • Avoid async void except for event handlers; use async Task instead
  • Support CancellationToken for long-running operations (database enumeration, RPC calls)
  • Prefer ValueTask over Task for hot paths that often complete synchronously
  • Avoid blocking on async code (no .Result, .Wait(), or .GetAwaiter().GetResult() in async contexts)
  • Don't use Task.Run() to wrap synchronous code in library methods; let the caller decide
  • Ensure proper exception handling in async methods (exceptions are captured in the returned Task)

3. Backwards Compatibility

Compatibility Requirements:

• Public API Changes: Any change to DSInternals Framework public APIs requires careful scrutiny
  • Breaking changes are generally not acceptable
  • New optional parameters, overloads, and interface implementations need careful consideration
  • Verify that API additions follow existing patterns and naming conventions
• PowerShell Cmdlet Compatibility: Ensure cmdlet parameter sets, output types, and behavior remain consistent
  • Parameter names and aliases must be preserved
  • Output object properties must maintain backwards compatibility
  • Pipeline behavior must be preserved
• Multi-Framework Support: Changes must work on both .NET Framework 4.8 and .NET 10.0
• Serialization Compatibility: Ensure changes don't break serialization of persisted data (e.g., exported credentials)
• Behavioral Changes: Even non-breaking changes can break consumers if behavior changes unexpectedly
  • Document behavioral changes clearly in CHANGELOG.md
• Obsolete APIs: Check that proper obsolescence process is followed (ObsoleteAttribute, documentation, migration path)

4. Cross-Component Interactions

Integration Points:

• C#/C++/CLI Boundaries: Verify proper marshaling of data structures between managed C# and C++/CLI code
  • Ensure correct memory ownership semantics
  • Check for proper exception handling across managed/native boundaries
• PowerShell/Framework Boundaries: Ensure clean separation between cmdlet implementations and underlying framework code
  • Cmdlets should be thin wrappers delegating to framework classes
  • Error handling should translate framework exceptions to appropriate PowerShell errors
• RPC Protocol Interactions: Verify correct implementation of MS-DRSR (replication) and MS-SAMR (SAM) protocols
  • Check for proper handling of protocol version differences
  • Ensure correct RPC stub memory allocation/deallocation
• ESE/JET Database Access: Ensure proper database session management, cursor handling, and transaction semantics
• Threading Models: Ensure thread-safety is maintained, especially for database connections and RPC handles
• Lifecycle Management: Verify proper initialization, disposal patterns (IDisposable), and cleanup across component boundaries
• Error Handling: Ensure exceptions and error codes are properly propagated across component boundaries

5. Correctness and Edge Cases

Code Correctness:

• Null Handling: While the compiler enforces nullable reference types, verify runtime null checks for AD data that may be absent
• Boundary Conditions: Test for empty collections, null attributes, maximum SID lengths, large group memberships
• AD Data Variations: Handle variations in AD data across different domain/forest functional levels and schema versions
• Error Paths: Ensure error handling is correct and complete; database cursors and RPC handles are properly cleaned up
• Concurrency: Identify race conditions, especially when accessing shared database connections
• Exception Safety: Verify operations maintain invariants even when exceptions occur (especially in C++/CLI code)
• Resource Management: Ensure IDisposable is implemented correctly; database sessions, RPC connections, and native handles are not leaked
• Numeric Overflow: Check for potential integer overflow when handling RID pools, USNs, or large object counts
• Encoding Issues: Verify proper handling of Unicode strings, UTF-8/UTF-16 conversions, and binary attribute data
• Time Handling: Check for proper handling of AD timestamps (FILETIME, GeneralizedTime, epoch differences)