Opened 4 months ago

Closed 4 months ago

#893 closed Bug / Defect (wontfix)

Possible null pointer dereference in time_string()

Reported by: gvranken Owned by:
Priority: minor Milestone:
Component: Generic / unclassified Version: 2.2.2
Severity: Not set (if unsure, select this one) Keywords:
Cc:

Description

In time_string() (otime.c):

buf_printf(&out, "%s", ctime(&t));

buf_printf call vsnprintf which tries to print the string returned ctime.
However, ctime may return NULL if 't' is invalid. On Linux/glibc and FreeBSD (these systems I've tested so far), vsnprintf will print the string "(null)". But as far as I can confirm, the behavior of vsnprintf is undefined in this case (relevant standards make no mention of this particular situation), so a NULL pointer dereference may occur.

Is there any code path in which 't' is defined (sent) by the peer?
Can the case vsnprintf with format string "%s" and argument NULL be tested on other systems than the ones I mentioned? I'd be interesting to see what the maximum impact, and on which system, may be.

Guido

Attachments (1)

0001-Prevents-that-uninitialized-data-is-formatted-and-pr.patch (1.1 KB) - added by gvranken 4 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 months ago by cron2

in time_string(), t is a local variable, so &t will always be valid.

(The caller can influence the number stored in t, but as there are no restrictions on the numerical value wrt "seconds since the begin of the epoch", it can not be invalid either).

comment:2 Changed 4 months ago by gvranken

Yes it can be invalid

#include <string.h>
#include <stdio.h>
#include <time.h>
int main(void)
{
    time_t t;
    memset(&t, 0x01, sizeof(t));
    printf("%p\n", ctime(&t));
}
Last edited 4 months ago by gvranken (previous) (diff)

comment:3 Changed 4 months ago by plaisthos

Yes, a time_t can be invalid, but the time_t in our code is always valid.

comment:4 Changed 4 months ago by cron2

The example prints "0x800bc7340" here... and printing the string instead of the pointer, int prints "Thu Aug 9 07:17:53 -2002596506", which might not be very useful, but is most certainly not NULL.

But otherwise, what plaistos says - we either pass in "0", or time values fully under our control (mi->created, route->last_reference, pin->time (0 or time()), or DHCP lease stuff also set ourselves, or e->tv via tv_string_abs(), which is our event stuff).

comment:5 Changed 4 months ago by gvranken

Ok, I get a NULL ptr on Debian 64 bit for my example.

It still might be worth the effort to do something like

timestr = ctime(&t);
if ( timestr )
   buf_printf(&out, "%s", ctime(&t));
else
   buf_printf(&out, "(null"));

Theoretically the preceding gettimeofday() can fail and leave 'tv' undefined. Definitely a corner case but who knows. And maybe in the future paths to time_string() may be created that handle arbitrary user time_t values.

Other than that, I have no further comments and you may close this report.

comment:6 Changed 4 months ago by gvranken

Added a proper patch

comment:7 Changed 4 months ago by cron2

  • Resolution set to wontfix
  • Status changed from new to closed

I do not see this is a good use of our collective time - there's more pressing things right now, and the otime.c code is due for a rewrite (to produce proper ISO timestamps) anyway, there's an existing trac ticket for that.

Given that all platforms that we support *either* just accept arbitrary values passed as time_t *or* just return "(null)" if feeding a NULL pointer to "%s", and we never pass in anything remotely undefined today (and there is no reason why we should in future, as there are no timestamps in the protocol we might be tempted to print), this is far too obscure to worry about.

Note: See TracTickets for help on using tickets.