Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
JCMais opened this issue Sep 14, 2021 · 6 comments

Comments

@JCMais
Copy link
Contributor

JCMais commented Sep 14, 2021

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

@JCMais JCMais changed the title HSTSWRITEFUNCTION stops being called as soon as an stsentry with expires set to TIME_T_MAX is found HSTSWRITEFUNCTION stops being called as soon as an stsentry with expires set to TIME_T_MAX is found due to EOVERFLOW on gmtime_r Sep 14, 2021
@bagder
Copy link
Member

bagder commented Sep 14, 2021

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 added a commit that referenced this issue Sep 14, 2021
When setting a blank expire string, meaning unlimited, curl would pass
TIME_T_MAX to getime_r() when creating the output, while on 64 bit
systems such a large value cannot be convetered to a tm struct making
curl to exit the loop with an error instead.

Starting now, unlimited expiry is instead handled differently by using a
human readable expiry date spelled out as "unlimited" instead of trying
to use a distant actual date.

Test 1660 and 1915 have been updated to help verify this change.

Reported-by: Jonathan Cardoso
Fixes #7720
@bagder
Copy link
Member

bagder commented Sep 14, 2021

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
Copy link
Contributor Author

JCMais commented Sep 14, 2021

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

@JCMais
Copy link
Contributor Author

JCMais commented Sep 15, 2021

@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
Copy link
Member

bagder commented Sep 15, 2021

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...

@bagder bagder closed this as completed in 54f6d83 Sep 15, 2021
@JCMais
Copy link
Contributor Author

JCMais commented Sep 15, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants