Microsoft June 2017 Security Rollup Woes

Let me set the scene for you.

It's the morning of June 14th, 6:30am to be exact, and we're going to make a standards deployment. You know how I know? Because it's Wednesday. And Wednesday morning is the morning that we usually make standards deployments. Monday is just too early in the week. Tuesday, well, we just don't like Tuesdays, but Wednesday morning we make sweet, standards deployments.

On the docket that morning were some changes that impacted how our dialog boxes behaved as well as printing throughout the entire system.

All of the scheduled changes had already gone through internal and automated testing. The necessary test records were applied to the ticket, we were ready to rock and roll.

My team and I pushed the deployment button, and not 15 minutes later, we received a call from one of our customer care representatives that printing was failing for a single customer. Then another, and another. To play it safe, we decided to simply roll back the change and ask questions later. Unfortunately.. it did not fix the problem.

The Problem

We continue to investigate potential causes and eventually made the realization that the issue only occurred on Windows machines that had consumed the latest Windows updates. Specifically the June 2017 security roll up.

After spinning up a VM with Windows 7 and the latest updates, we're in business. We notice that printing from an iframe results in a blank page. Fiddler and the developer tools reveal it's actually an HTTP status code of 404.

See, when printing an iframe (or from within an iframe) IE apparently takes the contents of the iframe and downloads it locally. This downloaded iframe is then used as the source for the iframe and then it is printed from that copy. However, it appears that this security change was now referencing the iframe on the web server.

For example

<iframe src="myframe.html" />  

Became

<iframe src="ABC123.html" />  

Where ABC123.html is just some auto-generated name for the locally downloaded iframe. The problem with this is that obviously ABC123.html does not exist on our web server. So the iframe was loading a page that did not exist, and thus, printing a blank page.

The Solution

Luckily the community banded together. It gave us confidence that this was not a localized issue and that other 3rd parties were experiencing the same pains. Progress!

Everyone in the community was pretty sold on the same workaround. Instead of using window.print() use document.execCommand('print'). So we decided to implement this change in the core areas of the system that print from within an iframe (which about covers 99% of the system).

We deploy the change to our quality environment where its tested by our internal teams and customers a like. Everything passes testing, everyone seems pretty happy with the results, and we push the change to our Production environment the following week.

Everything is going smoothly, we're back up and running! Except for one area of the system... our check printing module.

Now, checks unfortunately fell within that 1%. The part of the system that wasn't covered by our change. Which we immediately thought wasn't a big deal, we can just adjust how checks are printed using the same method. How wrong we were..

Checks are a special flower within our system and have their own design. Completely separate from how other areas of the system (reports, labels, etc) print. I won't get into all of the technical details of how they were implemented, but it essentially meant that the only way to resolve the issue was to fix the underlying HTML.

After a couple days of tweaking HTML and CSS, pixel by pixel. Comparing old checks to the new implementation, everything looked good for launch. The finish line was right in front of us.

Conclusion

But of course, the morning of the final deployment.. Microsoft released a hotfix.

It's honestly great news. That's the best outcome we could have hoped for. A timely fix from the original offender. It just could've come a little sooner...

That's the game you have to play though. Software that you depend on breaks, which in turn, breaks your customers. You only have a couple options. You can do nothing and hope that the offender will release a hotfix in a timely manner, or you can make strides to develop a workaround.

We immediately took the latter approach, as it's the safest and quickest way to get our customers back up and running. Who knows when Microsoft would've released a fix. It could have never come for all we know.

As they say.. better late than never.

Coding Standards Should Not Matter

Yeah that's right. That behemoth of a document that you call your standards document just does not matter. Now that's a pretty bold statement, so let me explain.

I believe that a standards document should be about how to style code. It should be about where to place curly braces, it should answer the great spaces vs. tabs debate (even though there is obviously only ever one clear choice), and similar concerns. What a standards document should not contain is substance, best practices. So what is a standard? A best practice?

It's very likely that the definition of a standard or a best practice vary from individual to individual, so to get everyone on the same page, these are how I define standards and best practices.

  • Standard: A rule put in place by an authority for the purpose of consistency.
  • Best Practice: A proven approach, generally the best way to solve a problem.

The key would in the standards definition is consistency. It should not matter at all how the code is formatted as long as the team agrees to an approach and follows it. Ensuring that the coding style of the repository remains consistent.

Consistency is extremely important. Code should look like a team wrote it, and not any one individual. Everyone on the team is going to be sharing the same code base and the style should not vary from developer to developer. You definitely do not want developers as their first order of business when starting a new task to be to reformat the code to their own personal preference.

A best practice is more along the lines of do not wrap your entire class in a try-catch and swallow the exception to avoid generating errors or keep your collection setters private. While as important as a standard, if not more so, these types of statements should not exist within a standards document.

So why shouldn't a standards document contain both?

First and foremost, they are different concerns. We generally like to split up concerns into easily digestible bits and not combine them. It is not going to matter all that much if your team puts curly braces on the same lines as your conditionals. It is going to matter if you start piling all of your concerns into a single class.

However, from my experience, a standards document is rarely just standards. A standards document will generally combine standards and best practices.

Though I do not believe the solution is to split up these two concerns into different documents. No, I believe a standards document should not exist. Emphasis on document. We should have standards. Unfortunately the medium we choose to present our standards is generally the incorrect one.

So whats the better approach? Let your code be your standards document.

I believe that coding standards are better enforced through tooling and the code base itself. Tools such as ReSharper and StyleCop allow teams to define their standards and then have them be automatically enforced through the development environment. No need to spend time writing out paragraphs in English, comparing and contrasting different styling approaches (Do vs. Don't). No need to worry about document formatting. Simply define your rules and push it to the teams who you intend to consume the styling that was agreed upon.

Like the old saying goes, the best standards document is no standards document at all.

The Rules Pattern (How to Drop Your Guard)

In your travels as a programmer, you will more than likely come across a body of code that looks a little something like the following:

public bool CheckSystem(Computer computer)  
{
    if (computer.Ghz < 3)
    {
        return false;
    }

    if (computer.Ram < 4)
    {
        return false;
    }

    if (computer.DiskSpace < 10)
    {
        return false;
    }

    return true;
}

Here we have a method called CheckSystem which tries to validate whether or not a model of a computer meets all of the system requirements. If the model fails to meet all of the specified requirements, the method will return false. It attempts to validate the model by using a number of different conditionals, one after the other. These types of conditionals are called Guard Clauses.

For a method that may only have a few conditions to check, guard clauses are completely acceptable. There's no need to over complicate things. However, if you find yourself with a large number of conditions to validate and/or you feel that the conditions will change over time, you may want to consider an alternative approach. Enter the Rules Pattern.

The Rules pattern is not a pattern that you'll see in the design patterns book by the Gang of Four, but could be considered an implementation of the Command pattern.

For the purpose of this blog post, we'll be implementing a set of rules to check if a computer meets all of the minimum requirements to run a given piece of software. Here's how it works.

First create an interface that all of your rules will inherit from.

public interface ISystemRequirementsRule  
{
    bool CheckRequirements(Computer computer);
}

The interface will only expose one method which will be used to validate the condition for the rule.

You will then need to create all of your rules. As stated previously, each of your rules will inherit from the same interface. Each rule will be one of your guard clauses.

class DiskSpaceRule : ISystemRequirementsRule  
{
    public bool CheckRequirements(Computer computer)
    {
        var ruleResult = computer.DiskSpace > 10;
        return ruleResult;
    }
}

This rule validates that the disk space of the computer exceeds 10. Ten what? You can associate whatever unit you want, doesn't matter in this case! Create as many of these rules as required to ensure all of your requirements are checked.

Next, you'll need to create a class whose responsibility is to run through and validate every rule that you have created. There are a couple approaches that I would recommend.

public class SystemRequirementsChecker  
{
    var _rules = new List<ISystemRequirementsRule>();

    public SystemRequirementsChecker()
    {
        _rules.Add(new DiskSpaceRule());
        _rules.Add(new RamRule());
    }

    public decimal CheckSystem(Computer computer)
    {
        foreach (var rule in _rules)
        {
            if(!rule.CheckRequirements(computer))
            {
                return false;
            }        
        }

        return true;
    }
}

This approach simply holds all of the available rules in a collection. When you want to validate all of your rules, simply call the CheckSystem method. This method will then iterate through all of the rules that you have defined in the constructor.

If you want an approach that fully embraced the Open/Closed Principle then I would recommend something similar to the following:

public class SystemRequirementsChecker  
{
    private readonly IEnumerable<ISystemRequirementsRule> _rules;

    public SystemRequirementsChecker()
    {
        _rules = GetRules();
    }

    public bool CheckSystem(Computer computer)
    {
        return _rules.All(r => r.CheckRequirements(computer));
    }

    private IEnumerable<ISystemRequirementsRule> GetRules()
    {
        var currentAssembly = GetType().GetTypeInfo().Assembly;
        var requirementRules = currentAssembly
                .DefinedTypes
                .Where(type => type.ImplementedInterfaces.Any(i => i == typeof(ISystemRequirementsRule)))
                .Select(type => (ISystemRequirementsRule)Activator.CreateInstance(type))
                .ToList();

        return requirementRules;
    }
}

This approach uses reflection to find all classes that inherit from the ISystemRequirementsRule interface (all of the rules that you will have written). It then takes all of these rules, instantiates them via CreateInstance, and returns them as a list so that the CheckSystem can iterate through them and validate each and every rule.

The reflection approach allows you to create a new rule with the expected interface, rebuild the application, and that's it. Your new rule will be enforced inside of the SystemRequirementsChecker. No need to touch any of the pre-existing source code!

The former approach, utilizing a list and instantiating each rule in the constructor would require you to create the class and then modify the SystemRequirementsChecker to include your new rule before it knew about its existence.

I'd consider either approach acceptable, it depends on your situation.

So there you have it, the Rules Pattern. A useful pattern that you typically don't run across when studying design patterns. Hope it helps!

Who Moved My Cheese?!

Earlier this morning, I was reminded of an incident that I had with a customer a couple years ago. It went a little something like this:

I was working on a web application. It consisted of a tabular grid that displayed information based on search criteria. Very similar to anything you would see on any sort of e-commerce website that allowed you to filter products by price range, name, etc. The customer wanted to enhance this specific grid. They wanted a new filter that would give them the ability to hide all records that were considered inactive. A fairly reasonable request.

Okay great, requirements established. I began on my merry way.

Now, this sort of request is relatively trivial in our environment. We need to add a new UI control to the filter section and then wire it up to the backing stored procedure. The whole process probably took around two hours from opening the development environment to pushing it out into our production environment. Once deployed to production, we inform the customer that their feature request is now live, they sign off on the change, and are as happy as could be.

...Until I receive a notification that the customer is having difficulty filtering results within the grid, because a filter that used to be there no longer is.

Now, this struck me as odd, because I actually added a filter. So I opened up the application myself to see if I could validate the customers claim. I was not able to do so. Everything looked exactly as I would expect, and nothing changed from when the customer had previously signed off on it.

In an effort to get the potential miscommunication resolved swiftly, I hopped on a conference call with the customer to see if we could get to the bottom of this. We had already done some back and forth on the ticket itself, so the customer (lets call him Mark) and I had already established a little bit of a rapport. The conversation went something along the lines of:

John: Hey Mark, It's John. So show me the screen you're looking at. I'm seeing the filter on me end.
Mark: See here? The filter isn't on the top left.
John: Yeah we added a new filter yesterday, so the filter that you're looking for is right next to it.
Mark: Oh, I see it now. Yeah, that's not going to work. We would have to retrain all of our employees.
John: Why is that?
Mark: Our documentation on how to use this screen in our facility includes screenshots of the filters. It explicitly states which filters to use and where they are on the screen. We would have to print out entirely new documentation for our facility and re-train all of our employees.

This immediately got me thinking about a story I had read some time ago called Who Moved My Cheese? I'll let you read the Wikipedia entry if you are so inclined, but it essentially speaks to how people have a hard time adjusting to change. Now, for me, this came out of nowhere. Never would I have guessed that the customer had such rigid documentation of how to use our software.

The software that I develop and maintain changes many times per days. Sometimes we push over a hundred changes to the system in a given day. It's fluid, changing constantly. So to think that a user would assume our layout had no potential for change and would revolve their whole training philosophy around something that had the potential to change is a little boggling. On top of all of this, it was the customer who requested the change, just a different department.

I ended up getting the situation resolved by getting the other department involved and explaining the situation. The change was going to stick, there really wasn't a way around it.

So here we had a feature request directly from the customer, the change approved in our quality environment, and then verified on our production environment. Only to be followed up with an urgent message from the same customer stating that their workforce was slowed because the training was no longer valid after the change was made.

The whole situation really made me take a step back and realize that you can never be completely confident in any change you make to an existing system.

A Comment on Comments

Full disclosure, I really don't like comments.

Like any feature of a language, I believe they have their purpose. After all, they were put in the language in the first place. I've just seen a lot of incorrect uses of comments, and the more I run into them, the more I feel like they don't add a whole lot of value.

Lets dive into some of the reasons why.

Comments bloat classes

I see auto generated comments everywhere. They plague a large portion of our code base. They have good intentions, but often end up making the file larger and harder to understand. Especially when the developer pays no mind to the content that was generated.

public class Cube  
{
    /**
    * The height of the cube.
    */
    public Height { get; set; }

    /**
    * The width of the cube.
    */
    public Width { get; set; }

    /**
    * The depth of the cube.
    */
    public Depth { get; set; }
}

We've turned a simple model of a cube which could be a few lines long, into a bloated model with more lines of comments than of actual useful code. Everything that the comments are describing are self evident. The comment headers just aren't needed. In all honesty, if I see something similar to the above, I just go ahead and delete the comments.

Comments are code duplication

One of my favorite light bulb moments. We as developers like to avoid code duplication, reuse is king. If you take a step back and think about comments in this regard, that's exactly what they are.

Code explains what is going to be done. Comments explain what is going to be done. They just do so in different languages. Just like with code duplication, if you change a line of code, you best remember to also change the comment lest you want to be left with:

/**
* ...
<summary>Returns true when the test succeeds; false otherwise.</summary>  
*/
public TestResult RunTest()  
{

You can probably guess that this method used to be a bool, and it was decided that the returned result needed to be a little more verbose. So the developer went ahead and created a TestResult object to store some more information about the result of the test. However, they forgot to update the comment, and now it's out of sync

Comments can be a sign of a bigger problem

Even if you believe you are using comments as intended, to explain potentially confusing code, a better solution is to just make the code less confusing.

if(!(x > 10 && y < 5 && z == -1)) // ensure the object is not out of bounds 
bool isOutOfBounds = (x > 10 && y < 5 && z == 0)  
if(isOutOfBounds) {

You could even go a step further and assign each int its own const variable so the user isn't guessing what exactly 10, 5, and 0 even mean.

Finally.. I've seen my fair share of auto generated comments, and you know what?

Auto generated comments make me cry
/**
* Processes the async
*/
public async Task<Result> ProcessAsync()

Oh.. I had no idea.

Anyway, after all this comment bashing, lets talk about a couple cases that I believe to be a strong candidates for comment use.

Public APIs are a great use of comments. You are going to have a large body of people consuming your code, calling your methods. It can be greatly helpful if everything is well documented, explaining exactly how everything is supposed to work.

Another valid use for comments is explaining why code is present. I like to think that code explains how it works, where comments explain why it works. Code can only make so much sense, regardless of how you name your variables, how well you format it, etc. Sometimes, there exists a scenario that just needs a little more explanation.

/* Reflection may seem like an odd choice here, but after testing the use case
* scenarios it proved to be much faster than splitting everything up into database
* calls */

Like I said in the beginning. Comments were put into languages for a reason, they have their place. Please, just use them sparingly.