HSTSWRITEFUNCTION stops being called as soon as an stsentry with expires set to TIME_T_MAX is found due to `EOVERFLOW` on `gmtime_r`

[JCMais]

I did this

(sorry, right now I cannot provide a source code to easily reproduce this - I hope just this description is enough)

Created an easy handle and set the option HSTSREADFUNCTION to read from a list with the following data (JSON here for convenience):

[
  {
    host: 'donotcall.gov',
    includeSubdomain: true
  },
  {
    host: 'owasp.org',
    expire: '20220914 12:29:24',
  },
  {
    host: 'github.com',
    expire: '20350101 00:00:00'
  },
]

Assume that if there is no expire in the JSON object, this means I'm not setting the expire in the sts struct. From the documentation:

Set 'expire' to a date stamp or a zero-length string for forever (wrong date stamp format might cause the name to not get accepted)

By default expire is already set to a zero-length string, so I'm just not changing it:

curl/lib/hsts.c

Line 438 in 1c1d9f1

e.expire[0] = 0;

And if there is no includeSubdomain, it means I'm setting it to false in the struct.

I have also set the HSTSWRITEFUNCTION option to save the data when the handle is closed. Right now I'm just printing it.

Finally, I made a request against https://owasp.org/ with CURLOPT_HSTS_CTRL set to ENABLE.

I expected the following

The cache saved has 3 items where only the second one was updated (with the new expire retrieved from the host after the request). However, what happens is that HSTSWRITEFUNCTION is not called at all.

I did some debugging, and it seems that when the easy handle is closed and Curl_hsts_save is called, the first stsentry passed to hsts_push has their sts->expires set to 9223372036854775807. This is the cache entry for donotcall.gov, as the expire is not set it defaults to TIME_T_MAX:

curl/lib/hsts.c

Lines 447 to 450 in 1c1d9f1

	if(e.expire[0])
	expires = Curl_getdate_capped(e.expire);
	else
	expires = TIME_T_MAX; /* the end of time */

This causes the call to Curl_gmtime here:

curl/lib/hsts.c

Line 286 in 1c1d9f1

result = Curl_gmtime((time_t)sts->expires, &stamp);

To return CURLE_BAD_FUNCTION_ARGUMENT. On my machine, this is using gmtime_r, and it seems that the tm pointer is set to null:

https://github.com/curl/curl/blob/4d2f8006777d6354d9b62eae38ebd0a0256d0f94/lib/parsedate.c#L586-L602

gdb output after running that statement:

(gdb) list
593       tm = gmtime(&intime);
594       if(tm)
595         *store = *tm; /* copy the pointed struct to the local copy */
596     #endif
597
598       if(!tm)
599         return CURLE_BAD_FUNCTION_ARGUMENT;
600       return CURLE_OK;
601     }
(gdb) 
(gdb) print *store
$18 = {tm_sec = 7, tm_min = 30, tm_hour = 15, tm_mday = 32767, tm_mon = 58, tm_year = 219248568, tm_wday = 0, tm_yday = 0, tm_isdst = 0, tm_gmtoff = 0, tm_zone = 0x7ffff6e351a9 "GMT"}
(gdb) print tm
$19 = (const struct tm *) 0x0
(gdb) print intime
$20 = 9223372036854775807

Checking errno:

(gdb) p __errno_location()
$21 = (int *) 0x7ffff7fdd6c8
(gdb) x/x $21
0x7ffff7fdd6c8: 0x0000004b

0x0000004b is EOVERFLOW. From https://en.cppreference.com/w/c/chrono/gmtime:

POSIX requires that gmtime and gmtime_r set errno to EOVERFLOW if they fail because the argument is too large.

I can default it to a value that is still far in the future, but not big enough to cause the overflow error. However this should probably be handled differently in case it is a real bug. If it is not, feel free to close this.

curl/libcurl version

curl 7.78.0 (x86_64-pc-linux-gnu) libcurl/7.78.0 OpenSSL/1.1.1k zlib/1.2.11 brotli/1.0.9 zstd/1.4.9 libidn2/2.1.1 libssh2/1.9.0 nghttp2/1.41.0 OpenLDAP/2.4.47
Release-Date: 2021-07-21
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS brotli Debug HSTS HTTP2 HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB SSL TLS-SRP TrackMemory UnixSockets zstd

operating system

uname -a

Linux JCM-PC 4.19.128-microsoft-standard #1 SMP Tue Jun 23 12:58:10 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

lsb_release -a

Distributor ID: Ubuntu
Description:    Ubuntu 18.04.5 LTS
Release:        18.04
Codename:       bionic

gcc: 7.5.0

[bagder]

Confirmed. gmtime_r() returns error for TIME_T_MAX. As a little experiment I also tried TIME_T_MAX - (24*3600*365) but that also makes it return NULL.

Clearly we need to handle the "unlimited" case differently.

[bagder]

The int tm_year field in the struct tm is for "year - 1900" but as a signed 32 bit integer it can then only store 2^31 (which then would represent year 2147485548 at most) and for time_t set to 9223372036854775807 that field not a large enough field to hold the year number...!

[JCMais]

Thanks for the insights @bagder - The fix makes sense too.

[JCMais]

@bagder not really related to this issue, but I just noticed that if CURLSTS_FAIL is returned from the HSTSREADFUNCTION, it does not stop the request from being made, is that on purpose? Should I open another issue?

I was expecting the request to be aborted with CURLE_ABORTED_BY_CALLBACK.

Important to notice that I'm using the multi interface.

[bagder]

if CURLSTS_FAIL is returned from the HSTSREADFUNCTION, it does not stop the request from being made, is that on purpose? Should I open another issue?

It sounds like a separate issue. And I think we should clarify in the docs what it means to return an error there. I think it should fail the transfer...

[JCMais]

@bagder created a new issue #7726

I just tested it using the easy handle directly, and the same behavior happens. The request is not aborted.