Merge: serialization: subtype check attributes & serialize part of the metamodel
authorJean Privat <jean@pryen.org>
Mon, 24 Oct 2016 18:48:10 +0000 (14:48 -0400)
committerJean Privat <jean@pryen.org>
Mon, 24 Oct 2016 18:48:10 +0000 (14:48 -0400)
commit7dbf5c9049ce856337bc5ccaf180226611138fd2
treeb636e4ce51af040382cdd2099e42a665acb99658
parente29762ebe29c86649597a484f2990e75b4e140de
parentb6e80035966a1072c68f1e9fb404d712eb7e746f
Merge: serialization: subtype check attributes & serialize part of the metamodel

This PR is a bit out of the ordinary, it tackles 2 issues with serialization:

1. Avoid deserializing unwanted types to fix a security hole where a refined attribute setter could be abused to cause side effects in the running program.

   The solution inserts 2 checks in the deserialization process, for JSON only:

   1. Check the dynamic type of the Nit object to create from a JSON object against the static type of the attribute. It is created and deserialized only if the dynamic type is a subtype of the static type. This check is activated by default, it can be turned off for some rare use case.

   2. A whitelist lists the accepted dynamic types to deserialize. It can add an extra level of security above the subtype check, or manually replace it when it can't be used. If the whitelist is empty, this check is disabled, the default.

   Pseudo-code algo:
   ~~~
   for name, value in json_object_as_hash_map do
       if value isa json_object_as_hash_map do
           # Existing: find the dynamic type from __class, class_name_heuristic or fallbacks on the static type
           var dynamic_type = heuristic_dynamic_type(value)

           # New
           check whitelist.has(dynamic_type)
           check dynamic_type isa static_type
       end
   end
   ~~~

   Note that attributes typed by `Object` still accept anything (as expected). But there is also a security hole with attributes typed by a virtual type bound to `Object`. We don't have runtime access to the precise type of the virtual attribute so we always use the upper bound `Object`. So `Array` and the like are still a vulnerable point.

2. Ask for a specific static type at serialization while preserving polymorphism.

   The solution adds an argument `static_type` to `JsonDeserializer::deserialize` to ask for a specific type. The deserialized object will be a subclass of `static_type`, or `null` on error.

   This should be preferred to calling directly `from_deserializer` as it allows for subclasses and return null on fatal errors.

   ~~~
   var engine = new JsonDeserializer("""{"item": 4}""")
   var obj = engine.deserialize("Ref[Int]")
   assert engine.errors.not_empty
   assert obj isa Ref[Int]
   ~~~

These features rely on a new intern service `class_inheritance_metamodel_json` (I'm not convinced by the name but at least it is private). This method returns a `POSet[String]` serialized to JSON representing the inheritance graph of the running program. The compiler stores the JSON string in the generated C code, it takes about 200kb for a small program.

It would be pretty easy to add other services to read the metamodel, as needed. However, there is a strong coupling between the JsonDeserializer and `class_inheritance_metamodel_json`, this could be solved by adding a few modules with abstract services.

Future work:
* Do the same in the binary serialization engine.
* Move the metamodel services out of the `json` package.
* With a runtime access to the precise type of virtual types (`V.class_name`),
  fix hole with attributes bound to a `nullable Object` static type.
* Do a full subtype check, not only subclass check (for generic classes).
* Use full qualified names in the serialized metamodel to avoid name conflicts.

@ppepos Did I correctly isolate the security issue? There is an example of the vulnerability in the test.

Pull-Request: #2311
Reviewed-by: Lucas Bajolet <r4pass@hotmail.com>
Reviewed-by: Romain Chanoir <romain.chanoir@viacesi.fr>
Reviewed-by: Jean Privat <jean@pryen.org>