General

• Before starting update/fix/remove TODO / FIXME / XXX so they don't get reported by the reviewer (which would mean fixing them anyway then another review for the results)

Managed Code

The security model provided by CoreCLR makes it easier to review the most critical parts of the managed class libraries bundled with Moonlight 2. Basically there's two types of code:

• Transparent: this includes application code and a large part of the class libraries. Transparent code has some limitation (e.g. no unsafe code, no p/invokes...) that makes it secure to execute untrusted, transparent, code inside the sandbox provided by CoreCLR.

• Critical: this code has unrestricted access to the system (i.e. fully trusted by the runtime). Bugs inside critical code could compromise the sandbox.

Critical code is also split in two:

• [SecuritySafeCritical] is code that should be safe to call from transparent code. "Should be" because we have tools to detect and tag them properly - but tagging that does not make them safe.

• [SecurityCritical] is code that is not safe to call (e.g. unsafe code, p/invokes) from transparent code. This restriction is enforced by CoreCLR. [SecurityCritical] code is generally called by [SecuritySafeCritical] code (but can be called by the runtime too).

So the major goal of this part of the audit is to make sure [SecuritySafeCritical] is (instead of should be) safe to be called by transparent code.

How

• Moonlight build tools automatically detect (and inject) the correct attributes for both [SecuritySafeCritical] and [SecurityCritical] code. All other code is transparent (default). So there is no need to manually classify managed code.

• The 'audit.exe' tool is used to generate/update the list of [SecuritySafeCritical] methods and their review status. If the code of a method change then it's status goes back to unreviewed. This makes it possible to gradually review the code and keep the results meaningful.

• To review code you simply need to looks at the unreviewed methods (one file per assembly), find it's source (either in /mcs/class or in /moon/class), review the code and update the audit data file.

Review

• First, spot why the method is critical. There could be several reasons why it was tagged as such.
• Understand the flow between the (trusted code / user) provided parameters and how they are being used (or passed) to the critical code.
  • Looking at the callers is not useful if the method is visible outside the assembly - in this case anyone (transparent code) can directly call it. Even if the method is not visible it's better not to assume anything about the caller(s) since this could change and won't be detected by audit.exe
• Make sure all parameters (and the return value) are validated, e.g.
  • values: like null, negative integers, NaN
  • file path restrictions

• Keep an eye on the math, in particular around bounds checks, since the ordering of the operations could lead to integer overflows.

Remember: From the platform (i.e. Moonlight) point of view [SecuritySafeCritical] is not safe (safety is our promise to application code) but is critical - it can do anything without being bothered by the CoreCLR restrictions.

Questions about the code ? ping the author(s) on IRC!

Alternative solutions

• Large, complex methods might be refactored into smaller methods. This can allow you to move all critical code in a, simpler, method (cutting review time).

• If the critical parts are not required for Moonlight then adding a NET_2_1 around it will turn this method into a transparent method.