Comment 7 for bug 1266066

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I reviewed strongswan version 5.1.2-0ubuntu1 as checked into trusty.

This review is different from most; strongswan is significantly more
complicated than the usual tools I've audited and nearly every line
of code is highly relevant to system security. So I've reviewed only a
portion of the "prompted" lines provided by our tools and investigated
all the results provided by cppcheck.

The only issue of note from cppcheck is about referencing some
auto-allocated memory after the function that declared the space had
returned; it is a testing routine to ensure that memory scrubbing
functions properly, and while it wildly violates standards and good
practices, in this specific case it makes sense. This routine may cause
problems if we eventually use Intel's Memory Protection Extensions, so
someone keep this in mind for that day.

The strongswan documentation is fantastic, the provided examples and
automated testing performed upstream are nice, the unit tests run during
build are useful, the quality of the code that I did inspect looked high.

The packaging does have extensive lintian errors, 137 instances of
unstripped-binary-or-object and one spelling-error-in-description.

The MIR description says strongswan depends upon libldns-dev and
libunbound-dev. I've not reviewed these packages -- DNS subject-area
experts told me that these packages are high-quality if highly opinionated
about "correctness" over "compatability". Security team ACK for promoting
these packages to main if necessary.

Security team ACK for promoting strongswan to main.

We would like to demote ipsec-tools to universe to reduce potential
support burden.

Thanks