#1331 Off-by-one error in sss_hmac_sha1
Closed: Fixed None Opened 6 years ago by jhrozek.

ikey is defined as unsigned char ikey[HMAC_SHA1_BLOCKSIZE]. If key_len is exactly HMAC_SHA1_BLOCKSIZE, we would access the item with index HMAC_SHA1_BLOCKSIZE. I actually tested this and memset survives fine, most probably because the number of items to set is 0. We can't rely on the behaviour and should fix this bug.

At conditional (3): "key_len > 64UL" taking the false branch.
57    if (key_len > HMAC_SHA1_BLOCKSIZE) {
58        /* keys longer than blocksize are shortened */
59        HASH_Begin(sha1);
60        HASH_Update(sha1, key, key_len);
61        HASH_End(sha1, ikey, &res_len, SSS_SHA1_LENGTH);
62        memset(ikey + SSS_SHA1_LENGTH, 0, HMAC_SHA1_BLOCKSIZE - SSS_SHA1_LENGTH);
63    } else {
64        /* keys shorter than blocksize are zero-padded */
65        memcpy(ikey, key, key_len);
CID 12623: Out-of-bounds read (OVERRUN_STATIC)Overrunning static array of size 64 bytes at byte position 64 by accessing with pointer "&ikey[key_len]" through dereference in call to "memset". (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)
Note: These bugs are often difficult to see at first glance. Coverity recommends a close inspection of the events leading to this overrun.
66        memset(ikey + key_len, 0, HMAC_SHA1_BLOCKSIZE - key_len);
67    }

Fields changed

owner: somebody => jcholast

Fields changed

keywords: => Coverity

Fields changed

keywords: Coverity => Coverity easyfix
milestone: NEEDS_TRIAGE => SSSD 1.9.0
rhbz: => 0

Fields changed

owner: jcholast => okos
patch: 0 => 1
status: new => assigned

master: 7350592

milestone: SSSD 1.9.0 => SSSD 1.9.0 RC1
resolution: => fixed
status: assigned => closed

Metadata Update from @jhrozek:
- Issue assigned to okos
- Issue set to the milestone: SSSD 1.9.0 RC1

2 years ago

Login to comment on this ticket.