[Swan] SHA2 support for ESP in KLIPS?

[D. Hugh Redelmeier]

| From: Paul Wouters <pwouters at redhat.com>

| err_t kernel_alg_esp_auth_ok(int auth,
|                              struct alg_info_esp *alg_info __attribute__(
|                                      (unused)))

| The function (misleadingly) returns NULL on success, and non-NULL on
| failure.

No, that's not misleading.  That's what err_t means: an error message,
or NULL if not an error.  This convention is widely used in Pluto.

| On Tue, 25 Jun 2013, Elison Niven wrote:
| 
| > I never had KERNEL_ALG undefined.
| > This if condition never passed :
| >
| > #ifdef KERNEL_ALG
| >    if (kernel_alg_esp_auth_ok(pi->attrs.auth, NULL)) {
| >
| > So it went to the default case below.
| > Changing it to
| >
| > if (!kernel_alg_esp_auth_ok(pi->attrs.auth, NULL)) {
| >
| > worked and pluto no longer gives the assertion.

An err_t return result should (almost) always be captured so that it
can be reported.  Like:

	err_t ugh = kernel_alg_esp_auth_ok(pi->attrs.auth, NULL);

	if (ugh != NULL) {
		log an error based on ugh
	} else {
		continue with computation
	}

Now I'm looking wider, but still narrowly, at the code where this was 
called:
                 case AUTH_ALGORITHM_HMAC_MD5:
                         needed_len += HMAC_MD5_KEY_LEN;
                         break;
                 case AUTH_ALGORITHM_HMAC_SHA1:
                         needed_len += HMAC_SHA1_KEY_LEN;
                         break;
                 default:
#ifdef KERNEL_ALG
                         if (kernel_alg_esp_auth_ok(pi->attrs.transattrs.
                                                    integ_hash, NULL) == NULL) {
                                 needed_len += kernel_alg_esp_auth_keylen(
                                         pi->attrs.transattrs.integ_hash);
                                 break;
                         }
#endif
                 case AUTH_ALGORITHM_DES_MAC:
                         bad_case(pi->attrs.transattrs.integ_hash);
                         break;

                 }

Th ifdef KERNEL_ALG code handles the success case (notice the ==
NULL).  If KERNEL_ALG isn't defined, or kernel_alg_esp_auth_ok fails,
the code falls through to case AUTH_ALGORITHM_DES_MAC.

At first glance, this looks really fishy: falling through from a
general case to a specific case is usually a mistake.

But it really isn't quite as bad as it seems: the code it falls
into is meant to handle an unexpected case, which surely this is.

But it results in an assertion failure.  So it appears to depend on
some earlier code to prevent this happening.

At this point, I stopped spelunking.

Suggestions:

1: add /* FALL THROUGH */ to make it evident that this was not an
   accident.

2: fix the earlier place which is supposed to deal with the cases
   where kernel_alg_esp_auth_ok fails