linux-kernel - Re: [PATCH][resend] Do not potentially overflow string in sumversion

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] [day] [month] [year] [list]

Date:	Mon, 21 Mar 2011 15:35:27 -0400
From:	Arnaud Lacombe <lacombar@...il.com>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org,
	WANG Cong <xiyou.wangcong@...il.com>,
	James Morris <jmorris@...ercode.com.au>,
	James Morris <jmorris@...ei.org>,
	"David S. Miller" <davem@...hat.com>,
	David Miller <davem@...emloft.net>,
	Steve French <sfrench@...ibm.com>,
	Steve French <smfrench@...il.com>,
	Andrew Tridgell <tridge@...ba.org>
Subject: Re: [PATCH][resend] Do not potentially overflow string in sumversion

Hi,

On Mon, Mar 21, 2011 at 3:17 PM, Jesper Juhl <jj@...osbits.net> wrote:
> In scripts/mod/sumversion.c (in get_src_version()) we call
> getenv("MODVERDIR"). This returns a pointer to a string of unknown length.
> This string of unknown length we then pass on as an argument to sprintf()
> and tell it to write the result to 'filelist' which has a, very much
> fixed, size of 'PATH_MAX + 1'. If the string returned by getenv() is too
> long we'll overrun the statically allocated buffer.
> This patch prevents the buffer overrun by using snprintf() and telling it
> to copy a maximum of 'PATH_MAX + 1' bytes (including the terminating \0).
>
> Signed-off-by: Jesper Juhl <jj@...osbits.net>
> Acked-by: WANG Cong <xiyou.wangcong@...il.com>
> ---
>  sumversion.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>  compile tested only. Patch against Linus' tree as of right now.
>
> diff --git
> a/scripts/mod/sumversion.c
> b/scripts/mod/sumversion.c
> index 9dfcd6d..522c675 100644
> --- a/scripts/mod/sumversion.c
> +++ b/scripts/mod/sumversion.c
> @@ -416,7 +416,7 @@ void get_src_version(const char *modname, char sum[], unsigned sumlen)
>                basename = strrchr(modname, '/') + 1;
>        else
>                basename = modname;
> -       sprintf(filelist, "%s/%.*s.mod", modverdir,
> +       snprintf(filelist, PATH_MAX + 1, "%s/%.*s.mod", modverdir,
>                (int) strlen(basename) - 2, basename);
>
Please, do not use hardcoded size. The `sizeof' operator would be much
more appropriate. Moreover, if you want thing to be done pedantically,
you should also check that you do not get a truncated output. If
snprintf() truncates the output, this should be reported and the error
path should be followed, not to fail silently.

 - Arnaud

>        file = grab_file(filelist, &len);
>
>
> --
> Jesper Juhl <jj@...osbits.net>       http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/