[SSSD] [PATCH] Out-of-bounds read fix in hmac-sha-1

Simo Sorce simo at redhat.com
Fri Sep 7 14:29:22 UTC 2012


On Fri, 2012-09-07 at 14:25 +0200, Jakub Hrozek wrote:
> On Fri, Sep 07, 2012 at 08:20:29AM -0400, Simo Sorce wrote:
> > On Fri, 2012-09-07 at 10:56 +0200, Ondrej Kos wrote:
> > > fixes https://fedorahosted.org/sssd/ticket/1331
> > > 
> > > Although the given memset wasn't doing anything when *key_len == 
> > > HMAC_SHA1_BLOCKSIZE* (length of bytes to set was 0), the first argument 
> > > to memset was an address starting right after the key itself - 
> > > out-of-bounds.
> > 
> > I haven't looked at the rest of the code, but a more defensive way of
> > doing the test is checking key_len < HMAC_SHA1_BLOCKSIZE, and not just
> > checking it is different.
> > 
> 
> That branch is only ever reachable if key_len < HMAC_SHA1_BLOCKSIZE. The
> whole logic looks like:
> 
> if (key_len > HMAC_SHA1_BLOCKSIZE) {
>     /* shorten the key */
> } else {
>     /* this branch is reachable only if key_len <= HMAC_SHA1_BLOCKSIZE
>      */
>     if (key_len != HMAC_SHA1_BLOCKSIZE) {
>         memset(ikey + key_len, 0, HMAC_SHA1_BLOCKSIZE - key_len);
>     }
> }

Yes I suspected that and that's why I didn;t call for a NACK, and said
"more defensive". The point is that if later someone 'fixes' the code
the flow might change, therefore having a stricter check may avoid pain
later (in this case pain may even end up being a security issue).

I am not saying this must be changed now, just point out I'd like people
to think a bit more defensively when writing this kind of code. Buffer
overruns are never fun (in this case it is memsetting to 0 so it is not
a big deal, it's not like taking random unchecked user input and
memcopying it into random memory places, however you may never be too
paranoid, 'clearing' bits sometimes can cause 'interesting' behaviors
too, like changing "bool failed_auth" from true to false :-)

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list