Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Randomize CoseHeaderLabel hash codes #19

Closed
wants to merge 2 commits into from
Closed

Conversation

GrabYourPitchforks
Copy link
Owner

@GrabYourPitchforks GrabYourPitchforks commented Jun 10, 2022

Summary

Dictionary<TKey, TValue> instances are not secure against untrusted inputs except for when TKey = string. In our scenario, since COSE header values are adversary-provided and the COSE message is backed by an Dictionary<CoseHeaderLabel, ...> populated by attacker-provided keys, this represents an algorithmic complexity (hash code collision) DoS attack vector.

Note the demonstration below, which shows a clear O(n^2) algorithmic complexity as n entries are added to the map.

static void RunTest(int collidingKeyCount)
{
    var map = new CoseHeaderMap();
    var collidingKeys = GenerateCollidingKeys(collidingKeyCount);

    Stopwatch sw = Stopwatch.StartNew();
    foreach (var key in collidingKeys)
    {
        map.SetValue(key, 42);
    }
    sw.Stop();

    Console.WriteLine($"Took {sw.Elapsed} time to add {collidingKeyCount} entries.");
}
Took 00:00:00.0009202 time to add 1000 entries.
Took 00:00:00.0035026 time to add 2000 entries.
Took 00:00:00.0274130 time to add 4000 entries.
Took 00:00:00.0801631 time to add 8000 entries.
Took 00:00:00.2482827 time to add 16000 entries.
Took 00:00:00.9334852 time to add 32000 entries.

Discussion

The downlevel "generate a randomized string hash code value" logic could be a little faster, but at that point it would require dropping down to unsafe-equivalent code. I didn't want to do that here until we have real evidence that we need such an optimization.

I also didn't add a "validate that there aren't a significant number of collisions in the dictionary" unit test because they're very complicated and take a while to run. See here for an example of such a test elsewhere in our repo. I updated the existing CoseHeaderLabel.GetHashCode unit test so that it will catch the case where somebody inadvertently reverts the method logic. That should be sufficient here, but I'm open to suggestions if you feel this needs more coverage.

@GrabYourPitchforks
Copy link
Owner Author

Checked in to runtime.

@GrabYourPitchforks GrabYourPitchforks deleted the cose_hashcode branch June 13, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant