openbmc / phosphor-certificate-manager

Apache License 2.0
4 stars 4 forks source link

Allow certificates with expiry date >2038 after time_t has been changed to int64 #15

Closed devenrao closed 2 years ago

devenrao commented 4 years ago

I am having an issue where the user uploads a certificate with validity period of 25 years which turns out to be year 2045. But the expiry date value wraps around due to overflow and the final time generated is year 1908. Time value is represented as number of seconds since the start of the Unix epoch. This happens when certificates are listed in GUI by fetching the properties using Redfish (https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/certificate_service.hpp#L659)

As time_t data structure is defined as int32 it can hold up to a maximum value of 2147483647 which covers certificate validity up to 2038.

Any certificate which is uploaded with expiry year greater than 2038, the seconds value in time_t wraps around and will generate a negative value, which when used expiry date value will be less than 1970.

Example: Mar 10 15:03:49 w5 bmcweb[170]: bmcweb not after 2366947077 Mar 10 15:03:49 w5 bmcweb[170]: bmcweb not after 1908-11-26T22:49:41+00:00

Probable solutions 1) Do nothing as the chances of uploading a certificate with expiry date > 18 chances never happen 2) Return error to the caller if the expirty date is greater than 2038.

Would like community feedback on this

https://stackoverflow.com/questions/6408914/what-is-year-2038-problem-how-to-find-out-if-the-existing-code-has-that-problem https://github.com/openbmc/phosphor-certificate-manager/issues/14

devenrao commented 4 years ago

On Wed, Mar 11, 2020 at 09:49:02AM +0000, Devender Rao wrote:

As time_t data structure is defined as int32 it can hold up to a maximum value There is significant upstream work going on to transition time_t to a 64 bit integer even on 32 bit machines (x86-64 and ARM64 already have a 64 bit time_t).

Kernel changes are already in as of 5.1 to support a userspace with 64-bit time_t but the kernel itself uses 32-bit internally. There is a merge that is heading into 5.6 to change the kernel (but I don't think we need this):

https://lore.kernel.org/lkml/CAK8P3a2iZyA1VSFqvcEc9o59F76GgzLBiOAmEuHKD81FErPLDQ@mail.gmail.com/

That pull request mentions userspace changes coming in glibc-2.32 that will use the 64-bit time_t syscalls and transition userspace over to 64-bit everywhere. glibc-2.32 is scheduled for August 2020.

Probable solutions 1) Do nothing as the chances of uploading a certificate with expiry date > 18 chances never happen 2) Return error to the caller if the expirty date is greater than 2038.

With this in mind I'd go with #2 in the short term until we get the upstream changes.

These coming changes should cause us to think through any cases where we might be relying on a 32-bit time_t, especially in serialization. I have a little concern that we're going to end up breaking some upgrade paths when we are using binary formats (like some code using Cereal might be). How do we want to audit and fix that now?

-- Patrick Williams

devenrao commented 4 years ago

On Thu, Mar 12, 2020 at 2:56 AM Patrick Williams patrick@stwcx.xyz wrote:

On Wed, Mar 11, 2020 at 09:49:02AM +0000, Devender Rao wrote:

As time_t data structure is defined as int32 it can hold up to a maximum value There is significant upstream work going on to transition time_t to a 64 bit integer even on 32 bit machines (x86-64 and ARM64 already have a 64 bit time_t).

Kernel changes are already in as of 5.1 to support a userspace with 64-bit time_t but the kernel itself uses 32-bit internally. There is a merge that is heading into 5.6 to change the kernel (but I don't think we need this):

https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_lkml_CAK8P3a2iZyA1VSFqvcEc9o59F76GgzLBiOAmEuHKD81FErPLDQ-40mail.gmail.com_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=lRk-_5CBeMVVDnT3QHI6mxitnCerDZxcmfBdwqFZjTw&m=wFq2hSjEr9W2wvtIX9scjbfS1ooetO4XZnbUvXrQcu4&s=4JzzlMiTLzdMnf24nJFUnX41H_jzT3E7J-6bSuUOdm4&e=

That pull request mentions userspace changes coming in glibc-2.32 that will use the 64-bit time_t syscalls and transition userspace over to 64-bit everywhere. glibc-2.32 is scheduled for August 2020.

Probable solutions 1) Do nothing as the chances of uploading a certificate with expiry date > 18 chances never happen 2) Return error to the caller if the expirty date is greater than 2038.

With this in mind I'd go with #2 in the short term until we get the upstream changes.

Agreed, a fair certificate should not have such an expiry date.

These coming changes should cause us to think through any cases where we might be relying on a 32-bit time_t, especially in serialization. I have a little concern that we're going to end up breaking some upgrade paths when we are using binary formats (like some code using Cereal might be). How do we want to audit and fix that now?

There is an interesting article in LWN talking about how Debian will handle the 2038 case: https://urldefense.proofpoint.com/v2/url?u=https-3A__lwn.net_Articles_812767_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=lRk-_5CBeMVVDnT3QHI6mxitnCerDZxcmfBdwqFZjTw&m=wFq2hSjEr9W2wvtIX9scjbfS1ooetO4XZnbUvXrQcu4&s=70kLJ7r_gFtEHqnIN9HhEZIDj7ridmVNyOLr4PDIMA4&e=

Toggle message open/close

devenrao commented 2 years ago

issue has been addressed