Code Highlighting

Tuesday, December 16, 2014

Ladies and gentlemen: salt your passwords

A few weeks ago I recovered the database of an old website from backup. The customer wanted the old contact info and had lost its own backup.
The old database hashed its passwords (SHA-1), but did not salt them. Looking at the old users table, I wondered how well the rainbow table approach would work - using Google as a big rainbow table. Just feed the hash into the search field and see what comes up.

Here are the results:
PasswordClear textCount
7C4A8D09CA3762AF61E59520943DC26494F8941B12345636
9C2028963DC9F7FBB4CB30140428A210C61DBB2Cwachtwoord24
9CF95DACD226DCF43DA376CDB6CBBA7035218921azerty22
62E839476B23E579EFB96B47391599FFCA4CFA94?18
7110EDA4D09E062AA5E4A390B0A572AC0D2C0220123418
F001F2E438738807D3079BFFAF66519B9D0F26C7azqswx16
F7C3BC1D808E04732ADF679965CCC34CA7AE344112345678916
D18C9E9DCCE902D6A3E21E72BADA443AC294CB5FPaswoord15
FDA48D6A63F351DC46D411336DE4BA33F77B66F5voetbal14
D22BC6BAD61129B636AFFA2511B3CE522CD74BB3brecht13
EE8D8728F435FD550F83852AABAB5234CE1DA528iloveyou12
48F285F9A1E15CB6240506182A3C08AEDD639F26dansen11

The count indicates how many users chose that password. Users spoke dutch, used an azerty keyboard. Of the top 12, I found all but 1 using a Google search. This is why you salt your passwords.

Mind you, it's also a reason to implement a decent password policy. One that does not allow '1234'.

I really wonder about that 1 that I wasn't able to find though. Must be some dutch word.

Friday, September 5, 2014

OutputCache on User Controls - VaryByControl

A question on stackoverflow prompted me to research this. Initially a code sample was included, showing how this user was trying to set up cache variation by property value. The VaryByControl parameter was set to the name of this property. This appeared to work for property values entered in markup, but failed if the property was set to a variable: the control bound to the property only ever showed the first value is was bound to.

I set up a test project to attempt to reproduce the problem, and saw identical behavior. So far so good.

<!-- the aspx page, the Number property is set to a random number -->
<%@ Register Src="~/UserControls/Test.ascx" TagPrefix="uc1" TagName="Test" %>

<asp:Content runat="server" ID="BodyContent" ContentPlaceHolderID="MainContent">
    <uc1:Test runat="server" ID="Test1" Number="1" />
    <uc1:Test runat="server" ID="Test2" Number="2" />
    <uc1:Test runat="server" ID="Test3" Number="<%# Number %>" />

    <asp:Label runat="server" Text="<%# Number %>"></asp:Label>
</asp:Content>

<!-- the user control -->
<%@ OutputCache Duration="180" VaryByControl="Number" VaryByParam="None" Shared="true" %>

<h1><%# Number  %></h1>


Detective hats everyone! Let's investigate how outputcache for user controls works!

First the code generation for user controls. An attribute is applied to the generated class that contains the caching parameters:

    [System.Web.UI.PartialCachingAttribute(180, null, "Number", null, null, true, ProviderName = null)]
    public class usercontrols_test_ascx : global::TestOutputCache.UserControls.Test

The parser for the page checks user controls for the caching attribute and - if present - creates a PartialCachingControl, passes it the caching parameters, control ID and a control builder delegate:

    System.Web.UI.StaticPartialCachingControl.BuildCachedControl(@__ctrl, "Test1", "50114342", 180, null, "Number", null, null, new System.Web.UI.BuildMethod(this.@__BuildControlTest1), null);

Inside the PartialCachingControl, a hash is calculated based on the variation parameters, and the cache is checked for the hash. A cache miss results in the delegate being called, and the results getting stored in cache.
Here is the relevant portion of the hash calculation routine:

        if (cachedVary._varyByControls != null) {

            // Prepend them with a prefix to make them fully qualified
            string prefix;
            if (NamingContainer == Page) {
                // No prefix if it's the page
                prefix = String.Empty;
            }
            else {
                prefix = NamingContainer.UniqueID;
                Debug.Assert(!String.IsNullOrEmpty(prefix));
                prefix += IdSeparator;
            }

            prefix += _ctrlID + IdSeparator;

            // Add all the relative vary params and their values to the hash code
            foreach (string varyByParam in cachedVary._varyByControls) {

                string temp = prefix + varyByParam.Trim();
                combinedHashCode.AddCaseInsensitiveString(temp);
                string val = reqValCollection[temp];
                if (val != null)
                    combinedHashCode.AddObject(reqValCollection[temp]);
            }
        }


Variable "reqValCollection" is a dictionary of POST values (if there are any) or GET values.
And the first question presents itself, because the VaryByControl only varies by POST values corresponding with the controls specified. The property value cannot matter: it is never read.

In fact; when deciding to server the cached markup or not, the control has not been created yet, and may not ever be. That's the entire point of the output caching. Obviously variation by Property value doesn't make sense.

So why does it ever work?

Back to the test project to confirm hypothesis by setting VaryByControl to something else.
<%@ OutputCache Duration="180" VaryByControl="PorkPies" VaryByParam="None" Shared="true" %>

<h1><%# Number  %></h1>

Yup. Everything still works identically to how it worked before. The value of VaryByControl does not matter, as long as there is a value. Dropping the VaryByControls entirely shows normal behavior: one single cached version being served for each request.

Smells like a bug! Lets see if we can find it!

In fact, if you look up to the code I posted, the bug is right there. The "temp" variable contains the unique id of the user control. I imagine this field is added to prevent a cache collision for two controls - both specified in VaryByControl - with identical values. But it is added to the combinedHashCode even before it is verified to have a value in reqValCollection. It still works because the HashCodeCombiner class generates a different hash for the same objects added in a different order - it does not simply xor everything and calls it a day.
That means User Control OutPutCache has a built-in variation for:

  • Control ID, and
  • NamingContainer ID
You can activate it by setting VaryByControl to any value whatsoever. I would argue that the correct code looks like this:

            // Add all the relative vary params and their values to the hash code
            foreach (string varyByParam in cachedVary._varyByControls) {

                string temp = prefix + varyByParam.Trim();
                string val = reqValCollection[temp];
                if (val != null){
                    combinedHashCode.AddCaseInsensitiveString(varyByParam.Trim());
                    combinedHashCode.AddObject(reqValCollection[temp]);
                }
            }

Perhaps there is a good reason to include the ID and naming container in the cache key, but it does not make sense to me.

I don't expect Microsoft will ever fix this bug; it would break a multitude of sites in very ugly ways. I'm guessing they've found it before, and decided to leave well enough alone.

Thursday, July 3, 2014

What if events in C# were, you know, cooler?

In the beginning events in C# were awesome. Some of us came from Java, and used to have to implement interfaces named IButtonClickListener on some inner classes whatever nonsense just to react to some button being clicked, but not in C#. No, in C# you just did the following:

    public partial class Form1 : Form
    {
        Button newButton;

        public Form1()
        {
            InitializeComponent();

            newButton = new Button()
            {
                Text = "Click me!",
                Left = 20,
                Top = 20
            };

            newButton.Click += new EventHandler(HandleClick);

            this.Controls.Add(newButton);
        }

        private void HandleClick(object sender, EventArgs e)
        {
            MessageBox.Show("Hi there!");

            newButton.Click -= HandleClick;
        }
    }

No obnoxious interface nonsense! Delegates! The future is now!

But over 10 years later we have gotten spoiled with anonymous delegates and lambdas, and all of a sudden the C# event model does not seem so cool anymore. Because when you hook up an event handler like this:

newButton.Click += (sender, e) => MessageBox.Show("Hi there!");

do you imagine you can remove it like this:

// fugetaboutit!
newButton.Click -= (sender, e) => MessageBox.Show("Hi there!");

No way! You would have to save the handler to a variable to have a reference handy to be able to remove
the handler later:
EventHandler handler = (sender, e) => MessageBox.Show("Hi there!");

Worse! Microsoft added lots of nice generic delegate types Action<T>, Action<T1, T2>, Action<T1, T2, T3> etc that we can't use because we're stuck with the EventHandler delegate types for Events. You would need a wrapper around the Invoke function to make that work.

Would you like to check out what is currently hooked up to your event? Tough!

Clear the whole thing and start over? Get outta here!

But despair no more! I have written a half-assed solution that I think might have been marginally better if it had worked properly!

Presenting: the new and improved Event:


    public class Event<TEventArgs,TEventHandler>
        where TEventArgs : EventArgs
        where TEventHandler : class
    {
        // the main store for handlers
        private Dictionary<int, Action<object, TEventArgs>> handlers =
            new Dictionary<int, Action<object, TEventArgs>>();

        // a dictionary that maps the original handlers to the generated wrappers
        private Dictionary<object, int> operatorAddedHandlers =
            new Dictionary<object, int>();

        public Event()
        {
        }

        static Event()
        {
            // check if my TEventHandler type parameter is a delegate type
            // where TEventHandler : Delegate is not supported
            if (!typeof(TEventHandler).IsSubclassOf(typeof(Delegate)))
            {
                throw new InvalidOperationException(typeof(TEventHandler).Name + " in generic type parameter TEventHandler is not a delegate type.");
            }
        }

        /// <summary>
        /// Maps events of the same name on the container object to the Event field that hides it
        /// </summary>
        /// <param name="eventGenerator">the object that contains the Event field (typically this)</param>
        public void CaptureBaseEvents(object eventGenerator)
        {
            var fieldNames = new List<String>();
            var classType = eventGenerator.GetType();
            var fields = classType.GetFields();
            
            // check which field the current instance has been assigned to, store the name
            foreach (var field in fields)
            {
                var val = field.GetValue(eventGenerator);
                if (Object.Equals(val, this))
                {
                    fieldNames.Add(field.Name);
                }
            }

            // get a MethodInfo for this class's Invoke method
            var invokeHandler =
                typeof(Event<TEventArgs, TEventHandler>).GetMethod("Invoke");

            // check if any base events exist that have the same name
            // if so, hook up our Invoke method
            foreach (var fieldName in fieldNames)
            {
                var classEvent = classType.GetEvent(fieldName);

                if (classEvent != null)
                {
                    var tDelegate = classEvent.EventHandlerType;
                    var d = Delegate.CreateDelegate(tDelegate, this, invokeHandler);

                    var addHandler = classEvent.GetAddMethod();
                    Object[] addHandlerArgs = { d };
                    addHandler.Invoke(eventGenerator, addHandlerArgs);
                }
            }
        }

        /// <summary>
        /// Invokes the event handlers
        /// </summary>
        /// <param name="sender">The object that triggered the event</param>
        /// <param name="e">The event parameters</param>
        public void Invoke(object sender, TEventArgs e)
        {
            // just in case a handler modifies the handlers
            var handlerDelegates = handlers.Select(hs => hs.Value).ToList();

            foreach (var handler in handlerDelegates)
            {
                handler.Invoke(sender, e);
            }
        }

        /// <summary>
        /// Removes all handlers
        /// </summary>
        public void ClearHandlers()
        {
            handlers.Clear();
        }

        /// <summary>
        /// Gets a list of all handlers and their removal token
        /// </summary>
        /// <returns></returns>
        public IEnumerable<EventHandlerSet<TEventArgs>> GetAllHandlers()
        {
            // store in list in case someone loops through to find a handler to remove
            // that seems kind of likely
            var handlerSets = handlers.Select(hs => new EventHandlerSet<TEventArgs>()
            {
                Handler = hs.Value,
                RemovalToken = hs.Key
            }).ToList();

            return handlerSets;
        }

        /// <summary>
        /// Adds an event handler
        /// </summary>
        /// <param name="newHandler">The new handler to add</param>
        /// <returns>The removal token for the newly added handler</returns>
        public int AddHandler(Action<object, TEventArgs> newHandler)
        {
            // we don't hook up the same handler twice,
            // just return its current removal token
            if (handlers.Any(ks => ks.Value.Equals(newHandler)))
            {
                return handlers.Where(ks => ks.Value.Equals(newHandler)).Single().Key;
            }

            // use a new guid cut off at int32 length for key generator
            var newGuid = Guid.NewGuid();
            int newKey = BitConverter.ToInt32(newGuid.ToByteArray(), 0);

            // it's a lot more likely at 32 bits
            while (handlers.ContainsKey(newKey))
            {
                newGuid = Guid.NewGuid();
                newKey = BitConverter.ToInt32(newGuid.ToByteArray(), 0);
            }

            handlers.Add(newKey, newHandler);

            return newKey;
        }

        /// <summary>
        /// Adds a new handler to the Event field
        /// </summary>
        /// <param name="thisEvent">The event field</param>
        /// <param name="newHandler">The handler to add</param>
        /// <returns></returns>
        public static Event<TEventArgs, TEventHandler> operator +(Event<TEventArgs, TEventHandler> thisEvent, TEventHandler newHandler)
        {
            var handlerDelegate = newHandler as Delegate;

            if (handlerDelegate == null)
                throw new InvalidOperationException("Handler is not a delegate type!");

            // if this handler has already been added, do nothing
            if(thisEvent.operatorAddedHandlers.ContainsKey(newHandler)){
                return thisEvent;
            }

            var removalToken = thisEvent.AddHandler(delegate(object sender, TEventArgs e)
            {
                handlerDelegate.DynamicInvoke(new object[] { sender, e });
            });

            thisEvent.operatorAddedHandlers.Add(newHandler, removalToken);

            return thisEvent;
        }

        /// <summary>
        /// Removes the specified handler
        /// </summary>
        /// <param name="handler">The handler to remove</param>
        public void RemoveHandler(Action<object, TEventArgs> handler)
        {
            if (handlers.Any(ks => ks.Value.Equals(handler)))
            {
                var removalToken = handlers.Where(ks => ks.Value.Equals(handler)).Single().Key;

                handlers.Remove(removalToken);
            }
        }

        /// <summary>
        /// Removes the specified handler
        /// </summary>
        /// <param name="removalToken">The handler's removal token</param>
        public void RemoveHandler(int removalToken)
        {
            if (handlers.ContainsKey(removalToken))
                handlers.Remove(removalToken);
        }

        /// <summary>
        /// Removes the specified handler
        /// </summary>
        /// <param name="thisEvent">The event field</param>
        /// <param name="handler">The handler to remove</param>
        /// <returns></returns>
        public static Event<TEventArgs, TEventHandler> operator -(Event<TEventArgs, TEventHandler> thisEvent, TEventHandler handler)
        {
            if (!thisEvent.operatorAddedHandlers.ContainsKey(handler))
                return thisEvent;

            var removalToken = thisEvent.operatorAddedHandlers[handler];

            thisEvent.RemoveHandler(removalToken);

            return thisEvent;
        }
    }

    public class EventHandlerSet<TEventArgs>
    {
        internal EventHandlerSet() { }
        public int RemovalToken { get; set; }
        public Action<object, TEventArgs> Handler { get; set; }
    }

I've made it so that it's mostly a drop-in replacement for pre-existing events. Obviously you need to inherit from the class implementing the events. So let's take that boring old button we had, and jazz up the click event:


    public class NewAndImprovedButton : System.Windows.Forms.Button
    {
        public NewAndImprovedButton()
            : base()
        {
            Click.CaptureBaseEvents(this);
        }

        public new Menno.Event.Event<EventArgs, EventHandler> Click = new Menno.Event.Event<EventArgs, EventHandler>();
    }

Turns out you can hide an event with a field. Who knew? Also, the CaptureBaseEvents in the constructor only works because fields are initialized before the constructor runs. Otherwise I'd have had to do something odd and confusing. More so.

If we take the first piece of code and replace Button by NewAndImprovedButton, like so:
    public partial class Form1 : Form
    {
        NewAndImprovedButton newButton;

        public Form1()
        {
            InitializeComponent();

            newButton = new NewAndImprovedButton()
            {
                Text = "Click me!",
                Left = 20,
                Top = 20
            };

            newButton.Click += new EventHandler(HandleClick);

            this.Controls.Add(newButton);
        }

        private void HandleClick(object sender, EventArgs e)
        {
            MessageBox.Show("Hi there!");

            newButton.Click -= HandleClick;
        }
    }

Everything keeps working exactly the way it did. I implemented the + and - operators to mirror the current Event syntax, and the CaptureBaseEvents method hooks up the event in the base class that the field hides to the Invoke method. Magic!
But we did not make this to keep working the same way. Here's what's new:

  • an AddHandler method that takes an Action<object, TEventArgs> and returns an int that functions as a token.
  • (obviously) a RemoveHandler that an int token.
  • for good measure: a RemoveHandler that takes an Action<object, TEventArgs>
  • a ClearHandlers method
  • a GetAllHandlers method that returns all handler delegates and their tokens, even the ones added through reflection.
Of course there are some rough edges too:
  • Field cannot be readonly if you use the operators. Anything could happen.
  • Operators generate wrappers through reflection. Performance is bound to be questionable.
  • Probably hundreds of corner cases I haven't considered.
Still, now you can do this:

            int removalToken = 0;
            removalToken = newButton.Click.AddHandler((sender, e) =>
            {
                MessageBox.Show("Hi there!");
                newButton.Click.RemoveHandler(removalToken);
            });

For some folks, that's worth a few rough edges.