Comment 3 for bug 291265

Revision history for this message
Jamie Strandboge (jdstrand) wrote : Re: Buffer overflow in check_ntp_peer - Nagios can't check time servers in Intrepid

I looked at this a bit, and the math seems to be wrong in this line:
#define SIZEOF_NTPCM(m) (12+ntohs(m.count)+((m.count)?4-(ntohs(m.count)%4):0))

In ntp_request we have (where MAX_CM_SIZE is defined as 468):
req.count=htons(MAX_CM_SIZE);

Which makes req.count = 54273. Later, we have:
if(read(conn, &req, SIZEOF_NTPCM(req)) == -1)

So the nbytes for read() ends up being:
(12 + 468 + (4 - 0)) = 484

However, a sizeof(req) reveals that it is 480 bytes (this can also be seen by looking at the ntp_control_message struct (1+1+2+2+2+2+2+468)). This is not security relevant, because the 4 bytes that are overwritten end up being the 'conn' file descriptor (as seen from gdb), which triggers read() to:

read(3, 0xbffff850, 484) = ? ERESTARTSYS (To be restarted)
--- SIGALRM (Alarm clock) @ 0 (0) ---

resulting in check_ntp_peer to error out with:
CRITICAL - Socket timeout after 10 seconds

This is a bug whether or not _FORTIFY_SOURCE is used, because read() may SIGALRM. You'll also notice that check_ntp.c suffers from the same problem (the code in question is identical), as seen with:
$ /usr/lib/nagios/plugins/check_ntp -H foo -j 1