linux-hardening - Re: RFC for a new string-copy function, using mixtures of strlcpy and strscpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHP4M8W2H4V=qgAeVp76GwfVUBzR3erZxJiuhm6jnyMo+gvknQ@mail.gmail.com>
Date:   Mon, 8 Nov 2021 18:15:54 +0530
From:   Ajay Garg <ajaygargnsit@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     andy@...nel.org, Kees Cook <keescook@...omium.org>,
        akpm@...ux-foundation.org, adobriyan@...il.com,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-hardening@...r.kernel.org
Subject: Re: RFC for a new string-copy function, using mixtures of strlcpy and strscpy

Hi Greg,

Thanks for your time.

>
> Wait, why?  We have recently added new string copy functions to resolve
> the type of issues you have pointed out.  The kernel is not yet
> converted to use them, so why add yet-another-function that also is not
> used?

Greg, would request your couple of minutes more, in suggesting a
concrete way forward, by working through an example as below.

In the file fs/kernfs/dir.c, there is a statement

        return strlcpy(buf, kn->parent ? kn->name : "/", buflen);

Here, there is no information of how long kn->name might be, so there
is a definite chance of overflow happening. Thus, accordingly, strlcpy
is used, to bound copying of upto buflen bytes. Of course, buf
(destination-buffer) is safe from any overflow now.

However, iffff strlen(kn->name) is greater than (buflen - 1), then
strlcpy would return a different value than the number of bytes
actually copied. Since there is no check, this (wrong) return value
will be propagated to the clients down the stack.

Now, in the current situation, following is the remedy :

########################################
        int ret = strlcpy(buf, kn->parent ? kn->name : "/", buflen);
        if(ret >= buflen)
            ret = buflen - 1;

        return ret;
########################################

Lines changed : 4

On the other hand, with the proposed new function strlscpy, the fix
would simply be :

########################################
       return strlscpy(buf, kn->parent ? kn->name : "/", buflen);
########################################

Lines changed : 1

Personally, as a "generic third-party", I would prefer investing my
time which makes lives of everyone - present and future - easier, so
the RFC for the new method is a step in that direction. This new
method would reduce effort from 4-lines-change to 1-line-change.

>
> I think you should work with the functions we have _first_ and help
> convert the kernel to use them, before adding another one please.
>

I tried looking a similar function in lib/string.c, but could not find any.
If there indeed is already a function that behaves like the new
intended method, kindly point me to it, case would be closed
immediately :D

Once again, many thanks for your time.

Thanks and Regards,
Ajay