Opened 5 years ago

Closed 5 years 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: OpenVPN 2.2.2 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:


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.


Attachments (1)

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

Download all attachments as: .zip

Change History (8)

comment:1 Changed 5 years ago by Gert Döring

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 5 years 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 5 years ago by gvranken (previous) (diff)

comment:3 Changed 5 years ago by plaisthos

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

comment:4 Changed 5 years ago by Gert Döring

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 5 years 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));
   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 5 years ago by gvranken

Added a proper patch

comment:7 Changed 5 years ago by Gert Döring

Resolution: wontfix
Status: newclosed

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.