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.

Why I love DevOps and hate DevSecOps

DevOps is becoming a buzzword, it makes hype and everyone want to be part of it, even if he/she does not know exactly what DevOps is. One of the symptoms of this is the “DevOpsEngineer”, a title that does not fit in my head.

We could debate for days or years on the right definition of DevOps, but essentially is a cultural approach on building software focused on building the right thing with the maximum quality and satisfaction for the customer. 

Remember, DevOps is a cultural approach based on transparency and inclusion, not a set of tools/practices

In my head DevOps is nothing really different from Agile, it has just a different perspective. And I know, probably most of you are crying out loud because we had lots of guru, articles, sites telling you the difference from Agile and DevOps, but I simply do not care. If you think that DevOps is about continuous deployment and only tools and practices, you are probably wrong. I can admit that tools and practices can be a backbone of DevOps culture, everything should start with culture, collaboration, inclusion and transparency, tools and practices come later in the game.

What I care, as a professional that gets paid to create software, is the satisfaction of the Customer, because it brings me more work and makes me proud of my work, after all, in this industry, we all love our works. I read “The Goal” lots of years ago, and it is still so actual, the Theory of Constraint is still actual, even in software. In my mind DevOps is just another tentative of changing the approach of making software for the good of the team, ops, Customer and users.

Given that, what is a DevOps Engineer? Giving DevOps prefixed roles in a DevOps culture is really bad, because every person of the team is part of DevOps: Customer + Developers + Operationals.

DevOps culture permeates work environment and we do not need DevOps XXX roles, everyone is part of DevOps culture and if you urge the need to find a DevOps Engineer it just means that other members in the team are out of DevOps culture. A DevOps XXX is just a patch in your culture problem and it will not just work.

Given that, since I love DevOps I hate every DevXXXOps, because there is nothing more to add to DevOps.

This is why I hate with all myself DevSecOps; since DevOps is now a buzzword as Security, why not to create a super buzzword like DevSecOps?

Let me be crystal clear, if you claim that your organization has a DevOps culture and you do not care about security, you are doing it dead wrong. Security is paramount, it should be part of every professional / practice / culture and it should permeate every part of software lifecycle. If you think that you need to add Sec to DevOps it just means that you are not caring about security in your culture, and this is a HUGE problem that should be address before bringing new Buzzword into the game.

Gian Maria.

Check for Malware in a Azure DevOps Pipeline

In a previous post I’ve showed Credential Scanner, a special task part of Microsoft Security Code Analysis available in Azure, today I want to have a quick peek at Anti Malware scanner task.

First of all a simple consideration: I’ve been asked several times if there is any need to have an AntiVirus or AntiMalware tools in build machines, after all the code that is build is developed by own developer, so there should be no need of such tools, right? In my opinion this is a false assumption, here is some quick consideration on how a malware can be downloaded in your build machine

1) If you build open source code where others can contribute and if there is no constant analysis of code, it is simple for a malicious user to modify a script or yaml build to do something nasty to your build machine. Ok, this is a really an edge case, but think at an angry employee that got fired and want to damage the company….

2) Nuget, Npm and in general every package manager download stuff from internet, this should be enough to justify keeeping an AntiVirus on your build agent. Some npm package you are using can be hijacked to download everything, and, generally speaking, everything can go south if you download stuff from the internet. I know that npm and nuget probably does some check of packages, but there is no real formal approval process, so I think that noone can guarantee that everything that comes from nuget, npm or the like is safe.

3) Custom Task in azure devops are also downloaded from the server, but in this situation the risk can be mitigated, because Microsoft checks product that are in marketplace.

In my opinion, since a build agent will download stuff from internet and executes scripts made by humans, it is better to have a good security solution constantly monitoring Agent Working folder

Ok, point 2 is the real risk, to mitigate it, the only solution is to point to a private Nuget or Npm repositories, and double check every package that you allow from nuget.org or npm main repository. The goal is: before a new version of a library is allowed to be used, someone should check if there are no risks. Npm is especially annoying, because an NPM install usually automatically updates libraries, this is why on a build you should always prefer npm ci instead of npm install.

Generally speaking, in my opinion it is better to have an antivirus on your build machine, and be 100% sure that agent folder and agent folder is constantly monitored.

To add an extra level of security, I’d also like to have a report in my build that certify that output of the build is safe, welcome Microsoft Malware Scanner Analyzer. This is another task part of the Microsoft Security Code Analysis whose purpose is scanning a specific folder and report analysis in the build.

Task configuration is quite simple, you can usually leave all default configurations, and you are ready to go.

image

Figure 1: Configuration of Malware scanner Task

The only real parameter you want to configure is the path you want to scan, usually is the artifacts directory, so you are confident that the output of the build that will be uploaded to the service is Malware free. Having another AntiVirus as I told before gives you double security, because standard antivirus kicks in automatically, and this task will do another check and upload result on the build.

image

Figure 2: Output of Malware scanner in build output

Call me paranoid, but I really like having an assurance that my artifacts are secure. I perfectly know that this is no 100% assumption that everything is good, but it is a good part to start.

Another nice aspect of the tool is that the output of the scan is also included as an artifacts.

image

Figure 3: Anti malware scanner log uploaded as artifacts.

This allows for everyone that downloads artifacts for installation to check output of the scanner.

Remember, when it is time for security, having a double check is better than have a single check.

Gian Maria