Auto selection of 'best' cipher suite available #32

vmauery · 2018-08-13T20:32:48Z

Based on current crypto alogrithms, one could rank cipher suites along
these lines:

17 > 3 >> all the rest

17 and 3 are the only cipher suites that implement any sort of
confidentiality alogorithm that is secure. In addition, any hmac-md5 or
md5 integrity algorithm used in integrity is weak at best and dangerous
for authentication.

This could possibly be enabled in a simpler mechanism by simply checking
for 17 and then choosing it before falling back to 3, but the way this
is implemented, it makes it easy to change the list of acceptable
algorithms from two to three or more items.

Signed-off-by: Vernon Mauery vernon.mauery@intel.com

AlexanderAmelkin · 2018-08-14T10:21:22Z

lib/ipmi_channel.c

 	memset(cipher_suite_data, 0, sizeof(cipher_suite_data));
+	memset(suites, 0, sizeof(struct cipher_suite_info)*n_max);


spaces around operator *, please.
also, I'd use sizeof(*suites) to make it independent of the actual type.

AlexanderAmelkin · 2018-08-14T10:25:18Z

include/ipmitool/ipmi_channel.h

@@ -131,6 +131,8 @@ struct get_channel_auth_cap_rsp {
 int _ipmi_get_channel_access(struct ipmi_intf *intf,
 		struct channel_access_t *channel_access,
 		uint8_t get_volatile_settings);
+int _ipmi_get_channel_cipher_suites(struct ipmi_intf *intf, const char *payload_type,
+		uint8_t channel, struct cipher_suite_info *suites, size_t *count);


I honestly hate these underscore-prefixed functions in the header. Underscored functions are something private/static and should not be exported. I know this isn't your invention here, but I'd prefer you avoid adding new ones.

AlexanderAmelkin · 2018-08-14T10:31:10Z

include/ipmitool/ipmi_intf.h

+	uint8_t crypt_alg;
+	uint32_t iana;
+};
+


Looks a lot like these should be put under #if defined(IPMI_INTF_LAN) || defined(IPMI_INTF_LANPLUS).

We cannot ifdef this out as only part of lan/lanplus enabled because this construct is used for two cases: determining the best cipher suite for a lanplus session; and as part of the "channel getciphers" call, which can be called over KCS.

AlexanderAmelkin · 2018-08-14T10:32:27Z

lib/ipmi_channel.c

-		uint8_t channel)
+int
+_ipmi_get_channel_cipher_suites(struct ipmi_intf *intf, const char *payload_type,
+		uint8_t channel, struct cipher_suite_info *suites, size_t *count)


This brings warning: ‘struct cipher_suite_info’ declared inside parameter list. Apparently, you forgot to include ipmi_intf.h.

Besides, CodeClimate complains that this function is too complex. The complexity factor raised from 24 (paths through the function) to 28 with your change. Please try to reduce it. With a separate follow-up commit within this pull request, please.

AlexanderAmelkin · 2018-08-14T10:37:45Z

lib/ipmi_channel.c

+}
+
+static int
+ipmi_get_channel_cipher_suites(struct ipmi_intf *intf, const char *payload_type,


I think this should be called ipmi_print_channel_cipher_suites(), and the other one (with underscore) should have this name.

AlexanderAmelkin · 2018-08-14T14:29:23Z

src/plugins/lanplus/lanplus.c

+		return IPMI_LANPLUS_CIPHER_SUITE_3;
+	}
+	for (ipref = 0; ipref < nr_preferred &&
+			IPMI_LANPLUS_CIPHER_SUITE_RESERVED != best_suite; ipref++) {


This won't run. IPMI_LANPLUS_CIPHER_SUITE_RESERVED != best_suite is false right away. You probably meant == here.

Besides, same comment applies as above regarding alignment and curly.

AlexanderAmelkin · 2018-08-14T14:59:54Z

src/plugins/lanplus/lanplus.c

+	struct cipher_suite_info suites[256];
+	size_t nr_suites = sizeof(suites)/sizeof(suites[0]);
+	/* cipher suite best order is chosen with this criteria:
+	 * hmac-md5 and md5 are BAD; xc4 is bad; aes128 is required


xRC4, I suppose, not xc4 ?
Anyway, I don't see anywhere in the following code that you check/compare algorithms.
You only have two items in cipher_order_preferred, only one of them being different from the default.
So basically, your function returns either 17 or 3, regardless of any other suites supported, and the comment does not help to understand why (without reading the IPMI spec section 22.15.2). Please add an explanation that applying these criteria to the aforementioned section, and taking in account that suite id 3 is mandatory, only id 17 is left that is better than 3.

This even boils down to a much simpler code that only checks if id 17 is supported, no need for all those nested loops, if I understood your logic right.

Yes, it would be possible to just check for 17. But I wanted to make it so an arbitrary priority list could be substituted if other people had different opinions about which cipher suites were important to them.

AlexanderAmelkin · 2018-08-14T15:00:21Z

src/plugins/lanplus/lanplus.c

+			IPMI_LANPLUS_CIPHER_SUITE_RESERVED != best_suite; ipref++) {
+		for (i = 0; i < nr_suites; i++) {
+			if (cipher_order_preferred[ipref] ==
+					suites[i].cipher_suite_id) {


And here too. Alignment and curly.

AlexanderAmelkin · 2018-08-14T15:00:49Z

src/plugins/lanplus/lanplus.c

+	 * pick the best one available
+	 */
+	if (IPMI_LANPLUS_CIPHER_SUITE_RESERVED ==
+			intf->ssn_params.cipher_suite_id) {


alignment and curly

AlexanderAmelkin · 2018-08-14T15:11:06Z

src/plugins/lanplus/lanplus.h

+#define IPMI_LANPLUS_CIPHER_SUITE_14 14
+#define IPMI_LANPLUS_CIPHER_SUITE_15 15
+#define IPMI_LANPLUS_CIPHER_SUITE_16 16
+#define IPMI_LANPLUS_CIPHER_SUITE_17 17


make these an enum type instead, please. enums are symbols that can be seen/resolved during gdb session, unlike macros. change the associated variables/struct fields to that type too, please.

AlexanderAmelkin · 2018-08-15T09:37:40Z

include/ipmitool/ipmi_intf.h

+	uint8_t crypt_alg;
+	uint32_t iana;
+};
+


AlexanderAmelkin · 2018-08-15T09:41:45Z

lib/ipmi_channel.c


+	do {
+		/* Always ask for cipher suite format */
+		rqdata[2] =  0x80 + list_index;


What is 0x80 ?

AlexanderAmelkin · 2018-08-15T09:44:02Z

lib/ipmi_channel.c

 		rsp = intf->sendrecv(intf, &req);
 		if (rsp == NULL) {
 			lprintf(LOG_ERR, "Unable to Get Channel Cipher Suites");
 			return -1;
 		}
-		if (rsp->ccode > 0) {
+		if (rsp->ccode > 0 || rsp->data_len < 1) {


ccode is uint8_t, it's quite safe to do just if (rsp->ccode) (I mean, omit the > 0 part).

AlexanderAmelkin · 2018-08-15T09:44:40Z

lib/ipmi_channel.c

+		 * Increment our list for the next call
+		 */
+		++list_index;
+	} while ((rsp->data_len == 17) &&


What is 17 ? Please don't use magic numbers.

AlexanderAmelkin · 2018-10-31T13:17:34Z

lib/ipmi_channel.c

-			offset += 3;
+				record->iana[0]        |
+				(record->iana[1] << 8) |
+				(record->iana[2] << 16);


There is an ipmi24toh() for this now. Please use it.

vmauery · 2018-10-31T19:33:57Z

I think I have addressed all the concerns you have raised.

Based on current crypto alogrithms, one could rank cipher suites along these lines: 17 > 3 >> all the rest 17 and 3 are the only cipher suites that implement any sort of confidentiality alogorithm that is secure. In addition, any hmac-md5 or md5 integrity algorithm used in integrity is weak at best and dangerous for authentication. This could possibly be enabled in a simpler mechanism by simply checking for 17 and then choosing it before falling back to 3, but the way this is implemented, it makes it easy to change the list of acceptable algorithms from two to three or more items. Resolves ipmitool#29 Signed-off-by: Vernon Mauery <vernon.mauery@intel.com>

Cipher suite ID is a byte as per IPMI spec. Use the appropriate function and consider target endianness (so write to a temporary variable). Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>

Some minor formatting corrections. Also introduced a new helper function to reduce nesting level. Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>

AlexanderAmelkin suggested changes Aug 14, 2018

View reviewed changes

vmauery force-pushed the improvement/29 branch from 55bcc9c to 5ba8ae0 Compare August 14, 2018 22:28

vmauery force-pushed the improvement/29 branch from 5ba8ae0 to 85e56ef Compare September 6, 2018 16:08

AlexanderAmelkin suggested changes Oct 31, 2018

View reviewed changes

vmauery force-pushed the improvement/29 branch 2 times, most recently from 202d973 to 35e5ab8 Compare October 31, 2018 19:30

AlexanderAmelkin previously approved these changes Nov 1, 2018

View reviewed changes

vmauery and others added 3 commits November 1, 2018 18:13

lanplus: Fix -C option processing

4599317

Cipher suite ID is a byte as per IPMI spec. Use the appropriate function and consider target endianness (so write to a temporary variable). Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>

lanplus: Refactoring

93450bb

Some minor formatting corrections. Also introduced a new helper function to reduce nesting level. Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>

AlexanderAmelkin approved these changes Nov 1, 2018

View reviewed changes

AlexanderAmelkin merged commit 65a2c54 into ipmitool:master Nov 1, 2018

AlexanderAmelkin mentioned this pull request Jun 18, 2021

Unable to establish IPMI connection to Dell R640 #150

Closed

linuxtek-canada mentioned this pull request Nov 4, 2022

IPMI commands failing on Cisco UCS servers after upgrade to 1.8.19 #376

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto selection of 'best' cipher suite available #32

Auto selection of 'best' cipher suite available #32

vmauery commented Aug 13, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 14, 2018 •

edited

Loading

AlexanderAmelkin Aug 14, 2018

vmauery Aug 14, 2018

AlexanderAmelkin Aug 15, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 14, 2018

vmauery Oct 31, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 14, 2018

AlexanderAmelkin Aug 15, 2018

AlexanderAmelkin Aug 15, 2018

AlexanderAmelkin Aug 15, 2018

AlexanderAmelkin Aug 15, 2018

AlexanderAmelkin Oct 31, 2018

vmauery commented Oct 31, 2018

		memset(cipher_suite_data, 0, sizeof(cipher_suite_data));
		memset(suites, 0, sizeof(struct cipher_suite_info)*n_max);

Auto selection of 'best' cipher suite available #32

Auto selection of 'best' cipher suite available #32

Conversation

vmauery commented Aug 13, 2018

Choose a reason for hiding this comment

AlexanderAmelkin Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmauery commented Oct 31, 2018

AlexanderAmelkin Aug 14, 2018 •

edited

Loading