Opened 7 years ago
Closed 7 years ago
#1368 closed defect (fixed)
Date oveflow problems with ngx_gmtime()
| Reported by: | Jamie Landeg-Jones | Owned by: | |
|---|---|---|---|
| Priority: | minor | Milestone: | |
| Component: | nginx-core | Version: | 1.12.x |
| Keywords: | epoch negative date ngx_gmtime | Cc: | |
| uname -a: | FreeBSD lapcat.dyslexicfish.net 11.1-STABLE FreeBSD 11.1-STABLE #0: Thu Jul 27 19:27:36 BST 2017 root@lapcat.dyslexicfish.net:/usr/obj/usr/src/sys/LAPCAT amd64 | ||
| nginx -V: |
nginx version: nginx/1.12.1
built with OpenSSL 1.0.2l 25 May 2017 TLS SNI support enabled configure arguments: --prefix=/usr/local/etc/nginx --with-cc-opt='-I /usr/local/include' --with-ld-opt='-L /usr/local/lib' --conf-path=/usr/local/etc/nginx/nginx.conf --sbin-path=/usr/local/sbin/nginx --pid-path=/var/run/nginx.pid --error-log-path=/var/log/nginx/error.log --user=www --group=www --modules-path=/usr/local/libexec/nginx --with-file-aio --http-client-body-temp-path=/var/tmp/nginx/client_body_temp --http-fastcgi-temp-path=/var/tmp/nginx/fastcgi_temp --http-proxy-temp-path=/var/tmp/nginx/proxy_temp --http-scgi-temp-path=/var/tmp/nginx/scgi_temp --http-uwsgi-temp-path=/var/tmp/nginx/uwsgi_temp --http-log-path=/var/log/nginx/access.log --with-http_addition_module --with-http_auth_request_module --with-http_dav_module --with-http_flv_module --with-http_gzip_static_module --with-http_gunzip_module --with-http_mp4_module --with-http_random_index_module --with-http_realip_module --with-http_secure_link_module --with-http_slice_module --with-http_stub_status_module --with-http_sub_module --with-pcre --with-http_v2_module --with-stream=dynamic --with-stream_ssl_module --with-stream_ssl_preread_module --with-threads --with-mail=dynamic --without-mail_imap_module --without-mail_pop3_module --without-mail_smtp_module --with-mail_ssl_module --with-http_ssl_module |
||
Description
There is a problem in ngx_gmtime() with dates earlier than the standard epoch, such that the negative number is treated as an unsigned integer, causing the year field to be 5 characters larger than allocated for, and hence the "(ngx_pool_t)*pool" variable to overflow by 5 characters. (E.G. "Last-Modified: Thu, 09 Nov 438498967 05:59:16 GMT")
Now, I don't know if this overflow into "(ngx_buf_t)*header_in" can be exploited, but still, I think it should be avoided.
Now, I appreciate that you aren't concerned with "negative" dates, which is fair enough, but may I at least suggest setting such dates to zero rather than simply casting then onto an unsigned type, as in the attached patch?
01:58 (452) "nginx-1.12.1" jamie@lapcat% l -T /scratch/jamie/ports_base/usr/local/www/nginx-dist/index.html
4 -rw-r--r-- 1 jamie jamie - 4 31 Dec 23:59:00 1969 /scratch/jamie/ports_base/usr/local/www/nginx-dist/index.html
BEFORE PATCH:
01:58 (453) "nginx-1.12.1" jamie@lapcat% curl -vvv localhost:55555
- Rebuilt URL to: localhost:55555/
- Connected to localhost (127.0.0.1) port 55555 (#0)
GET / HTTP/1.1
Host: localhost:55555
User-Agent: curl/7.55.1
Accept: */*
< HTTP/1.1 200 OK
< Server: nginx/1.12.1
< Date: Mon, 28 Aug 2017 00:58:42 GMT
< Content-Type: text/html
< Content-Length: 4
< Last-Modified: Thu, 09 Nov 438498967 05:59:16 GMT
< Connection: keep-alive
< ETag: "-e4c-4"
< Accept-Ranges: bytes
<
boo
- Connection #0 to host localhost left intact
AFTER PATCH:
02:02 (461) "nginx-1.12.1" jamie@lapcat% curl -vvv localhost:55555
- Rebuilt URL to: localhost:55555/
- Trying 127.0.0.1...
- Connected to localhost (127.0.0.1) port 55555 (#0)
GET / HTTP/1.1
Host: localhost:55555
User-Agent: curl/7.55.1
Accept: */*
< HTTP/1.1 200 OK
< Server: nginx/1.12.1
< Date: Mon, 28 Aug 2017 01:02:17 GMT
< Content-Type: text/html
< Content-Length: 4
< Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT
< Connection: keep-alive
< ETag: "-e4c-4"
< Accept-Ranges: bytes
<
boo
- Connection #0 to host localhost left intact
Cheers, Jamie
Attachments (1)
Change History (7)
by , 7 years ago
| Attachment: | patch-src_core_ngx__times.c added |
|---|
comment:1 by , 7 years ago
comment:2 by , 7 years ago
Hiya. No problem!
Arrrgh! Good call! Of course, you're right that you can overflow the 4 digit year with positive numbers too. I'm so stupid... Always focusing on the negative! :-)
Yep, I've seen one or two non-nginx programs hiccup with 64bit u_time, especially with a pre-epoch date.
I must admit, I was surprised that in ngx_times.c where the string itself was created, there was no bound-checking on the character length of the string components, however I'm guessing the reason for that (and also not using strftime) is efficiency. I know nginx has a great reputation for performance, and it would just be a waste of processing cycles bound checking something you can (ordinarily!) rely on to be safe.
Anyway, truncating times after year 9999 sounds good, though you do realise that people will be blaming you for the Y10K problem! ha-ha!
How about something like this? :
--- src/core/ngx_times.c.orig 2017-07-11 14:24:07.000000000 +0100
+++ src/core/ngx_times.c 2017-08-30 07:52:08.403273000 +0100
@@ -304,7 +304,7 @@
/* the calculation is valid for positive time_t only */
- n = (ngx_uint_t) t;
+ n = (ngx_uint_t) t & 0xfffffffff;
days = n / 86400;
That mask will force it to be positive, and also drop the higher bits, meaning the upper limit will be in the 4 digit-year:
Last-Modified: Sun, 20 Aug 4147 07:32:14 GMT
Of course, that will also be the same as "-1" so you may still want to still have the < 0 check before applying the mask, to make the output less confusing at least when a negative date is encountered, although I personally wouldn't care for dates before 1970 or after 4147 other than for enforcing bound checking!
I'm assuming this wouldn't be considered a kludge, or non-portable solution? I'm willing to take the blame for the Y4.147K problem when it occurs :-)
Anyway, just throwing out a few thoughts.
Cheers, Jamie
comment:3 by , 7 years ago
Well, I don't think that introducing Y4.147K problem is a good idea. Rather, I would stick to Y10K one, which is already in various date formats nginx uses. Here are couple of patches which add truncation to December 31, 9999 and also fix ngx_gmtime() on 32-bit platforms with 64-bit time_t.
# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1504058302 -10800
# Wed Aug 30 04:58:22 2017 +0300
# Node ID 22bb74db6e82cc47feb1f516bb78240dadfd85c9
# Parent 9bd5c23f0ed006c904263e3b3c716e4f4a214668
Fixed ngx_gmtime() on 32-bit platforms with 64-bit time_t.
In ngx_gmtime(), instead of casting to ngx_uint_t we now work with
time_t directly. This allows using dates after 2038 on 32-bit platforms
which use 64-bit time_t, notably NetBSD and OpenBSD.
As the code is not able to work with negative time_t values, argument
is now set to 0 for negative values. As a positive side effect, this
results in Epoch being used for such values instead of a date in distant
future.
diff --git a/src/core/ngx_times.c b/src/core/ngx_times.c
--- a/src/core/ngx_times.c
+++ b/src/core/ngx_times.c
@@ -300,23 +300,25 @@ void
ngx_gmtime(time_t t, ngx_tm_t *tp)
{
ngx_int_t yday;
- ngx_uint_t n, sec, min, hour, mday, mon, year, wday, days, leap;
+ ngx_uint_t sec, min, hour, mday, mon, year, wday, days, leap;
/* the calculation is valid for positive time_t only */
- n = (ngx_uint_t) t;
+ if (t < 0) {
+ t = 0;
+ }
- days = n / 86400;
+ days = t / 86400;
+ sec = t % 86400;
/* January 1, 1970 was Thursday */
wday = (4 + days) % 7;
- n %= 86400;
- hour = n / 3600;
- n %= 3600;
- min = n / 60;
- sec = n % 60;
+ hour = sec / 3600;
+ sec %= 3600;
+ min = sec / 60;
+ sec %= 60;
/*
* the algorithm based on Gauss' formula,
# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1504059349 -10800
# Wed Aug 30 05:15:49 2017 +0300
# Node ID 2427016f24617a379c3b891396fae6b01d49381f
# Parent 22bb74db6e82cc47feb1f516bb78240dadfd85c9
Introduced time truncation to December 31, 9999 (ticket #1368).
Various buffers are allocated in an assumption that there would be
no more than 4 year digits. This might not be true on platforms
with 64-bit time_t, as 64-bit time_t is able to represent more than that.
Such dates with more than 4 year digits hardly make sense though, as
various date formats in use does not allow them anyway.
As such, all dates are now truncated by ngx_gmtime() to December 31, 9999.
This should have no effect on valid dates, though will prevent potential
buffer overflows on invalid ones.
diff --git a/src/core/ngx_times.c b/src/core/ngx_times.c
--- a/src/core/ngx_times.c
+++ b/src/core/ngx_times.c
@@ -311,6 +311,16 @@ ngx_gmtime(time_t t, ngx_tm_t *tp)
days = t / 86400;
sec = t % 86400;
+ /*
+ * no more than 4 year digits supported,
+ * truncate to December 31, 9999, 23:59:59
+ */
+
+ if (days > 2932896) {
+ days = 2932896;
+ sec = 86399;
+ }
+
/* January 1, 1970 was Thursday */
wday = (4 + days) % 7;
comment:4 by , 7 years ago
Yeah, looks good! And much cleaner than my hack!
Incidentally, thanks for the quick and thorough response - sadly, too many projects seem to never even look at bug reports.
cheers, jamie
comment:6 by , 7 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Patches were reviewed and committed, thanks.
This used to result in 2038..2106 dates on 32-bit platforms, though certainly need some tweaking for 64-bit. Thanks for reporting this.
I don't think that using Epoch for negative timestamps is enough though. Positive values can result in years longer than 4 digits as well, and the result will be the same. So we should either allocate enough space in all relevant places, or somehow reject/truncate dates after 9999 year.