[ntp:security] Re: Possibly a security bug

Danny Mayer mayer at gis.net
Fri Aug 18 02:09:04 UTC 2006


Martin Burnicki wrote:
> Danny,
> 
> In order to find out more about the reasons for bug #527 I've run the
> evaluation version of IBM's Rational Purify on ntpd:
> http://www14.software.ibm.com/webapp/download/search.jsp?q=purify
> 
> That's a memory checking tool for Windows. Unfortunately this didn't
> return new hints to solve bug #527, but it has returned some minor
> warnings, plus one which should be examined in detail.
> 

Yes, I'm familiar with it. We used it a lot when I was doing work for
IBM. I really didn't have a lot of hope that this would find the
problem. I have a different idea involving VS 2005.

> Since this warning includes copying of random bytes from the stack to a
> variable I've not yet opened a bugzilla issue for this. However, I don't
> really think this could be used as a point for attacks, so if you want I
> can still open an issue, or someone else can do so for a closed group as
> for bug #527.
> 

No, but it is a buffer overflow and it needs to be fixed.

> In my ntp.conf there's a single "server" line which contains a hostname
> rather tha an IP address. If I run ntpd then purify displays the
> following error:
> 
> [E] ABR: Array bounds read in memcpy {1 occurrence}
>         Reading 16 bytes from 0x0226e298 (12 bytes at 0x0226e29c
>           illegal)
>         Address 0x0226e298 is at the beginning of a 4 byte block
>         Address 0x0226e298 points to a malloc'd block in heap 0x02260000
>         Thread ID: 0x798
>         Error location
>             memcpy         [atonexit.c]
>             do_nodename    [d:\ntp\bk\ntp-dev\libntp\ntp_rfc2553.c:416]
>                     ai->ai_family = hp->h_addrtype;
>                     ai->ai_addrlen = sizeof(struct sockaddr);
>                     sockin = (struct sockaddr_in *)ai->ai_addr;
>              =>     memcpy(&sockin->sin_addr, hp->h_addr, hp->h_length);
>                     ai->ai_addr->sa_family = hp->h_addrtype;
>                 #ifdef HAVE_SA_LEN_IN_STRUCT_SOCKADDR
>                     ai->ai_addr->sa_len = sizeof(struct sockaddr);
>             getaddrinfo    [d:\ntp\bk\ntp-dev\libntp\ntp_rfc2553.c:221]
>             getnetnum      [d:\ntp\bk\ntp-dev\ntpd\ntp_config.c:2226]
>             getconfig      [d:\ntp\bk\ntp-dev\ntpd\ntp_config.c:651]
>             ntpdmain       [d:\ntp\bk\ntp-dev\ntpd\ntpd.c:846]
>             main
> [d:\ntp\bk\ntp-dev\ports\winnt\ntpd\ntservice.c:86]
>             mainCRTStartup [crtexe.c:338]
>         Allocation location
>             malloc         [dbgheap.c:129]
>             AddToAddresses
> [d:\ntp\bk\ntp-dev\ports\winnt\libntp\dnslookup.c:87]
>                     else
>                     {
>                         csize = 0;
>              =>         addr_list = malloc(sizeof(struct in_addr));
>                     }
>                     addr = (char *) addr_list;
>                     sinaddr = &((struct in_addr*) addr)[(*cnt)];
> 
> I.e. the line
> 
> memcpy(&sockin->sin_addr, hp->h_addr, hp->h_length);
> 
> copies 16 bytes from hp->h_addr to &sockin->sin_addr. The the memory of
> hp->h_addr has been allocated in dnslookup.c:
> 
> addr_list = malloc(sizeof(struct in_addr));
> 
> but only a 4 bytes block is allocated there, and the 12 remaining bytes
> which are copied contain random values from the stack. As a workaround
> I've changed that line to read:
> 
> addr_list = malloc(sizeof(struct in_addr) + 12);
> 
> and the warning disappeared.
> 

I did these, so you can blame me for this. It had to do with some
special work to get control of return codes from dns lookups. Getting it
right was hard and I overlooked this code. Thanks for finding these.

> As already said, I neither see how this could be used for an attack, nor
> do I see any bad behaviour of ntpd that could be caused by this. And
> unfortunately, it doesn't seem to have to do with bug #527.
> 

No, it won't I did this work after we had this problem with bug #527.

> I've not enough insight in the code, so I don't know how the code should
> be fixed correctly, and I leave it up to Danny to do so.
> 

Yes I'll fix it.

> BTW, the minor warnings include the local variables named hToken, the
> addresses of which are passed to a Windows API which shall store handle
> value there.
> 
> For what reason ever, those Windows APIs seem to read those variables
> before they write the newly assigned handle number to it, so purify also
> returneds warnings on this.
>

Actually no. It's being passed by reference in order for the API to
return a value in it. It doesn't really care what's in the variable.
Purify can't know that without knowing what goes on in the API and it
has no access to that. It's just warning as a precaution.

> In my latest patch I've initialized those hToken variables with
> INVALID_HANDLE_VALUE which satisfies purify since the variables now
> contain determined inital values which should not confuse those Windows
> API calls, once they should really evaluate those values.
> 

Which is the right thing to do anyway.

Danny
> 
> Martin



More information about the security mailing list