| Message ID | 8ac57c96bf5a0695ecc67fd230440b0b9d15740f.1502246502.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix y2038 issues for security/keys subsystem | expand |
On Wed, Aug 9, 2017 at 4:51 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h > index 5de0673..76421e2 100644 > --- a/include/keys/rxrpc-type.h > +++ b/include/keys/rxrpc-type.h > @@ -127,4 +127,25 @@ struct rxrpc_key_data_v1 { > #define AFSTOKEN_K5_ADDRESSES_MAX 16 /* max K5 addresses */ > #define AFSTOKEN_K5_AUTHDATA_MAX 16 /* max K5 pieces of auth data */ > > +/* > + * truncate a time64_t to the range from 1970 to 2106 as > + * in the network protocol > + */ > +static inline u32 rxrpc_time64_to_u32(time64_t time) > +{ > + if (time < 0) > + return 0; > + > + if (time > UINT_MAX) > + return UINT_MAX; > + > + return (u32)time; > +} >+ > +/* extend u32 back to time64_t using the same 1970-2106 range */ > +static inline time64_t rxrpc_u32_to_time64(u32 time) > +{ > + return (time64_t)time; > +} When reviewing this, I could not find any clear definition on whether the preparse functions should treat this as a signed or unsigned 32-bit number. The function as defined here documents as an unsigned as that is more useful (it pushes out the last day this works from year 2038 to 2106) and matches the existing behavior that we got on 64-bit architectures (intentionally or not). > @@ -433,6 +435,7 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep, > struct rxrpc_key_token *token, **pptoken; > struct rxk5_key *rxk5; > const __be32 *end_xdr = xdr + (toklen >> 2); > + time64_t expiry; > int ret; > > _enter(",{%x,%x,%x,%x},%u", > @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep, > pptoken = &(*pptoken)->next) > continue; > *pptoken = token; > - if (token->kad->expiry < prep->expiry) > - prep->expiry = token->kad->expiry; > + expiry = rxrpc_u32_to_time64(token->kad->expiry); > + if (expiry < prep->expiry) > + prep->expiry = expiry; > > _leave(" = 0"); > return 0; I'm still slightly puzzled by what this function does: it does have four timestamps (authtime, starttime, endtime, renew_till) that are all transferred as 64-bit values and won't expire, but then it also uses the 32-bit expiry field in rxrpc_key_token->kad->expiry instead of the 64-bit rxrpc_key_token->k5 fields. This appears to overlay the first 32 bits of the rxrpc_key_token->k5->starttime field, which is also a time value on little-endian architectures by chance, but I would assume that it's always in the past, meaning the keys would already be expired. Any idea what is the intended behavior here? Arnd
Arnd Bergmann <arnd@arndb.de> wrote: > > @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep, > > pptoken = &(*pptoken)->next) > > continue; > > *pptoken = token; > > - if (token->kad->expiry < prep->expiry) > > - prep->expiry = token->kad->expiry; > ... > > I'm still slightly puzzled by what this function does: it does have four > timestamps (authtime, starttime, endtime, renew_till) that are all > transferred as 64-bit values and won't expire, but then it also uses the > 32-bit expiry field in rxrpc_key_token->kad->expiry instead of the 64-bit > rxrpc_key_token->k5 fields. Good catch. This is a cut'n'paste error. It should be using token->k5->expiry here not token->kad->expiry. > This appears to overlay the first 32 bits of the > rxrpc_key_token->k5->starttime field, which is also a time value on > little-endian architectures by chance, but I would assume that it's always > in the past, meaning the keys would already be expired. Yeah - I'm not sure why it works. David
On Wed, Aug 9, 2017 at 11:33 AM, David Howells <dhowells@redhat.com> wrote: > Arnd Bergmann <arnd@arndb.de> wrote: > >> > @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep, >> > pptoken = &(*pptoken)->next) >> > continue; >> > *pptoken = token; >> > - if (token->kad->expiry < prep->expiry) >> > - prep->expiry = token->kad->expiry; >> ... >> >> I'm still slightly puzzled by what this function does: it does have four >> timestamps (authtime, starttime, endtime, renew_till) that are all >> transferred as 64-bit values and won't expire, but then it also uses the >> 32-bit expiry field in rxrpc_key_token->kad->expiry instead of the 64-bit >> rxrpc_key_token->k5 fields. > > Good catch. This is a cut'n'paste error. It should be using > token->k5->expiry here not token->kad->expiry. Do you know which format is used in practice? Are both kad and k5 common among rxrpc users? Arnd
Arnd Bergmann <arnd@arndb.de> wrote: > Do you know which format is used in practice? Are both kad and k5 common > among rxrpc users? The aklog program I'm using uses the non-XDR interface to push a Kerberos 5 ticket to the kernel, so it doesn't actually invoke rxrpc_preparse_xdr() from rxrpc_preparse(). David
On Wed, Aug 9, 2017 at 3:26 PM, David Howells <dhowells@redhat.com> wrote: > Arnd Bergmann <arnd@arndb.de> wrote: > >> Do you know which format is used in practice? Are both kad and k5 common >> among rxrpc users? > > The aklog program I'm using uses the non-XDR interface to push a Kerberos 5 > ticket to the kernel, so it doesn't actually invoke rxrpc_preparse_xdr() from > rxrpc_preparse(). Ah, I'm slowly starting to understand how this fits together. So you can add a key either through key_add() from local user space, or through an rxrpc socket. From what I can tell, the program you have at http://people.redhat.com/~dhowells/rxrpc/klog.c will keep working beyond 2038 but not beyond 2106 on all 64-bit architectures and on those 32-bit systems that have a libc with 64-bit time_t. It could be modified to use the xdr_rxk5 key format, which would make it use 64-bit time values (and require the kernel fix mentioned above). In contrast, the rxrpc socket interface would need a major rework to support 64-bit expiration times. It receives a kerberos ticket with a 32-bit issue time that gets used to calculate the expiry time in rxkad_decrypt_ticket, and from there we pass it through a rxrpc_key_data_v1 into key_instantiate_and_link(), which calls rxrpc_preparse() and that just takes the expiry out again and sticks it into another 32-bit field in struct rxkad_key, from where it finally gets copied into the (now 64-bit) key_preparsed_payload->expiry field. Does my understanding match what you intended for the interfaces? Is there a need to extend the rxrpc socket interface to support xdr_rxk5 keys as well? Arnd
Arnd Bergmann <arnd@arndb.de> wrote: > Ah, I'm slowly starting to understand how this fits together. So you can add > a key either through key_add() from local user space, or through an rxrpc > socket. No, you can't add keys through an rxrpc socket. There are three 'classes' of key: (1) Client keys (type rxrpc). These must be added by add_key() by userspace (but could also be acquired by upcalling to /sbin/request-key) and then the kernel calls request_key() to locate them on entry through either a kafs inode/file operation or through sendmsg() to an AF_RXRPC socket. (2) Server keys (type rxrpc_s). These are created by userspace and are presented to an AF_RXRPC server socket by calling setsockopt(). The server uses these to validate/decrypt the token passed by a RESPONSE packet. (3) Service connection keys (type rxrpc). These are created internally by AF_RXRPC after a successful challenge/response negotiation to hold the security details so that we have a struct key to pass around that corresponds to the key in (1). David
diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h index 5de0673..76421e2 100644 --- a/include/keys/rxrpc-type.h +++ b/include/keys/rxrpc-type.h @@ -127,4 +127,25 @@ struct rxrpc_key_data_v1 { #define AFSTOKEN_K5_ADDRESSES_MAX 16 /* max K5 addresses */ #define AFSTOKEN_K5_AUTHDATA_MAX 16 /* max K5 pieces of auth data */ +/* + * truncate a time64_t to the range from 1970 to 2106 as + * in the network protocol + */ +static inline u32 rxrpc_time64_to_u32(time64_t time) +{ + if (time < 0) + return 0; + + if (time > UINT_MAX) + return UINT_MAX; + + return (u32)time; +} + +/* extend u32 back to time64_t using the same 1970-2106 range */ +static inline time64_t rxrpc_u32_to_time64(u32 time) +{ + return (time64_t)time; +} + #endif /* _KEYS_RXRPC_TYPE_H */ diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index 69b9733..3c11443 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -894,7 +894,7 @@ void rxrpc_new_incoming_connection(struct rxrpc_sock *, int rxrpc_request_key(struct rxrpc_sock *, char __user *, int); int rxrpc_server_keyring(struct rxrpc_sock *, char __user *, int); -int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time_t, +int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time64_t, u32); /* diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c index 5436922..e2d3661 100644 --- a/net/rxrpc/key.c +++ b/net/rxrpc/key.c @@ -92,6 +92,7 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep, const __be32 *xdr, unsigned int toklen) { struct rxrpc_key_token *token, **pptoken; + time64_t expiry; size_t plen; u32 tktlen; @@ -158,8 +159,9 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep, pptoken = &(*pptoken)->next) continue; *pptoken = token; - if (token->kad->expiry < prep->expiry) - prep->expiry = token->kad->expiry; + expiry = rxrpc_u32_to_time64(token->kad->expiry); + if (expiry < prep->expiry) + prep->expiry = expiry; _leave(" = 0"); return 0; @@ -433,6 +435,7 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep, struct rxrpc_key_token *token, **pptoken; struct rxk5_key *rxk5; const __be32 *end_xdr = xdr + (toklen >> 2); + time64_t expiry; int ret; _enter(",{%x,%x,%x,%x},%u", @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep, pptoken = &(*pptoken)->next) continue; *pptoken = token; - if (token->kad->expiry < prep->expiry) - prep->expiry = token->kad->expiry; + expiry = rxrpc_u32_to_time64(token->kad->expiry); + if (expiry < prep->expiry) + prep->expiry = expiry; _leave(" = 0"); return 0; @@ -691,6 +695,7 @@ static int rxrpc_preparse(struct key_preparsed_payload *prep) { const struct rxrpc_key_data_v1 *v1; struct rxrpc_key_token *token, **pp; + time64_t expiry; size_t plen; u32 kver; int ret; @@ -777,8 +782,9 @@ static int rxrpc_preparse(struct key_preparsed_payload *prep) while (*pp) pp = &(*pp)->next; *pp = token; - if (token->kad->expiry < prep->expiry) - prep->expiry = token->kad->expiry; + expiry = rxrpc_u32_to_time64(token->kad->expiry); + if (expiry < prep->expiry) + prep->expiry = expiry; token = NULL; ret = 0; @@ -955,7 +961,7 @@ int rxrpc_server_keyring(struct rxrpc_sock *rx, char __user *optval, */ int rxrpc_get_server_data_key(struct rxrpc_connection *conn, const void *session_key, - time_t expiry, + time64_t expiry, u32 kvno) { const struct cred *cred = current_cred(); @@ -982,7 +988,7 @@ int rxrpc_get_server_data_key(struct rxrpc_connection *conn, data.kver = 1; data.v1.security_index = RXRPC_SECURITY_RXKAD; data.v1.ticket_length = 0; - data.v1.expiry = expiry; + data.v1.expiry = rxrpc_time64_to_u32(expiry); data.v1.kvno = 0; memcpy(&data.v1.session_key, session_key, sizeof(data.v1.session_key)); diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c index 46d1a1f..34c86d2 100644 --- a/net/rxrpc/rxkad.c +++ b/net/rxrpc/rxkad.c @@ -854,7 +854,7 @@ static int rxkad_decrypt_ticket(struct rxrpc_connection *conn, struct sk_buff *skb, void *ticket, size_t ticket_len, struct rxrpc_crypt *_session_key, - time_t *_expiry, + time64_t *_expiry, u32 *_abort_code) { struct skcipher_request *req; @@ -864,7 +864,7 @@ static int rxkad_decrypt_ticket(struct rxrpc_connection *conn, struct in_addr addr; unsigned int life; const char *eproto; - time_t issue, now; + time64_t issue, now; bool little_endian; int ret; u32 abort_code; @@ -960,15 +960,15 @@ static int rxkad_decrypt_ticket(struct rxrpc_connection *conn, if (little_endian) { __le32 stamp; memcpy(&stamp, p, 4); - issue = le32_to_cpu(stamp); + issue = rxrpc_u32_to_time64(le32_to_cpu(stamp)); } else { __be32 stamp; memcpy(&stamp, p, 4); - issue = be32_to_cpu(stamp); + issue = rxrpc_u32_to_time64(be32_to_cpu(stamp)); } p += 4; - now = get_seconds(); - _debug("KIV ISSUE: %lx [%lx]", issue, now); + now = ktime_get_real_seconds(); + _debug("KIV ISSUE: %llx [%llx]", issue, now); /* check the ticket is in date */ if (issue > now) { @@ -1053,7 +1053,7 @@ static int rxkad_verify_response(struct rxrpc_connection *conn, struct rxrpc_skb_priv *sp = rxrpc_skb(skb); struct rxrpc_crypt session_key; const char *eproto; - time_t expiry; + time64_t expiry; void *ticket; u32 abort_code, version, kvno, ticket_len, level; __be32 csum;
Since the 'expiry' variable of 'struct key_preparsed_payload' has been changed to 'time64_t' type, which is year 2038 safe on 32bits system. In net/rxrpc subsystem, we need convert 'u32' type to 'time64_t' type when copying ticket expires time to 'prep->expiry', then this patch introduces two helper functions to help convert 'u32' to 'time64_t' type. This patch also uses ktime_get_real_seconds() to get current time instead of get_seconds() which is not year 2038 safe on 32bits system. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- include/keys/rxrpc-type.h | 21 +++++++++++++++++++++ net/rxrpc/ar-internal.h | 2 +- net/rxrpc/key.c | 22 ++++++++++++++-------- net/rxrpc/rxkad.c | 14 +++++++------- 4 files changed, 43 insertions(+), 16 deletions(-) -- 1.7.9.5