Strange Error uploading artifacts in Azure DevOps pipeline

I have a library that is entirely written in .NET core that deal with some self signed X509 certificates used to encrypt and digitally sign some data. Software runs perfectly and is composed by a server and client part.

At a certain point we decided that the client should be used not only by software that runs .NET core, but also software with full framework, so I’ve changed target framework to target both netstandard 2.0 and full framework 4.6.1, everything compiles perfectly, tests run fine, everything seems to be green. The problem is that unit test project ran tests only with .NET Core, so I was not exercising tests in full framework.

Running the new project in .NET core application raise no error, but when it run in full Framework, it throws an exception while accessing the private key property of X509Certificate2 object: System.Security.Cryptography.CryptographicException: Invalid provider type specified.

Code runs perfect on .NET core runtime, but throws Invalid provider type specified on Full Framework.

Actually the error is quite straightforward, it is just telling me that the implementation of RSA private key of certificate is probably done differently in .NET Standard, an information that I already know, but indeed it was difficult to find where my code was wrong.

After long digging in the internet I found some interesting article that pointed me in the right direction a library that allows for some extension methods called HasCngKey and GetCngPrivateKey(). I spare you the gritty details, but basically the problem arise because Microsoft change Cryptographic API in Windows and I was doing a bad thing.

Original code simply cast the PrivateKey property of the certificate to the RSA class, a piece of code that I’ve found somewhere in the internet and that seems pretty straightforward. Since my library created the certificate I’m pretty sure that the private key is RSA so I can safely cast to RSA object Right? I was deadly wrong.

var rsa = (RSA)clientCertificate.PrivateKey;

The above code works only in .NET Standard because even if Full Framework has an RSA class, its implementation is different from NetStandard and make impossible to directly access PrivateKey property once the certificate was written by .NET Standard code. Welcome to Crypto API next generation whose acronym is CNG used by .NET Standard.

.NET standard code used CNG API to write the certificate, this means that, once loaded by the same class in Full Framework, PrivateKey property cannot be accessed directly from Full Framework, that uses CAPI and is not able to decrypt the key. To validate the assumption I referenced a special package from Nuget that allows me to write code that can interact with CNG API from full framework. (wrapped in a #if NETFULL directive to be used only with full framework)

if (clientCertificate.HasCngKey())
{
    var rsa2 = clientCertificate.GetCngPrivateKey();
    var cng = new RSACng(rsa2);
    aesDecryptedKey = cng.Decrypt(licenseVault.EncryptionKey, RSAEncryptionPadding.OaepSHA512);
}

This fixed the problem perfectly, actually the HasCngKey methods tells me if the private key is using CNG Api and then allows me to retrieve the key with a call to GetCngPrivateKey().

image

Figure 1: GetCngPrivateKey() method explanation

Ok, that confirms all of my suspicions, but I felt really dirty because I have to run a different code for full framework and .NET core and I was really convinced that I was doing something wrong. It turns out that the error was in my code that directly cast PrivateKey property of certificate to RSA, because the right call to obtain the RSA object from a X509Certificate2 object is a call to GetRSAPrivateKey();

RSA rsa = clientCertificate.GetRSAPrivateKey();

This method hides nifty gritty implementation details from the caller, it just retrieve the RSA key or returns null if the private key is not RSA. While it seems perfectly legit to cast the PrivateKey to RSA object in .NET Standard, this assumption is not anymore legit to Full Framework.

As always, this is a classic example on how Cryptography is a complex subject, and you should always double check your code.

Gian Maria.

Home Made Zero trust Security step 2

If you read my old post about how to create a simple program that can manage Windows Firewall to open ports with a simple udp request you surely got disappointed by the complete lack of security in the request. That program was no more than a mere proof of concept to understand if I can manage windows firewall programmatically in .NET Core.

The absolute critical problem in that program is that, UDP request to open a Tcp port is sent in clear text.

Basically the protocol is, a client C send to the server S a UDP packet in a specific port with a secret key, the server S check if the secret is correct and opens a corresponding TCP port, associated by UDP port in configuration, for requesting IP only and for a predetermined period of time.

You can easily spot the problem: the UDP packet was sent in clear text, everyone that intercept the communication will be able to open port because the secret is sent in clear text.

We have obvious solution to the problem, the most simple one is using the shared secret password to derive a symmetric cryptographic key to encrypt the message. This is far from being perfect, but it is a further step towards a more secure solution.

Since reusing the very same cryptographic key multiple time is not encouraged (even if using a different Initialization Vector solves the problem), a special class called PassdowrdDeriveBytes can be used to derive a sequence of bytes from a password, using a salt and it is cryptographically secure.

public static ICryptoTransform GetEncryptorFromPassword(
    this Aes aes,
    string password,
    byte[] salt)
{
    using (var pdb = new PasswordDeriveBytes(password, salt))
    {
        var key = pdb.GetBytes(32);
        var IV = pdb.GetBytes(16);
        return aes.CreateEncryptor(key, IV);
    }
}

The salt is a sequence of bytes to be used only once, to avoid generating the very same key each time you sent a message. You can use another approach, where you use the very same key and each time you change the Initialization vector, but using the salt to generate a unique Key and IV is probably a better method.

Given this brief introduction we can create a function to generate a random salt to be used for each message.

public static byte[] GenerateRandomSalt()
{
    using (var csp = new RNGCryptoServiceProvider())
    {
        byte[] salt = new byte[saltSize];
        csp.GetBytes(salt);
        return salt;
    }
}

Even for this simple method, it is important to use a random number generator that is cryptographically secure, such as RNGCryptoServiceProvider.  Armed with these two functions we can create a method to encrypt a generic stream of bytes.

public static Byte[] SimmetricEncrypt(string password, byte[] salt, byte[] data)
{
    using (var aes = Aes.Create())
    using (var encryptor = aes.GetEncryptorFromPassword(password, salt))
    using (MemoryStream msEncrypt = new MemoryStream())
    {
        using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
        {
            csEncrypt.Write(data, 0, data.Length);
        }

        // important, dispose CryptoStream before accessing the array
        return msEncrypt.ToArray();
    }
}

As you can see the shared password and the salt are needed and clearly the sequence of byte to encrypt. Encrypted message can be decrypted by a corresponding method that basically accepts the very same set of parameters.

public static Byte[] SimmetricDecrypt(string password, byte[] salt, byte[] data)
{
    try
    {
        using (var aes = Aes.Create())
        using (var encryptor = aes.GetDecryptorFromPassword(password, salt))
        using (MemoryStream msDecrypt = new MemoryStream())
        {
            using (MemoryStream msOriginalData = new MemoryStream(data))
            using (CryptoStream csDecrypt = new CryptoStream(msOriginalData, encryptor, CryptoStreamMode.Read))
            {
                csDecrypt.CopyTo(msDecrypt);
            }

            // important, dispose CryptoStream before accessing the array
            return msDecrypt.ToArray();
        }
    }
    catch (CryptographicException cex)
    {
        Log.Error(cex, "Error decrypting message");
        //Do not disclose anything to the caller.
        throw new SecurityException("Error in decrypting");
    }
}

Code is really simple to read, the only special care is I’ve intercepted each CryptographicException that the code can raise (such as bad password) and I rethrow a generic Security Exception with no any information. The aim is avoiding to give to the caller any possible clue on what went wrong.

Armed with these two simple functions, we can change communication protocol between client and server, using the shared key to encrypt the request message, so anyone that intercepts the message cannot understand what is contained inside.

To avoid reply attack, were an attacker simple retransmit the very same intercepted UDP packet, content of encrypted packet is a simple class that contains three properties

        /// <summary>
        /// This is the port we want to open
        /// </summary>
        public Int32 PortToOpen { get; private set; }

        /// <summary>
        /// This is the end of opening Date, remember that the
        /// server could leave the port opened for a lesser time
        /// if needed.
        /// </summary>
        public DateTime EndOpeningDate { get; private set; }

        /// <summary>
        /// Ip address to scope port opening to.
        /// </summary>
        public String IpAddress { get; private set; }

Now an attacker can reply an intercepted request, but the net result is to reapply the very same request, opening a port for a required IpAddress. Since the message is encrypted he/she cannot read what is inside the message, nor they can alter it.

This solution is more secure and starts to be almost production ready.

As usual the code is on GitHub (as today encryption is still on a feature branch) https://github.com/alkampfergit/StupidFirewallManager 

Gian Maria.

Do not trust user input part 3

In part 2 we continued our journey to prevent malicious users to receive dangerous data, limiting customer id to be a 5 letters string value. We have two aspect to improve because usually I got 2 complains when I show that code.

First one: Customer object, has a composite id, serialized value is somewhat clumsy to access from client code as you can see in Figure 1. Second: if you forget to create a CustomerId from value passed from the user, you are still victim of SQL Injection.

image

Figure 1: Returned object has a composite id property.

One possible solution is rewriting the CustomerId class to use a custom JsonConverter, capable of serializing / deserializing id object with custom code.

[JsonConverter (typeof (CustomerIdClassConverter))]
public class CustomerId {
  private String _id;
  public CustomerId () { }

  public CustomerId (String customerId) {
    Id = customerId;
  }

  public String Id {
    get { return _id; } 
    set {
      if (value.Length != 5)
        throw new ArgumentException ("Invalid Id");
      if (value.Any (c = & gt; !Char.IsLetter (c))) throw new ArgumentException ("Invalid Id");
      _id = value;
    }
  }
}

This is not a perfect solution, because, as you can see, I inserted a default constructor that allows creation of an object with null id and I moved all validation inside setter of the property. Sadly enough, default constructor is required from ASP.NET to allow people to directly bind the id to a get parameter.

image

Figure 2: Thanks to CustomerId default constructor I can bind the id directly with get parameter

Code in Figure 2 works because FromQuery simply create the object with default constructor, then it simply proceed to assign every property by convention with Get parameters. This allows you to create a nice Get request http://localhost:5010/api/v1/QueryExample/GetCustomer?id=ALFKI but it introduces a default constructor that completely violates every good practice, you can create a Customer Id with a null value.

Since I value protecting my object my final solution would be this one:

[JsonConverter (typeof (CustomerIdClassConverter))]
public class CustomerId 
{
  private String _id;

  public String Id => _id;

  public CustomerId (String customerId) 
  {
    if (customerId.Length != 5)
      throw new ArgumentException ("Invalid Id");

    if (customerId.Any (c = & gt; !Char.IsLetter (c)))
      throw new ArgumentException ("Invalid Id");

    _id = customerId;
  }
}

This is a nice Id object, it does not have default constructor, readonly id property and a constructor that allows only valid id to exists in my system. If you notice, I’ve also a nice JsonConverter attribute that specify a custom serializer for this class.

    public class CustomerIdClassConverter : JsonConverter
    {
        public override bool CanConvert(Type objectType)
        {
            if (objectType == typeof(CustomerId))
            {
                return true;
            }
            return false;
        }

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
        {
            if (reader.Value is String id)
            {
                return new CustomerId(id);
            }
            reader.Read();
            if ("id".Equals(reader.Value as String, StringComparison.OrdinalIgnoreCase))
            {
                //we have an id property, just read it 
                reader.Read(); return new CustomerId(reader.Value as String);
            }
            throw new ArgumentException("Value is not a valid id");
        }

        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
        {
            if (value is CustomerId customerId) 
            { 
                var o = JToken.FromObject(customerId.Id); 
                o.WriteTo(writer); 
                return; 
            }

            throw new ArgumentException("Value is not a valid id");
        }

This simple serializer is specific for CustomerId class, but it is not difficult to generalize for every id of type string you need. It simply contains three methods, a CanConvert that tells the caller if this converter can handle a specific type, then a WriteJson value that is used to convert this id to Json and a ReadJson that create a real CustomerId from a JsonReader. This last method is the most interesting one, it accepts a single string, but if Value is not a string it read the stream, check if we have a property called id and then create a CustomerId from that value. This allows me to accept as valid customer id both a single string token, or a property CustomerId with corresponding value.

Now I can rewrite the API with a  different attribute

image

Figure 3: FromBody attribute allows using custom serialized in a POST call.

Call is changed to be HttpPost, I know that a get seems better and more REST oriented, but if you do not want to have default constructor and use your custom serializer, using FromBody in a HttpPost is a viable solution. Now you can call the method in POST, specifying id you want to get and have a nice and plain JSON object as response.

image

Figure 4: Call in Postman using custom serializer.

As you can see the answer does not contains composite object, I simply have a CustomerId property with a value of ALFKI, the only drawback is that it is a POST request containing a json payload that represent required object.

If you really understand how the serializer is done, you can simply pass plain id as string, because a string is a valid JSON token. The request still should be POST

image

Figure 5: You can pass a simple string as valid JSON token thanks to our custom serializer.

This is a limitation of ASP.NET serializer, if you use FromQuery option to create a get request, the object must have a parameterless constructor and it is build setting properties with reflection. To use your custom serializer you need to issue a POST request with payload.

If you do not care that, to get a single customer by id, you need to do a POST call with a payload, this version has one distinct advantage, you can use CustomerId directly as parameter of your API call, preventing any injection.

This approach has a clear advantage if you start passing complex requestes composed by more than one parameter, you usually pass some DTO in POST.

    public class GetCustomer2Dto
    {
        public CustomerId CustomerId { get; set; }

        public Int32 OtherParam { get; set; }
    }

This object has two distinct property, one is a simple Int32 the other is a CustomerId. Thanks to my serializer I can call with this code.

image

Figure 6: Using CustomerId inside a complex dto class.

Maintaining the real advantage of strict validation, because if I pass something that is not a valid CustomerId, I got my request rejected.

image

Figure 7: Passing wrong id trigger internal server error because an ArgumentException is raised.

Form a security perspective, if you force your programmer to always require a specific DTO in every request composed only by custom objects with their validation, instead of simple strings, risks of injection are greatly reduced. Thanks to custom serializer you can pass simple string property from the caller, in this example customerId property is simple string from caller perspective, but in server side code an exception is thrown if something unexpected is passed because it gots translated in a real CustomerId object with specific validation.

Always double check everything comes from the user, if possible always implement a whitelist of permitted values, throwing exception if something unexpected is passed.

Code can be found here: https://github.com/alkampfergit/AzureDevopsReleaseSamples

Gian Maria.

Do not trust user input part 2

After we fixed our code in part 1 of this serie, we continue to expand our API adding a method to select  a Customer. Northwind database Customer table has an id of type string, so we could start with this very bad, bad, bad piece of code.

image

Figure 1: Another bad example of API vulnerable with Sql Injection

Again the question is: what is the most critical error in that piece of code? If you answer “Query with string concatenation” probably you are wrong. Indeed that is a huge problem, but in my mind is accepting a string from the user is still the number one problem.

Remember that accepting a string from the user basically means that you accept really every possible sequence of characters, probably not what you really want.

If in part 1 example using a parameter string for an integer id is a real stupid error, someone could be tempted to affirm that, since the id of the customer is a string, it is normal for the API to accept a string. Believe me, the answer is: NO!

If I look at northwind database, id is composed by 5 uppercase letters, every id has this specific pattern and is not random. This is the reason why I can define a specific class to represent a valid customer id that throws exception if anything different from a valid pattern is passed as argument.

public class CustomerIdClass
{
    public CustomerIdClass(String customerId)
    {
        if (customerId.Length != 5)
            throw new ArgumentException("Invalid Id", nameof(customerId));

        if (customerId.Any(c => !Char.IsLetter(c)))
            throw new ArgumentException("Invalid Id", nameof(customerId));

        Id = customerId;
    }

    public String Id { get; private set; }
}

As you can see, this class is really simple, it represents the id of a customers that can be created only from a string composed exactly by 5 letters. Customer object has CustomerId property of type CustomerIdClass and this will ensure that you cannot create a customer with invalid id. This enforces Customer Id pattern in every instance in your code, preventing you to create a customer with invalid id like “3”.

This type of technique is extremely useful also for security purposes, because you can now change your API method to construct a valid CustomerId.

image

Figure 2: GetCustomer method now construct a valid CustomerIdClass before issuing the query

Thanks to this code, you can still ask for customer with a simple get:

image

Figure 3: Simple query for customers

Any request that deviates from the standard pattern of a customer Id (5 chars) returns an error

image

Figure 4: Error returned from wrong id

Now it is mandatory that you should also change the way you are issuing your query to Sql Engine removing the injection, but we can all agree that an attacker has an hard life to try injection of any kind using only 5 letters even if your query is really bad and directly concatenates strings.

The lesson is always the same: limit what the user can pass as parameter to the exact pattern of the data you can expect, instead of allowing any string to be passed.

Gian Maria.

Do not trust user input

It is time to start blogging a little bit about security, because injection is still high in OWASP TOP 10 and this implies that people still trust their users. Remember, you should not trust your users, never, never, never, because for 10.000 good users there could be 1 bad user, and he/she is enough to damage your organization.

Here you have a really bad, bad, bad, bad, piece of code that is meant to allow product retrieval from northwind database Products table.

image

Figure 1: Standard piece of code that suffer from SQL Injection.

I hear you screaming something like: if you use Nhbernate, Entity Framework or parameterized commands the problem will go away, never ever build sql string from raw user output. Legit, but my question is: What would be your first fix for code in Figure 1? If your answer regards how you create the query to the database, the answer is only partially correct.

The real problem of function in Figure 1 is that productId is an integer in the database, but the method admits a string as parameter, this basically allows anything to be passed as productId. This kind of stupid errors are simple to made and since everything works as expected often are never corrected.

Now my question is: how dangerous is the above code? Rest assured that such code leads to a full compromise of your database and potentially could compromise your entire system. Any script kiddie can use SqlMap to test if the url is vulnerable

image

Figure 2: A simple call and sqlmap find that the url is vulnerable

Voilà, the url is vulnerable, the attacker can do almost everything to your database trough your application, exfiltrate data, deleting and modifying data and if you have the bad habit of using server admin (like sa) in your connection string, every database is compromised. The attacker can also use the –privileges options to understand the privilege of code running the injection.

image

Figure 3: Here are all privilege that sqlmap can use, it seems that someone access the database with an administrator connection string, too bad.

The –current-user option list the current user, actually I’m running the .NET core application in a console with my Windows User, and indeed sqlmap is able to get the current user of the system.

image

Figure 4: Current user detection on the system

Believe me, if database engine is very old or it is bad configured, you can even transfer files to and from the system or open a shell and compromise the entire system (https://niiconsulting.com/checkmate/2014/01/from-sql-injection-to-0wnage-using-sqlmap/). You should trust me, you really do not want to find yourself in this situation.

A single entry point vulnerable to SQL Injection could compromise the entire system

Since the real flaw is accepting a string for product id, a much more secure version is obtained simply declaring the parameter as Int32.

image

Figure 5: A real secure version of the API

Even if code in Figure 5 still contains a query created with string concatenation (that should be changed immediately after changing parameter type because it is a bad error) productId parameters is now declared as integer and the attacker cannot use SQL Injection any more. This solution is better because it not only protects you from known sql injection tricks, but it limits user input to an integer, reducing any parameter manipulation technique he/she can use. Anything that is not an integer is simply not accepted.

image

Figure 6: This is what an attacker find when he/she tries to send something that is not an integer to the system.

You can now run again sqlmap against the api and you can verify that the endpoint is not vulnerable anymore.

Remember, limiting what your users can send to your code to the minimum value set that allows the code to work greatly limits the risk of  receiving a dangerous payload.

Gian Maria.