Note: This bug is displayed in read-only format because
the product is no longer active in Red Hat Bugzilla.
RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
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__
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).