On 06/05/16 at 07:49pm, Tobias Stoeckmann wrote:
> Signature files larger than INT_MAX are already suspicious,
> but if they are larger than SIZE_MAX, this code couldn't even
> copy them into memory, accepting them as "blank" files at worst.
>
> While adding the INT_MAX check, I also rearranged the code to
> avoid a quite harmless TOCTOU race condition between
> stat() and fopen().
>
> Signed-off-by: Tobias Stoeckmann <[email protected]>
> ---
> Thanks for pointing out the flaw Florian!
> ---
> lib/libalpm/be_package.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
> index c9ed770..055fb1e 100644
> --- a/lib/libalpm/be_package.c
> +++ b/lib/libalpm/be_package.c
> @@ -700,17 +700,17 @@ static int read_sigfile(const char *sigpath, unsigned
> char **sig)
> struct stat st;
> FILE *fp;
>
> - if(stat(sigpath, &st) != 0) {
> + if((fp = fopen(sigpath, "rb")) == NULL) {
> return -1;
> }
>
> - MALLOC(*sig, st.st_size, return -1);
> -
> - if((fp = fopen(sigpath, "rb")) == NULL) {
> - free(*sig);
> + if(fstat(fileno(fp), &st) != 0 || st.st_size > INT_MAX) {
limits.h should be included for INT_MAX. Is there not a more
meaningful limit we can use for this than INT_MAX?
> + fclose(fp);
> return -1;
> }
>
> + MALLOC(*sig, st.st_size, return -1);
Needs fclose(fp)
> +
> if(fread(*sig, st.st_size, 1, fp) != 1) {
> free(*sig);
> fclose(fp);
> --
> 2.8.3
