Hide Forgot
Created attachment 815082 [details] Proposed fix. Description of problem: This code attempts to store a byte with user name length: freeipmi-1.3.2/libfreeipmi/util/ipmi-rmcpplus-util.c memcpy (hash_data + hash_data_len, (void *)&user_name_len, sizeof (uint8_t)); hash_data_len += sizeof (uint8_t); It works on little-endian, but not on big-endian machine. The easy fix is to replace that monstrosity with: hash_data[hash_data_len++] = user_name_len; The buggy code is the same in freeipmi-1.3.2 and freeipmi-1.2.1. I noticed another bug, but it's a relatively harmless one (failure to memset to 0 one of the auth buffers after work).
BTW, there are tons of secure_memset (foo_array, '\0', IPMI_FOO_ARRAY_SIZE); lines which just beg to be cleaned up using sizeof(foo_array) for the size parameter, so that a typo doesn't turn into a disaster. Better yet, create and use a zero_out_array(foo) which hides '\0' and sizeof() and also checks that foo is an array, not a pointer (using sizeof would be a hidden bug if foo is a ptr).
From kernel source: # define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) /* &a[0] degrades to a pointer: a different type from an array */ #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
(In reply to Denys Vlasenko from comment #0) > This code attempts to store a byte with user name length: > > freeipmi-1.3.2/libfreeipmi/util/ipmi-rmcpplus-util.c > > memcpy (hash_data + hash_data_len, > (void *)&user_name_len, > sizeof (uint8_t)); > hash_data_len += sizeof (uint8_t); > > It works on little-endian, but not on big-endian machine. > > The easy fix is to replace that monstrosity with: > > hash_data[hash_data_len++] = user_name_len; Forgot to mention that I tested this fix and it works.
(In reply to Denys Vlasenko from comment #0) > hash_data[hash_data_len++] = user_name_len; Patch applied. Will be in the FreeIPMI 1.3.3 release. I didn't apply the second half of the patch w/ the memset cleanup. Acknowledged that the memsets should be cleaned up globally, but would like to do a huge fix/audit everywhere later. > also checks that foo is an array, not a pointer I don't believe the macro you posted is portable to non-gcc compilers? > I noticed another bug, > but it's a relatively harmless one (failure to memset to 0 one of the auth > buffers after work). Can you tell me the location? I wouldn't be surprised if it was missed somewhere.
(In reply to Albert Chu from comment #6) > > I noticed another bug, > > but it's a relatively harmless one (failure to memset to 0 one of the auth > > buffers after work). > > Can you tell me the location? I wouldn't be surprised if it was missed > somewhere. Here: secure_memset (buf, '\0', IPMI_MAX_KEY_DATA_LENGTH); secure_memset (buf, '\0', IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH); the second memset operates on the wrong array of chars, it should be: secure_memset (digest, '\0', IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH);
(In reply to Denys Vlasenko from comment #11) > Here: > > secure_memset (buf, '\0', IPMI_MAX_KEY_DATA_LENGTH); > secure_memset (buf, '\0', > IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH); > > the second memset operates on the wrong array of chars, it should be: > > secure_memset (digest, '\0', > IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH); Doh! Thanks for the catch.
(In reply to Albert Chu from comment #6) > > also checks that foo is an array, not a pointer > > I don't believe the macro you posted is portable to non-gcc compilers? Array-ness check can be made conditional on #ifdef __GNUC__