[PATCH] Fixing buffer over-read when accepting unix domain sockets

Maxim Dounin mdounin at mdounin.ru
Thu Jul 11 11:14:39 UTC 2013


Hello!

On Wed, Jul 10, 2013 at 01:15:48PM -0700, Yichun Zhang (agentzh) wrote:

> Hello!
> 
> On Wed, Jul 10, 2013 at 6:18 AM, Maxim Dounin wrote:
> >> -        if (ls->addr_ntop) {
> >> +        if (ls->addr_ntop && socklen > sizeof(c->sockaddr->sa_family)) {
> >>              c->addr_text.data = ngx_pnalloc(c->pool, ls->addr_text_max_len);
> >>              if (c->addr_text.data == NULL) {
> >>                  ngx_close_accepted_connection(c);
> >
> > The patch looks wrong - it doesn't initialize c->addr_text at all,
> > while it's requested by a caller.
> >
> 
> Thank you for the review!
> 
> How about this?

This doesn't looks good either.  It looks like on linux unix 
sockaddr can't be printed without socklen argument due to abstract 
namespace sockets (see [1]).  Therefore the only correct solution 
seems to be to change ngx_sock_ntop() interface to accept (and 
use) socklen.

Vladimir looked into this a while ago, and I've just reviewed his 
latest patch he resubmitted due to your attempts to fix the same 
issue.  The patch is good enough and expected to be committed 
after few minor fixes.

[1] http://man7.org/linux/man-pages/man7/unix.7.html
    Note "Three types of address are distinguished in this 
    structure..." and below.

-- 
Maxim Dounin
http://nginx.org/en/donation.html



More information about the nginx-devel mailing list