| Message ID | 20210822085522.3416-1-Simon.THOBY@viveris.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: reject unknown hash algorithms in ima_get_hash_algo | expand |
On 8/22/21 10:55 AM, THOBY Simon wrote: > The new function validate_hash_algo() assumed that ima_get_hash_algo() > always return a valid 'enum hash_algo', but it returned the > user-supplied value present in the digital signature without > any bounds checks. > > Update ima_get_hash_algo() to always return a valid hash algorithm, > defaulting on 'ima_hash_algo' when the user-supplied value inside > the xattr is invalid. > > Signed-off-by: THOBY Simon <Simon.THOBY@viveris.fr> > Reported-by: syzbot+e8bafe7b82c739eaf153@syzkaller.appspotmail.com > Fixes: 50f742dd9147 ("IMA: block writes of the security.ima xattr with > unsupported algorithms") > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > security/integrity/ima/ima_appraise.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 8f1eb7ef041e..dbba51583e7c 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -185,7 +185,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, > switch (xattr_value->type) { > case EVM_IMA_XATTR_DIGSIG: > sig = (typeof(sig))xattr_value; > - if (sig->version != 2 || xattr_len <= sizeof(*sig)) > + if (sig->version != 2 || xattr_len <= sizeof(*sig) > + || sig->hash_algo >= HASH_ALGO__LAST) > return ima_hash_algo; > return sig->hash_algo; > break; > Oops, I forgot to update the patch version, and to add a changelog. Here it is: - Updating the commit message - Adding the 'Fixes:' and 'Reviewed-by:' annotations As the patch content didn't change, the comment on syszbot ("This patch was successfully tested by syszbot, see https://syzkaller.appspot.com/bug?extid=e8bafe7b82c739eaf153.") is still true. Sorry about that, Simon
On Sun, 2021-08-22 at 09:00 +0000, THOBY Simon wrote: > On 8/22/21 10:55 AM, THOBY Simon wrote: > > The new function validate_hash_algo() assumed that ima_get_hash_algo() > > always return a valid 'enum hash_algo', but it returned the > > user-supplied value present in the digital signature without > > any bounds checks. > > > > Update ima_get_hash_algo() to always return a valid hash algorithm, > > defaulting on 'ima_hash_algo' when the user-supplied value inside > > the xattr is invalid. > > > > Signed-off-by: THOBY Simon <Simon.THOBY@viveris.fr> > > Reported-by: syzbot+e8bafe7b82c739eaf153@syzkaller.appspotmail.com > > Fixes: 50f742dd9147 ("IMA: block writes of the security.ima xattr with > > unsupported algorithms") > > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > > --- > > security/integrity/ima/ima_appraise.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > > index 8f1eb7ef041e..dbba51583e7c 100644 > > --- a/security/integrity/ima/ima_appraise.c > > +++ b/security/integrity/ima/ima_appraise.c > > @@ -185,7 +185,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, > > switch (xattr_value->type) { > > case EVM_IMA_XATTR_DIGSIG: > > sig = (typeof(sig))xattr_value; > > - if (sig->version != 2 || xattr_len <= sizeof(*sig)) > > + if (sig->version != 2 || xattr_len <= sizeof(*sig) > > + || sig->hash_algo >= HASH_ALGO__LAST) > > return ima_hash_algo; > > return sig->hash_algo; > > break; > > > > Oops, I forgot to update the patch version, and to add a changelog. > Here it is: > - Updating the commit message > - Adding the 'Fixes:' and 'Reviewed-by:' annotations > > As the patch content didn't change, the comment on > syszbot ("This patch was successfully tested by syszbot, see > https://syzkaller.appspot.com/bug?extid=e8bafe7b82c739eaf153.") > is still true. > > Sorry about that, This looks fine. Comments that you want to convey to reviewers/maintainers, like the testing info, go after the patch description after three-dash line. Only the information that should be retained should be in the patch description. thanks, Mimi
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 8f1eb7ef041e..dbba51583e7c 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -185,7 +185,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, switch (xattr_value->type) { case EVM_IMA_XATTR_DIGSIG: sig = (typeof(sig))xattr_value; - if (sig->version != 2 || xattr_len <= sizeof(*sig)) + if (sig->version != 2 || xattr_len <= sizeof(*sig) + || sig->hash_algo >= HASH_ALGO__LAST) return ima_hash_algo; return sig->hash_algo; break;