On Fri, Dec 1, 2017 at 5:21 PM, William Roberts
<bill.c.roberts@xxxxxxxxx> wrote:
> On Fri, Dec 1, 2017 at 1:31 PM, Paul Moore <pmoore@xxxxxxxxxx> wrote:
>> From: Paul Moore <paul@xxxxxxxxxxxxxx>
>>
>> The syzbot/syzkaller automated tests found a problem in
>> security_context_to_sid_core() during early boot (before we load the
>> SELinux policy) where we could potentially feed context strings without
>> NUL terminators into the strcmp() function.
>>
>> We already guard against this during normal operation (after the SELinux
>> policy has been loaded) by making a copy of the context strings and
>> explicitly adding a NUL terminator to the end. The patch extends this
>> protection to the early boot case (no loaded policy) by moving the context
>> copy earlier in security_context_to_sid_core().
>>
>> Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
>> ---
>> security/selinux/ss/services.c | 18 ++++++++----------
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>> index 33cfe5d3d6cb..d05496deb229 100644
>> --- a/security/selinux/ss/services.c
>> +++ b/security/selinux/ss/services.c
>> @@ -1413,27 +1413,25 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
>> if (!scontext_len)
>> return -EINVAL;
>>
>> + /* Copy the string to allow changes and ensure a NUL terminator */
>> + scontext2 = kmemdup_nul(scontext, scontext_len, gfp_flags);
>> + if (!scontext2)
>> + return -ENOMEM;
>> +
>> if (!ss_initialized) {
>> int i;
>>
>> for (i = 1; i < SECINITSID_NUM; i++) {
>> - if (!strcmp(initial_sid_to_string[i], scontext)) {
>> + if (!strcmp(initial_sid_to_string[i], scontext2)) {
>> *sid = i;
>> - return 0;
>> + goto out;
>> }
>> }
>> *sid = SECINITSID_KERNEL;
>> - return 0;
>> + goto out;
>> }
>> *sid = SECSID_NULL;
>>
>> - /* Copy the string so that we can modify the copy as we parse it. */
>> - scontext2 = kmalloc(scontext_len + 1, gfp_flags);
>> - if (!scontext2)
>> - return -ENOMEM;
>> - memcpy(scontext2, scontext, scontext_len);
>> - scontext2[scontext_len] = 0;
>> -
>> if (force) {
>> /* Save another copy for storing in uninterpreted form */
>> rc = -ENOMEM;
>>
>>
>
> I like negative diffstats.
>
> <Reviewed-By William Roberts william.c.roberts@xxxxxxxxx>
So do I, it's one of the few pleasures I have in this job.
Merged into selinux/next, thanks for the review.
--
paul moore
www.paul-moore.com
