| Message ID | 20190528153226.248785-1-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | apparmor: enforce nullbyte at end of tag string | expand |
On 5/28/19 8:32 AM, Jann Horn wrote: > A packed AppArmor policy contains null-terminated tag strings that are read > by unpack_nameX(). However, unpack_nameX() uses string functions on them > without ensuring that they are actually null-terminated, potentially > leading to out-of-bounds accesses. > > Make sure that the tag string is null-terminated before passing it to > strcmp(). > > Cc: stable@vger.kernel.org > Signed-off-by: Jann Horn <jannh@google.com> gah! yes! Acked-by: John Johansen <john.johansen@canonical.com> > --- > Warning: The existence of this bug has not been verified at runtime, and > the patch is compile-tested only. I noticed this while browsing through > the code, but didn't want to spend the time necessary to figure out how > to actually test this at runtime. > > > security/apparmor/policy_unpack.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c > index f6c2bcb2ab14..33041c4fb69f 100644 > --- a/security/apparmor/policy_unpack.c > +++ b/security/apparmor/policy_unpack.c > @@ -276,7 +276,7 @@ static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name) > char *tag = NULL; > size_t size = unpack_u16_chunk(e, &tag); > /* if a name is specified it must match. otherwise skip tag */ > - if (name && (!size || strcmp(name, tag))) > + if (name && (!size || tag[size-1] != '\0' || strcmp(name, tag))) > goto fail; > } else if (name) { > /* if a name is specified and there is no name tag fail */ >
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index f6c2bcb2ab14..33041c4fb69f 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -276,7 +276,7 @@ static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name) char *tag = NULL; size_t size = unpack_u16_chunk(e, &tag); /* if a name is specified it must match. otherwise skip tag */ - if (name && (!size || strcmp(name, tag))) + if (name && (!size || tag[size-1] != '\0' || strcmp(name, tag))) goto fail; } else if (name) { /* if a name is specified and there is no name tag fail */
A packed AppArmor policy contains null-terminated tag strings that are read by unpack_nameX(). However, unpack_nameX() uses string functions on them without ensuring that they are actually null-terminated, potentially leading to out-of-bounds accesses. Make sure that the tag string is null-terminated before passing it to strcmp(). Cc: stable@vger.kernel.org Signed-off-by: Jann Horn <jannh@google.com> --- Warning: The existence of this bug has not been verified at runtime, and the patch is compile-tested only. I noticed this while browsing through the code, but didn't want to spend the time necessary to figure out how to actually test this at runtime. security/apparmor/policy_unpack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)