kauth: fix clock skew detection
authorBenjamin Kaduk <kaduk@mit.edu>
Wed, 22 Apr 2015 17:43:43 +0000 (13:43 -0400)
committerJeffrey Altman <jaltman@your-file-system.com>
Wed, 6 May 2015 22:16:19 +0000 (18:16 -0400)
Commit 5b3c1042969daec38ccb260e61d665eda0c713ea changed/removed some
uses of abs() on unsigned time values. While the previous use of abs()
was indeed incorrect, the result wasn't necessarily much better, even
though it built with recent compilers, since it only checked for skew
in one direction.

Define and use a  macro to correctly evaluate the conditionals in 64-bit
precision, avoiding C's integer promotion rules which prefer unsigned types
(Date) to signed types of the same width (time_t on 32-bit systems).

Change-Id: Ifcbe59e73942a52a8635cb0f43cce94fdeea85a3
Reviewed-on: http://gerrit.openafs.org/11850
Reviewed-by: Stephan Wiesand <stephan.wiesand@desy.de>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>

src/kauth/kaprocs.c
src/kauth/kauth_internal.h
src/kauth/krb_udp.c

index 8a4914d..315096a 100644 (file)
@@ -711,7 +711,7 @@ ChangePassWord(struct rx_call *call, char *aname, char *ainstance,
     /* validate the request */
     request_time = ntohl(request.time);        /* reorder date */
     kvno = ntohl(request.kvno);
-    if (labs(request_time - time(NULL)) > KTC_TIME_UNCERTAINTY ||
+    if (check_ka_skew(request_time, time(NULL), KTC_TIME_UNCERTAINTY) ||
        strncmp(request.label, KA_CPW_REQ_LABEL, sizeof(request.label)) ||
        request.spare || kvno > MAXKAKVNO) {    /* these are reserved */
        code = KABADREQUEST;
@@ -1143,7 +1143,7 @@ Authenticate(int version, struct rx_call *call, char *aname, char *ainstance,
     }
 #endif /* EXPIREPW */
 
-    if (request.time - now > KTC_TIME_UNCERTAINTY) {
+    if (check_ka_skew(request.time, now, KTC_TIME_UNCERTAINTY)) {
 #if 0
        if (oanswer->MaxSeqLen < sizeof(afs_int32))
            code = KAANSWERTOOLONG;
index ae25452..c5c8e01 100644 (file)
@@ -71,4 +71,30 @@ ktc_to_EncryptionKey(struct ktc_encryptionKey *key) {
     return (EncryptionKey *)key;
 }
 
+/*
+ * Some of the RPCs need to verify that two times are within a given
+ * skew window (usually KTC_TIME_UNCERTAINTY, 15 minutes).  However,
+ * there are multiple sources of timestamps.  The "AFS-native" type,
+ * Date, is afs_uint32; timestamps fetched from the system will
+ * generally be the output of time(NULL), i.e., time_t.  However, the
+ * base type of time_t is platform-dependent -- on some systems, it
+ * is int32_t, and on others it is int64_t.  Arithmetic operations
+ * and comparisons between integers of different type are subject to
+ * the usual arithmetic promotions in C -- comparing a uint32_t and
+ * an int32_t results in the int32_t being "promoted" to uint32_t, which
+ * has disasterous consequences when the value being promoted is
+ * negative.  If, on the other hand, time_t is int64_t, then the promotions
+ * work the other way, with everything evaluated at int64_t precision,
+ * since int64_t has a higher conversion rank than int32_t (which has
+ * the same conversion rank as uint32_t).  In order to properly and
+ * portably check the time skew, it is simplest to cast everything to
+ * afs_int64 before evaluating any expressions.
+ *
+ * The expression evaluates to true if the absolute value of the difference
+ * between the two time inputs is larger than the skew.
+ */
+#define check_ka_skew(__t1, __t2, __skew) \
+       ((afs_int64)(__t1) - (afs_int64)(__skew) > (afs_int64)(__t2) || \
+       (afs_int64)(__t2) - (afs_int64)(__skew) > (afs_int64)(__t1))
+
 #endif
index 189e313..88a37a0 100644 (file)
@@ -294,7 +294,7 @@ UDP_Authenticate(int ksoc, struct sockaddr_in *client, char *name,
            code = KERB_ERR_NAME_EXP;   /* XXX Could use another error code XXX */
            goto abort;
        }
-       if (startTime - now > KTC_TIME_UNCERTAINTY) {
+       if (check_ka_skew(startTime, now, KTC_TIME_UNCERTAINTY)) {
            code = KERB_ERR_SERVICE_EXP;        /* was KABADREQUEST */
            goto abort;
        }