[PATCH 0/2] xfrm: fix buffer overflows

[Thomas Haller]

On Mon, 2016-06-27 at 19:10 +0200, Thomas Egerer wrote:
> Hello Thomas,
> 
> On 06/25/2016 02:02 PM, Thomas Haller wrote:
> > On Tue, 2016-05-31 at 17:29 +0200, Thomas Egerer wrote:
> > > Hi *,
> > > 
> > > we have found one definite and one potential buffer overflow
> > > in libnl when adding xfrm states.
> > > The definite one is triggered whenever an aead/auth (etc) key
> > > is added to an xfrmnl_sa structure. The potential one is only
> > > triggered if the same functions are called with alg_names
> > > longer than 72/68 bytes + keysize. Then a strcpy call writes
> > > beyond the appropriate data structures in struct xfrmnl_sa.
> > > 
> > > Cheers,
> > > Thomas
> > > 
> > > Thomas Egerer (2):
> > >   xfrm: fix buffer overflow when copying keys
> > >   xfrm: check length of alg_name before strcpying it
> > > 
> > 
> > 
> > Hi Thomas,
> > 
> > 
> > thanks for the 2 patches. Merged both to master:
> > https://github.com/thom311/libnl/commit/c009b20919562e6968969309049
> > 064d59d35010a
> > 
> > 
> > 
> > xfrmnl_sa_get_comp_params() et al. seems wrong too.
> > How can somebody safely know the required size of the @key buffer?
> If changing the API of this part of the code is not a problem, then
> the prototype could be changed such that the key pointer (char *)
> becomes a char ** which is allocated by the function and has to be
> freed by the user. Another option -- adding another maxlen parameter
> to indicate the size of the key buffer -- also means an API change.


Hi,

I think changing ABI or API should be avoided.

But the current version cannot be called without danger of buffer-
overflow (or can it? How?). Thus changing behavior to a completely
broken version seems acceptable.


How about
 https://github.com/thom311/libnl/commits/xfrm-get-params-keylen
?


> > The required size for @alg_name is also not documented, so a user
> > would
> > have to read the source to see that it must be >= 64 bytes
> I don't see a problem here. The kernel API is very similar and
> applications
> use the sizeof operator to obtain the size of the name buffer. Or am
> I
> getting you wrong?

It's nowhere documented in libnl3 API that this buffer shall be >= 64
bytes. Appart from that, there is no problem.



Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20160627/8bd4ee9e/attachment.sig>