Create rule S6781: JWT secret keys should not be disclosed

[sebastien-andrivet-sonarsource]

Why

As part of MMF-3716, we want to close the gap between C# and other languages regarding cryptography related rules support. S6781 is one of the rules that is not currently supported by this analyzer.

What

S6781 aims to detect when JWT secrets keys are disclosed. We want to add support for this behavior for both .NET core and .NET framework.

Detection logic

We want to raise when the System.IdentityModel.Tokens.SymmetricSecurityKey constructor is called with a key that is hard-coded or stored insecurely.

This covers the following methods of storing a key:

• As a byte[] in the source code with a hard-coded value.
• As a string in the source code.
• For .NET Framework applications: as a string in the <appSettings> section of App.config/web.config and obtained via the ConfigurationManager.AppSettings collection.
• For .NET Core applications: as a string in the appsettings.json file and obtained via the IConfiguration interface.

If the value is stored as a string, the following methods can be used to convert it to a byte[]:

• System.Text.Encoding.GetBytes()
• System.Convert.FromBase64String()

Example code

.NET Core examples

Compliant: key is obtained from an environment variable

[ApiController]
[Route("login-env")]
public class LoginEnvController : ControllerBase
{
    private readonly IConfiguration _config;
    public LoginEnvController(IConfiguration config) 
    {
        _config = config;
    }

    [HttpPost]
    public IActionResult Post([FromBody] LoginModel login)
    {
        // Code to validate the user's credentials is omitted

        var key = Environment.GetEnvironmentVariable("JWT_KEY") ?? "";
        var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key));  // Compliant (even if can be used dangerously)
        var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256);

        var Sectoken = new JwtSecurityToken(
            "sonarsource.com",
            "sonarsource.com",
            null,
            expires: DateTime.Now.AddMinutes(120),
            signingCredentials: credentials);

        var token =  new JwtSecurityTokenHandler().WriteToken(Sectoken);

        return Ok(token);
    }
}

Noncompliant: key is hard-coded in code

[ApiController]
[Route("login-const")]
public class LoginConstController : ControllerBase
{
    private const String Issuer = "sonarsource.com";
    private const String Key = "SecretSecretSecretSecretSecretSecretSecretSecret";

    [HttpPost]
    public IActionResult Post([FromBody] LoginModel login)
    {
        // Code to validate the user's credentials is omitted

        var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(Key));  // Noncompliant
        var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256);

        var Sectoken = new JwtSecurityToken(
            "sonarsource.com",
            "sonarsource.com",
            null,
            expires: DateTime.Now.AddMinutes(120),
            signingCredentials: credentials);

        return Ok(new JwtSecurityTokenHandler().WriteToken(Sectoken));
    }
}

Noncompliant: key is stored insecurely in appsettings.json

[ApiController]
[Route("login-config")]
public class LoginConfigController : ControllerBase
{
    private readonly IConfiguration _config;
    public LoginConfigController(IConfiguration config) 
    {
        _config = config;
    }

    [HttpPost]
    public IActionResult Post([FromBody] LoginModel login)
    {
        // Code to validate the user's credentials is omitted

        var key = _config["Jwt:Key"] ?? ""; 
        var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant
        var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256);

        var Sectoken = new JwtSecurityToken(
            "sonarsource.com",
            "sonarsource.com",
            null,
            expires: DateTime.Now.AddMinutes(120),
            signingCredentials: credentials);

        return Ok(new JwtSecurityTokenHandler().WriteToken(Sectoken));
    }
}

.NET Framework examples

Noncompliant: key is stored insecurely in App.config/web.config

public class ConstLoginController : ApiController
{
    public IHttpActionResult Post([FromBody] LoginModel login)
    {
        // Code to validate the user's credentials is omitted

        var key = ConfigurationManager.AppSettings["jwt-key"] ?? "";
        var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant
        var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256);

        var secToken = new JwtSecurityToken(
            "sonarsource.com",
            "sonarsource.com",
            null,
            expires: DateTime.Now.AddMinutes(128),
            signingCredentials: credentials
        );

        var token = new JwtSecurityTokenHandler().WriteToken(secToken);

        return Ok(token);
    }
}

RSPEC

This rule's RSPEC contains information regarding messages and highlighting.

Message

JWT secret keys should not be disclosed.

Highlight

The call to create a new instance of SymmetricSecurityKey.

[costin-zaharia-sonarsource]

Hi @sebastien-andrivet-sonarsource, I have a question about reading configuration files (app.config, web.config, appsettings.json).

SectionInformation.ProtectSection API allows developers encrypt sections of the configuration files. Couldn't our checks for IConfiguration lead to FPs when the configuration is encrypted?

[sebastien-andrivet-sonarsource]

Yes, this is right. I don't think our analyzer can detect such a situation (using IConfiguration.GetX with an encrypted configuration). I assumed that using ProtectSection and encryption is rare because it is not so easy to use safely in practice (I also looked at SourceGraph to get an idea of the usage). Most of the codes you can find using ProtectSection are not so secure. They often look like this:

ConnectionStringsSection section = config.GetSection("connectionStrings") as ConnectionStringsSection;
if (section != null)
{
    if (!section.SectionInformation.IsProtected)
    {
        section.SectionInformation.ProtectSection("DataProtectionConfigurationProvider");
        section.SectionInformation.ForceSave = true;
        config.Save(ConfigurationSaveMode.Full);
    }
}

This means that the configuration was not encrypted at one point (before this code ran the first time). If there is a bug (such as a typo in names), the configuration will never be encrypted.

I concluded that the benefit of detection (TP) would be higher than that of the rare FP. Of course, this can be challenged.

To be complete, we have the opposite situation with GetEnvironmentVariable. I assume it is safe most of the time, even if environment variables are not perfect and can be wrongly used (it depends on how and where they are defined).