Bug 1022178 - Endian bug in authentication code
Endian bug in authentication code
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: freeipmi (Show other bugs)
6.5
Unspecified Unspecified
high Severity high
: rc
: ---
Assigned To: Ales Ledvinka
qe-baseos-daemons
: ZStream
Depends On:
Blocks: 993793 1005265 1032963 1189065
  Show dependency treegraph
 
Reported: 2013-10-22 12:05 EDT by Denys Vlasenko
Modified: 2016-09-20 00:33 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 1189065 (view as bug list)
Environment:
Last Closed: 2014-01-09 09:52:04 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Proposed fix. (1.25 KB, patch)
2013-10-22 12:05 EDT, Denys Vlasenko
no flags Details | Diff

  None (edit)
Description Denys Vlasenko 2013-10-22 12:05:32 EDT
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).
Comment 1 Denys Vlasenko 2013-10-22 12:10:11 EDT
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).
Comment 2 Denys Vlasenko 2013-10-22 12:18:14 EDT
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]))
Comment 5 Denys Vlasenko 2013-10-23 08:56:42 EDT
(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.
Comment 6 Albert Chu 2013-10-23 14:22:11 EDT
(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.
Comment 11 Denys Vlasenko 2013-10-30 10:54:53 EDT
(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);
Comment 12 Albert Chu 2013-10-30 13:08:06 EDT
(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.
Comment 13 Denys Vlasenko 2013-11-01 05:36:33 EDT
(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__

Note You need to log in before you can comment on or make changes to this bug.