Review security operations that log nothing

Rule ND3131 Security

Matches publicly visible methods whose name denotes an authentication or authorization operation (sign-in, sign-out, authorize, password change or reset, token revocation) when no logging call is reachable within two call levels: not in the method, not in what it calls, not in what that calls.

A call through an interface or abstract method of the application counts as logging when one of its implementations logs: an IAppLogger<T> adapter over ILogger<T> is followed.

Login failures, access-control denials and credential changes are the events an incident responder looks for first. When they are not recorded, an attack cannot be detected while it happens nor reconstructed afterwards.

The rule is silent for an application that uses no logging framework at all, since it has nothing to compare against.

This is CWE-778 Insufficient Logging (OWASP Top 10 A09 Security Logging and Monitoring Failures).

Flagged Code
public async Task<IActionResult> Login(LoginModel model) {
   var user = await _users.FindByNameAsync(model.UserName);
   if (user == null || !_hasher.Verify(user, model.Password))
      return Unauthorized();   // flagged: the failed attempt leaves no trace
   await _signInManager.SignInAsync(user, model.RememberMe);
   return RedirectToAction("Index");
}
Fixed Code
public async Task<IActionResult> Login(LoginModel model) {
   var user = await _users.FindByNameAsync(model.UserName);
   if (user == null || !_hasher.Verify(user, model.Password)) {
      _logger.LogWarning("Failed login for {User} from {Ip}", model.UserName, RemoteIp);
      return Unauthorized();
   }
   _logger.LogInformation("User {User} signed in", model.UserName);
   await _signInManager.SignInAsync(user, model.RememberMe);
   return RedirectToAction("Index");
}

How to fix

Log the outcome of the operation, successful or not, with the subject, the source address and the time. Log the reason for a denial, never the credential itself: see the rule Review secrets passed to logging methods. Route these events to a channel someone actually watches.

Suppress the issue when the operation is logged by a middleware, an interceptor or an audit filter that this rule cannot see from the call graph.

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Review security operations that log nothing</Name>
// <Id>ND3131:ReviewSecurityOperationsThatLogNothing</Id>

warnif count > 0

// Third-party members are part of the code model only when the application uses them.
let loggingMembers = ThirdParty.Methods.Where(m =>
   m.ParentType.FullName.StartsWithAny("Microsoft.Extensions.Logging", "Serilog", "NLog", "log4net", "Common.Logging") ||
   m.ParentType.FullNameEqualsAny("System.Diagnostics.Trace", "System.Diagnostics.TraceSource", "System.Diagnostics.EventLog"))
   .ToHashSetEx()

where loggingMembers.Any()

let callsLogging = new Func<IMethod, bool>(x => x.MethodsCalled.Intersect(loggingMembers).Any())

// A call through an interface or abstract method of the application, like its own IAppLogger<T>
// adapter over ILogger<T>, logs when one of the implementations logs.
let logs = new Func<IMethod, bool>(x => callsLogging(x) ||
   (x.IsAbstract && x.OverridesDerived.Any(callsLogging)))

from m in Application.Methods

      // A one-liner forwarding to the real implementation is not the place to log.
      // These cheap checks come first, the name matching below is the costly filter.
where m.NbLinesOfCode > 3 &&
      m.IsPubliclyVisible &&
     !m.IsGeneratedByCompiler &&

      // Names are matched case-sensitively so that a PascalCase word is required:
      // *ShowDialogInParameter* must not read as *...LogIn...*
      m.SimpleNameContainsAny(
        "Authenticate", "Login", "SignIn", "Logout", "SignOut", "Authorize",
        "ResetPassword", "ChangePassword", "RevokeToken")

      // Nothing logs: neither the method, nor what it calls, nor what that calls.
where !logs(m) &&
      !m.MethodsCalled.Any(mc => logs(mc) || mc.MethodsCalled.Any(mc2 => logs(mc2)))

select new {
  m,
  m.NbLinesOfCode,
  Debt = 10.ToMinutes().ToDebt(),
  Severity = Severity.Medium
}

//<Expl>
// The *{0}* performs an authentication or authorization operation over
// {1} lines, and neither it nor the methods it calls write anything to a
// log. A failed or abusive attempt here leaves no trace.
//</Expl>

// <Desc>
// Matches publicly visible methods whose name denotes an authentication or
// authorization operation (sign-in, sign-out, authorize, password change
// or reset, token revocation) when no logging call is reachable within two
// call levels: not in the method, not in what it calls, not in what that
// calls.
//
// A call through an interface or abstract method of the application
// counts as logging when one of its implementations logs: an
// *IAppLogger<T>* adapter over *ILogger<T>* is followed.
//
// Login failures, access-control denials and credential changes are the
// events an incident responder looks for first. When they are not
// recorded, an attack cannot be detected while it happens nor
// reconstructed afterwards.
//
// The rule is silent for an application that uses no logging framework at
// all, since it has nothing to compare against.
//
// This is CWE-778 *Insufficient Logging* (OWASP Top 10 A09 Security
// Logging and Monitoring Failures).
//
// **Flagged Code**
// ```csharp
// public async Task<IActionResult> Login(LoginModel model) {
//    var user = await _users.FindByNameAsync(model.UserName);
//    if (user == null || !_hasher.Verify(user, model.Password))
//       return Unauthorized();   // flagged: the failed attempt leaves no trace
//    await _signInManager.SignInAsync(user, model.RememberMe);
//    return RedirectToAction("Index");
// }
// ```
//
// **Fixed Code**
// ```csharp
// public async Task<IActionResult> Login(LoginModel model) {
//    var user = await _users.FindByNameAsync(model.UserName);
//    if (user == null || !_hasher.Verify(user, model.Password)) {
//       _logger.LogWarning("Failed login for {User} from {Ip}", model.UserName, RemoteIp);
//       return Unauthorized();
//    }
//    _logger.LogInformation("User {User} signed in", model.UserName);
//    await _signInManager.SignInAsync(user, model.RememberMe);
//    return RedirectToAction("Index");
// }
// ```
// </Desc>

// <HowToFix>
// Log the outcome of the operation, successful or not, with the subject,
// the source address and the time. Log the reason for a denial, never the
// credential itself: see the rule *Review secrets passed to logging
// methods*. Route these events to a channel someone actually watches.
//
// Suppress the issue when the operation is logged by a middleware, an
// interceptor or an audit filter that this rule cannot see from the call
// graph.
// </HowToFix>

Open in the interactive Rules Explorer · All default rules

Security: all 39 items