A review of ITTC code.

This review was conducted before objectives crystallised. Activities: read key modules, ran test key creation, analysed one major bug in cert signing test (fix written but not compiled nor tested).

Headline conclusions

We can measure the code according to a few metrics:

Results

Good points

Own protocol layout for on-the-wire units

Instead of using ASN1 or some other volative system, the ITTC implements its own simple formats.

Bad points

Things I don't like.

Protocol is not defined

The protocol assumes that the other end is an exact copy; as the types and sizes are not defined (uses enums to allocate the numbers). This means that all share servers have to be upgraded together, in case there are changes.

(Lack of) Defensive Coding

The coding style is not defensive. In practice, a function does all of what it intends, so when it gets complex, the if statements get messy. Specifically,

• Instead of reducing complexity by returning fast, it mixes in a few big ifs followed by dangling returns.
• Frequent use of mallocs within both complex and simple patterns suggests memory leaks, potential for crashes, and a lot of effort in proving that it has got the memory management right.
• Implements own stack paradigm, passing "heap" around, saving it, manipulating it, then restoring it. This is not thread safe. (This may be a result of working with openssl bignums.)
• Worse, it mixes lock management into the functions, so that to unlock and return there are a lot of complex paths.
• Hence, a tendency to use gotos to handle error conditions and singular unlocks,
• Inappropriate use of #defines. (apps_startup)

Most of which could be eased by better layering within functions and use of more functions to firewall areas.

1. ittc_req main() function does it all ... from arg parsing to key creation.

Protocol

• Protocol is incomplete, sending Hello called ACKs and not confirming with Acks. UTL_Connection.UTL_SetupConnection()
• The code seems to be written from a trust-the-other implementation pov.

Signature Requestor ittc_req

The signature requestor application is one single file, with primarily one single function. It even creates the bignums within.

API to ITTC

The API is effectively opening the key using ITTC_load_RSA_key(char *keyfilename) which returns a key that is pre-loaded with the context for the share servers.

It could be that this is clever because it then allows the OpenSSL key to be used as any other key?

OpenSSL

Style, etc

Inconsistent styles. Very little commentary.

Points that need to be secured

There appear to be these primary critical zones.

Server port

Each server will need to protected. The code for the protocol, incoming request handlers, outgoing request handlers, etc needs to be analysed to see how well it protects itself.

If protection is poor, the servers will have to do it (boring array of VPN, firewalls, etc).

Request Authentication

Are the client requests authenticated? If not, the system is vulnerable to insider attack. (Probably CSRs are signed and ok. Checked?)

Client program

This is primarily about checking that it is robust, and can be firewalled off.

Points that need to be robustified

Client and server. Check the parsing of bad keys / requests. Fork protection in the server? Mallocs and structure are going to make this hard.

Miscellaneous

ITTC has these lines of code: