netdev - RE: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning

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]

Date:	Mon, 30 Sep 2013 10:29:19 +0100
From:	"David Laight" <David.Laight@...LAB.COM>
To:	"Sohny Thomas" <sthomas@...ux.vnet.ibm.com>,
	"Fan Du" <fan.du@...driver.com>
Cc:	<stephen@...workplumber.org>, <netdev@...r.kernel.org>
Subject: RE: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning

> > This is a false positive warning as the destination pointer "buf"
> > pointers to
> > an ZERO length array, which actually will occupy alg.buf mostly.
> > Fix this by using memcpy.
> >
> > struct xfrm_algo {
> >          char            alg_name[64];
> >          unsigned int    alg_key_len;    /* in bits */
> >          char            alg_key[0];
> > };
> >
> > struct {
> >          union {
> >                  struct xfrm_algo alg;
> >                  struct xfrm_algo_aead aead;
> >                  struct xfrm_algo_auth auth;
> >          } u;
> >          char buf[XFRM_ALGO_KEY_BUF_SIZE];
> > } alg = {};
> >
> > buf = alg.u.alg.alg_key;

That is worse than horrid...
The tools have every right to complain about any accesses to alg_key[].

> > ---
> >   ip/xfrm_state.c |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
> > index 0d98e78..5cc87d3 100644
> > --- a/ip/xfrm_state.c
> > +++ b/ip/xfrm_state.c
> > @@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
> > enum xfrm_attr_type_t type,
> >               if (len > max)
> >                   invarg("\"ALGO-KEY\" makes buffer overflow\n", key);

I presume there is a return hiding in invarg().

> >
> > -            strncpy(buf, key, len);
> > +            memcpy(buf, key, len);

Passing the length of the SOURCE to strncpy() is almost always wrong.
You are still not terminating the copied string.

	David