Cryptolens/cryptolens-dotnet

Method to generate LicenseKey signature string is fragile

Opened this issue · 2 comments

Deleted user commented

When verifying if a key is genuine, the signature string is a csv string created from a property name/value dictionary generated using reflection on the license key instance.

cryptolens-dotnet/Cryptolens.Licensing/SKMv3/ExtensionMethods.cs

Line 185 in 4da0f4c

var rawResult = licenseKey.AsDictionary();

Since the order of the elements is critical in the signature csv string, this is extremely fragile.

  1. The order of the elements in a dictionary is non-deterministic. Most of the time they will enumerate in the order elements were added, but it is not guaranteed. Perhaps a SortedDictionary or OrderedDictionary could be used, but I don't think it is enough in itself.
  2. The properties values are discovered and added using reflection. This is not problem, but the order of the members in the class definition will affect the outcome. So any code reorganization would break the output. Additionally if properties unrelated to the serialization/deserialization are added to a derived LicenseKey class it would break.

I see two possible solutions:

  1. Create a dedicated string LicenseKey.ToCsvString method that would return precisely the csv string. Order and which property is included is totally controlled.

  2. An OrderedDictionary used in combination of either IndexAttribute or JsonPropertyAttribute(Order = xx) on each property with the exact order. When using reflection we sort the items per their assigned index. Properties that are not decorated are ignored.

I think solution 1 is the easiest, but explicitly tagging each serialized properties with JsonProperty(name, index) would make it more robust and obfuscation friendly because serialization would not rely on member names, but literal strings with the serialized property name. Perhaps a combination of 1 and 2. Thoughts? Ideas?

How is the order of the properties determined server-side when generating the signature?

artemlos commented

@Paramethod thank you for the feedback.

You are right about that the order is not guaranteed, and MSDN advises against relying on a particular order.

The GetProperties method does not return properties in a particular order, such as alphabetical or declaration order. Your code must not depend on the order in which properties are returned, because that order varies [1]

On the server side, we use the declared order. From my experience, all versions of .NET Framework (from 3.5) use the declared order. This problem has only occurred for users running on Mono, for which a different protocol is used: https://help.cryptolens.io/getting-started/unity

If possible, I would recommend to use a different overload of Key.Activate which does not take ActivateModel. The protocol that is used is the same that we use in other languages and is the one that should preferably be used in newer implementations.

// call to activate
var result = Key.Activate(token: auth, productId: 3349, key: "GEBNC-WZZJD-VJIHG-GCMVD", machineCode: "foo");

// obtaining the license key (and verifying the signature automatically).
var license = LicenseKey.FromResponse("RSAPubKey", result);

Your suggestion sounds good. I think it should be enough to have the order enforced using attributes and keep the normal dictionary, but I need to review the docs more carefully.

A possible solution could be to change the AsDictionary as follows and by introducing a new attribute:

return source.GetType().GetProperties(bindingAttr).Where(x=> x.MemberType == MemberTypes.Property).OrderBy(x=> x.GetCustomAttributes(typeof(OrderAttribute), false).FirstOrDefault()).ToDictionary
(
    propInfo => propInfo.Name,
    propInfo => propInfo.GetValue(source, null).ToStringCultureSpecific()
);

I did some tests this morning but got a few errors, so I will look at it a bit later.

P.S. The long term plan is to completely abandon the old protocol and move to the new one in the .NET SDK.

[1] https://docs.microsoft.com/en-us/dotnet/api/system.type.getproperties?view=net-5.0

Deleted user commented

Hi @artemlos

Since this is considered old protocol I think the simplest solution to make the older protocol stable is add this method to the LicenseKey class

        internal string ToSignatureCsvString()
        {
            return $"{ProductId},{ID},{Key},{Created.ToStringCultureSpecific()},{Expires.ToStringCultureSpecific()},{Period},{F1},{F2},{F3},{F4},{F5},{F6},{F7},{F8},{Notes},{Block},{GlobalId},{Customer},{ActivatedMachines.ToStringCultureSpecific()},{TrialActivation},{MaxNoOfMachines},{AllowedMachines},{DataObjects.ToStringCultureSpecific()},{SignDate.ToStringCultureSpecific()},";
        }

I will try the new protocol today.

As for reflection and the OrderAttribute. I think all serialized properties should be decorated with a JsonPropertyAttribute. This would ensure the serialization is not relying on member names and explicitly state which property is serialized. It would also enable us to obfuscate the library without breaking serialization. The added benefit, is that the JsonPropertyAttribute has an Order property which could be used when ordering is required.

  • 👍1