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.
O.
On 09/07/2012 10:56 AM, 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.
O.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
ACK.
On Fri, Sep 07, 2012 at 01:24:49PM +0200, Michal Židek wrote:
On 09/07/2012 10:56 AM, 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.
O.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
ACK.
Pushed to master.
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.
Simo.
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); } }
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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Michal Židek -
Ondrej Kos -
Simo Sorce