Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Jan 2022 23:28:04 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Ariadne Conill <ariadne@dereferenced.org>
CC:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Eric Biederman <ebiederm@xmission.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH] fs/exec: require argv[0] presence in do_execveat_common()
User-Agent: K-9 Mail for Android
In-Reply-To: <202201252241.7309AE568F@keescook>
References: <20220126043947.10058-1-ariadne@dereferenced.org> <202201252241.7309AE568F@keescook>
Message-ID: <39480927-B17F-4573-B335-7FCFD81AB997@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On January 25, 2022 10:42:41 PM PST, Kees Cook <keescook@chromium=2Eorg> w=
rote:
>On Wed, Jan 26, 2022 at 04:39:47AM +0000, Ariadne Conill wrote:
>> The first argument to argv when used with execv family of calls is
>> required to be the name of the program being executed, per POSIX=2E
>>=20
>> By validating this in do_execveat_common(), we can prevent execution
>> of shellcode which invokes execv(2) family syscalls with argc < 1,
>> a scenario which is disallowed by POSIX, thus providing a mitigation
>> against CVE-2021-4034 and similar bugs in the future=2E
>>=20
>> The use of -EFAULT for this case is similar to other systems, such
>> as FreeBSD and OpenBSD=2E
>>=20
>> Interestingly, Michael Kerrisk opened an issue about this in 2008,

For v2 please include a URL for this=2E I assume you mean this one?
https://bugzilla=2Ekernel=2Eorg/show_bug=2Ecgi?id=3D8408

>> but there was no consensus to support fixing this issue then=2E
>> Hopefully now that CVE-2021-4034 shows practical exploitative use
>> of this bug in a shellcode, we can reconsider=2E
>>=20
>> Signed-off-by: Ariadne Conill <ariadne@dereferenced=2Eorg>
>
>Yup=2E Agreed=2E For context:
>https://www=2Equalys=2Ecom/2022/01/25/cve-2021-4034/pwnkit=2Etxt
>
>> ---
>>  fs/exec=2Ec | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/fs/exec=2Ec b/fs/exec=2Ec
>> index 79f2c9483302=2E=2Ede0b832473ed 100644
>> --- a/fs/exec=2Ec
>> +++ b/fs/exec=2Ec
>> @@ -1897,8 +1897,10 @@ static int do_execveat_common(int fd, struct fil=
ename *filename,
>>  	}
>> =20
>>  	retval =3D count(argv, MAX_ARG_STRINGS);
>> -	if (retval < 0)
>> +	if (retval < 1) {
>> +		retval =3D -EFAULT;
>>  		goto out_free;
>> +	}

Actually, no, this needs to be more carefully special-cased to avoid maski=
ng error returns from count()=2E (e=2Eg=2E -E2BIG would vanish with this pa=
tch=2E)

Perhaps just add:

if (retval =3D=3D 0) {
        retval =3D -EFAULT;
        goto out_free;
}

>
>There shouldn't be anything legitimate actually doing this in userspace=
=2E

I spoke too soon=2E

Unfortunately, this is not the case:
https://codesearch=2Edebian=2Enet/search?q=3Dexecve%5C+*%5C%28%5B%5E%2C%5D=
%2B%2C+*NULL&literal=3D0

Lots of stuff likes to do:
execve(path, NULL, NULL);

Do these things depend on argc=3D=3D0 would be my next question=2E=2E=2E

>
>-Kees
>
>>  	bprm->argc =3D retval;
>> =20
>>  	retval =3D count(envp, MAX_ARG_STRINGS);
>> --=20
>> 2=2E34=2E1
>>=20
>

--=20
Kees Cook