(usagi-users 00805) problem with pfkey for IPSEC and crypto algorithms

[latten@xxxxxxxxxxxxxx]

Hi,

I have installed the September 3 snapshot of the USAGI kit.
I also configured the IPSEC portion and ran into a problem. I had
forgotten to define the CRYPTO modules for the kernel so I 
didn't have any cryto algorithms available to me. I configured
with "pfkey" as demonstrated in the README.USAGI-IPSEC file.
My information was added to the SADB although the auth and
encryption algorithms could not be found. So when it came time 
to send data, because I had an SA, the data would be sent, it was
just never encrypted or authenticated since the algorithms didn't exist.
I thought this might be incorrect behaviour. It gave the illusion 
that security had occured and all went well, although it hadn't. 
I did see complaints from kern.log in syslog. 

I didn't think this might be right behaviour. I thought either
the packet should not have been sent or the SA should not have
been added if the crypto algorithms were missing. 

I checked the code, and found that the return value from 
some routines were not being saved and thus were always
believed to have worked and thus the SAs were always being
added even if something had failed while processing them.

I could fix this two ways. First, if the crypto algorithm does not
exist, we do not want to add the SA to the SADB. 
	OR
If the crypto algorithm did not exist, do not add it to the SA,
but still add the SA to the SADB. 

I made the assumption that if the crypto algorithm did not exist,
then the SA should not be added to the SADB. Thus two files
needed to be changed. If you choose the second way, then only
the first file needs to be changed. 

1.
pfkey_msg_interp() needs to check for errors from the sadbXXX functions
because otherwise, the variable "error" will always be 0. And 
pfkey_sendmsg() who calls pfkey_msg_interp() and checks the return
code will always believe no errors occured and not report them.

 
--- pfkey_v2.c.orig	Tue Sep 18 15:52:22 2001
+++ pfkey_v2.c	Tue Sep 18 15:54:00 2001
@@ -107,29 +107,29 @@
 		break;
 	case SADB_ADD:
 		printk(KERN_INFO "PF_KEY:interp: SADB_ADD\n");
-		sadb_msg_add_parse(sk, pfkey_msg, pfkey_reply);
+		error = sadb_msg_add_parse(sk, pfkey_msg, pfkey_reply);
 		break;
 	case SADB_DELETE:
-		sadb_msg_delete_parse(sk, pfkey_msg, pfkey_reply);
+	        error = sadb_msg_delete_parse(sk, pfkey_msg, pfkey_reply);
 		printk(KERN_INFO "PF_KEY:interp: SADB_DELETE\n");
 		break;
 	case SADB_FLUSH:
-		sadb_msg_flush_parse(sk, pfkey_msg, pfkey_reply);
+		error = sadb_msg_flush_parse(sk, pfkey_msg, pfkey_reply);
 		printk(KERN_INFO "PF_KEY:interp: SADB_FLUSH\n");
 		break;
 	case SADB_REGISTER:
-		sadb_msg_register_parse(sk, pfkey_msg, pfkey_reply);
+		error = sadb_msg_register_parse(sk, pfkey_msg, pfkey_reply);
 		printk(KERN_INFO "PF_KEY:interp: SADB_REGISTER");
 		break;
 	case SADB_X_GRPSA:
 		printk(KERN_INFO "PF_KEY:interp: SADB_X_GRPSA\n");
 		break;
 	case SADB_X_ADDFLOW:
-		sadb_msg_addflow_parse(sk, pfkey_msg, pfkey_reply);
+		error = sadb_msg_addflow_parse(sk, pfkey_msg, pfkey_reply);
 		printk(KERN_INFO "PF_KEY:interp: SADB_ADDFLOW\n");
 		break;
 	case SADB_X_DELFLOW:
-		sadb_msg_delflow_parse(sk, pfkey_msg, pfkey_reply);
+		error = sadb_msg_delflow_parse(sk, pfkey_msg, pfkey_reply);
 		printk(KERN_INFO "PF_KEY:interp: SADB_DELFLOW\n");
 		break;
 	default:




2.

Adding a goto in sadb_msg_add_parse() if an error occurs prevents sadb_append()
from being called and adding the SA.

--- pfkey_v2_msg_add.c.orig	Tue Sep 18 16:30:23 2001
+++ pfkey_v2_msg_add.c	Tue Sep 18 17:36:58 2001
@@ -154,8 +154,10 @@
 
 		if ( sa->sadb_sa_auth ) {
 			if( (ext_msgs[SADB_EXT_KEY_AUTH]) ) {
-				error = sadb_key_to_auth(sa->sadb_sa_auth,
-					(struct sadb_key*) ext_msgs[SADB_EXT_KEY_AUTH], &sa_entry);
+				if (error = sadb_key_to_auth(sa->sadb_sa_auth,
+					(struct sadb_key*) ext_msgs[SADB_EXT_KEY_AUTH], &sa_entry)) {
+					goto err;
+		    		}
 			} else {
 				pr_debug("sadb_msg_add_parse: sa has auth"
 					" algo but there is no key for auth\n");
@@ -166,8 +168,10 @@
 
 		if ( sa->sadb_sa_encrypt ) {
 			if( (ext_msgs[SADB_EXT_KEY_ENCRYPT]) ) {
-				error = sadb_key_to_esp(sa->sadb_sa_encrypt, 
-					(struct sadb_key*) ext_msgs[SADB_EXT_KEY_ENCRYPT], &sa_entry);
+				if (error = sadb_key_to_esp(sa->sadb_sa_encrypt,
+					(struct sadb_key*) ext_msgs[SADB_EXT_KEY_ENCRYPT], &sa_entry)) {
+				goto err;
+				}
 			} else {
 				pr_debug("sadb_msg_add_parse: sa has esp"
 					" algo but there is no key for esp\n");



3.
Once I made the above fix, I noticed that sadb_msg_addflow_parse()
ALWAYS reported an error even when I had successfully added
my policy to the SPD.
This is why:

sadb_msg_addflow_parse() calls spd_find_by_selector() when adding a
policy to the SPD. spd_find_by_selector() returns -EEXIST or -ESRCH in
the variable "error" which tells sadb_msg_addflow_parse() to create a 
new policy or an index into an already existing one. However, afterwards
the variable "error" was not being cleared. Thus sadb_msg_addflow_parse()
was always reporting an error although none had really occured. 
I clear this variable by using it as the return value on proceeding 
functions that could report errors or success. 


--- pfkey_v2_msg_flow.c.orig	Wed Sep 19 15:07:02 2001
+++ pfkey_v2_msg_flow.c	Fri Sep 21 15:01:08 2001
@@ -152,16 +152,22 @@
 				goto err;
 			}
 			memcpy(&(policy->selector), &selector, sizeof(struct selector));
-			sa_list_append(&(policy->sa_index_list), &sa_idx);
+			if (error = sa_list_append(&(policy->sa_index_list), &sa_idx)) {
+				goto err;
+			}	
 			dump_ipsec_sp(policy);
-			spd_append(policy);
-			ipsec_sp_kfree(&policy);
+			if (error = spd_append(policy)) {
+				goto err;
+			}
+			error = ipsec_sp_kfree(&policy);
 
 		} else if (error == -EEXIST) {
 
 			/* It has already been in spd_list, I append sa_index into it's sa_list */
 			pr_debug("pfkey_msg_addflow_parse: policy=%p\n", policy);
-			sa_list_append(&(policy->sa_index_list), &sa_idx);
+			if (error = sa_list_append(&(policy->sa_index_list), &sa_idx)) {
+				goto err;
+			}
 			dump_ipsec_sp(policy);
 
 		} else {


After making these changes to the above 3 files, the SA is not added if the
crypto alogrithms are not there and the error is reported. Is this correct? 
Shouldn't the addition of the SA fail if the crypto algorithms are missing??


Thanks,
Joy