RFR 6722928: Support SSPI as a native GSS-API provider

Nico Williams Nico.Williams at twosigma.com
Fri Feb 15 21:45:02 UTC 2019


On Tue, Jan 22, 2019 at 10:52:59AM +0800, Weijun Wang wrote:
> Webrev updated again at
> 
>   https://cr.openjdk.java.net/~weijun/6722928/webrev.04/

> This time I updated gssapi.h to make use of gss_const_xyz_t types. The types
> in NativeFunc.h and sspi.cpp are updated as well. (I wish there is an
> automatic tool to check the consistence).

Thanks.  It's not quite right though.  See below.

> Several Windows API calls (QueryContextAttributes, MakeSignature,
> VerifySignature, EncryptMessage, DecryptMessage) need an explicit cast (from
> const *p to *p) because they don't announce the pointer to be const, but I
> think it's safe.

Thanks for the warning.  I'll double-check, but it seems likely to be safe,
yes.

 - sspi.cpp:61

   61 #define SEC_SUCCESS(status) (*minor_status = status, (status) >= SEC_E_OK)
                                   ^^^^^^^^^^^^^^^^^^^^^^

   Please add parenthesis around the whole assignment expression.

 - sspi.cpp:129

   129 static long
   130 secondsUntil(int inputIsUTC, TimeStamp *time)

   There's no need to return a signed number, and you assign it to OM_uint32
   variables anyways, and we don't care if some credential expired in the past.

   So just make this return an unsigned integral type, preferably just
   OM_uint32.

   So just rewrite as:

    static OM_uint32
    secondsUntil(int inputIsUTC, TimeStamp *time)
    {
        // time is local time
        ULARGE_INTEGER uiLocal;
        FILETIME now;
        GetSystemTimeAsFileTime(&now);
        if (!inputIsUTC) {
            FILETIME nowLocal;
            if (FileTimeToLocalFileTime(&now, &nowLocal) == 0) {
                return -1;
            }
            now = nowLocal;
        }
        uiLocal.HighPart = now.dwHighDateTime;
        uiLocal.LowPart = now.dwLowDateTime;
        long diff = (long)((time->QuadPart - uiLocal.QuadPart) / 10000000);
        if (diff < 0)
            return 0;
        if (sizeof(long) > sizeof(OM_uint32) && diff > (long)~(OM_uint32)0)
            return GSS_C_INDEFINITE;
        return diff;
    }

 - sspi.cpp:NewContext()

   Please fully-initialize all fields of the allocated context structure before
   returning it.

   Or else use new gss_ctx_id_struct() syntax, or specify a constructor.

 - sspi.cpp:169

   -->165     out->phCred = NULL;
      166     out->cbMaxMessage = pkgInfo->cbMaxToken;
      167     PP("pkgInfo->cbMaxToken: %ld", pkgInfo->cbMaxToken);
      168     FreeContextBuffer(pkgInfo);
   -->169     out->phCred = NULL;

   Dup code.

 - sspi.cpp:flagSspi2Gss() and flagGss2Sspi()

   I realize that this would be a change to the Java bindings, so this is just
   a note, but, we should add support for GSS_C_DELEG_POLICY_FLAG.

 - sspi.cpp:getTGTEndTime()

   Please immediately write to the `endTime' output parameter upon entry.

 - sspi.cpp:getTGTEndTime()

   You leak the logonHandle.  You have to close it with
   LsaDeregisterLogonProcess() before returning after LsaConnectUntrusted()
   returns successfully.

 - sspi.cpp:getTGTEndTime()

   Maybe just make this return OM_uint32 and simplify the one call site, making
   it return 86400 seconds (1 day) or whatever for the error case, else the
   secondsUntil() the time indicated by the LSA in response to the
   KerbRetrieveTicketMessage request.

 - sspi.cpp:316,355

   It is rather concerning that get_full_name() can return its input or a
   modified copy of its input.  At the call sites there's no check to see if
   the returned value is allocated, and it's hard to analyze if you're
   ultimately leaking or double-freeing.

   This isn't performance-critical code, so just always allocate a copy.

 - sspi.cpp:315

   314     // input has realm, no need to add one
   315     if (wcschr(input, '@')) {

   This is not going to work with UPNs.  You should probably implement proper
   RFC1864 name format support, including backslash-quoting.

 - sspi.cpp:315

   315     if (wcschr(input, '@')) {
                             ^

   I don't know if it is necessary that this be L'@' or if the compiler will do
   the right thing because the `wc' argument of `wcschr()' is a `wchar_t'.

   Elsewhere you do use wide character literals, so I assume even if type
   promotion works correctly for char->WCHAR (does it?) you might want to be
   consistent.

 - sspi.cpp:328

   328         // Solution #2: Or just use an empty string (and hope referral works).
   329         // This does not work now because ServicePermission check is based on
   330         // the exported name. Unless we update ServicePermission check to
   331         // be friendly with referral we need a realm.
   332         //realm = L"";

   Yes, ServicePermission is a bit of a disaster.  I hope after we integrate
   our contribution we can clean it up somewhat.

 - sspi.cpp:337

   337         realm = _wgetenv(L"USERDNSDOMAIN");

   This is actually probably the best thing to do.  Referrals should get chased
   from there.

   Or we could get the realm from the user's principal name and use that.

 - sspi.cpp:344

   There's no wide string version of strdup()??  Ugh.

   wchar_t/WCHAR is cancer.

   I worry that adding any more wchar_t-using code makes worse the legacy
   cleanup as Windows starts to prefer UTF-8.  But whatever.

 - sspi.cpp:351

   351     wcscpy_s(fullname + oldlen + 1, wcslen(realm) + 1, realm);

   It would be safer to use wcscat_s().

 - sspi.cpp:362

   362 #define CHECK_OUTPUT(x)  if (!x) return GSS_S_CALL_INACCESSIBLE_WRITE;

   I would much prefer the more standard and safer do { ... } while (0)
   construction:

    362 #define CHECK_OUTPUT(x)  \
        do { if (!x) return GSS_S_CALL_INACCESSIBLE_WRITE; } while (0)

   I would do this for all your macros.

   I'm also not sure it's worth having these macros.

 - sspi.cpp:398-405

   I don't believe there's an exported name token format for SPNEGO MNs.  In
   principle any MNs produced by SPNEGO should be for the negotiated mechanism.
   What you should do here instead is check that the exported name token is for
   Kerberos, and if not return an error immediately.

   And your gss_export_name() correctly always puts the Kerberos mechanism OID
   in exported name tokens.

 - sspi.cpp:412

   412     len = MultiByteToWideChar(CP_ACP, 0, input, len, value, len+1);
                                     ^^^^^^

   Please use CP_UTF8 when en-widening the string in an exported Kerberos name
   token.

 - sspi.cpp:465

   465     // Initialized as different
   466     *name_equal = 0;

   No need to comment the obvious :)

 - sspi.cpp:gss_compare_name()

   Just a note: your `struct gss_name_struct' doesn't keep track of import name
   type, so you can't check that here.  That's OK.  It's not worth trying to
   keep track of it -- I'm just pointing this out.

 - sspi.cpp:527

   527     int len = (int)wcslen(fullname);

   You don't need that cast.  But you should check first that `wcslen()' did
   not return a value larger than `INT_MAX'.

   You only even need an `int len' because `WideCharToMultiByte()' returns
   `int'...

   I'd write this like:

    527     int len;
            size_t namelen = wcslen(fullname);
            if (namelen > 255)
                goto err;
            // 04 01 00 ** 06 ** OID len:int32 name
            // ...
            //

 - sspi.cpp:545

   545         len = WideCharToMultiByte(CP_ACP, 0, fullname, len,
                                         ^^^^^^

   s/CP_ACP/CP_UTF8/

   It's clear that the intent of RFC2743 was to have name strings be in some
   standard encoding, not the locale's, though RFC2743 says "Latin-1", which
   isn't precise enough, and useless anyways.  Otherwise, if we needed to use
   the codepage being used by the application we'd have to first determine what
   that is.  The application here is Java, which will be using UTF-8 anyways,
   no? so just do that.  Definitely not ANSI.

 - sspi.cpp:532,574

   You can't assume the length of a multi-byte string will be the same number
   of `char' as the number of `wchar_t' in the wide-char version of the string.

   Determinining the correct length is a bit tricky, though since we should be
   converting to UTF-8 (see above), you can assume that 4x is enough, use
   4*len for the buffer allocation, then take the correct length from
   WideCharToMultiByte().

 - sspi.cpp:630-644

   This should be done after successfully acquiring a credential.  Before that,
   if actual_mechs != NULL then set *actual_mechs = GSS_C_NO_OID_SET.

 - sspi.cpp:gss_acquire_cred()

   It would be good to set *minor_status in more cases.

 - sspi.cpp:607

   606     TimeStamp ts;
   607     ts.QuadPart = 0;

   Huh?  TimeStamp is a typedef of SECURITY_INTEGER, and SECURITY_INTEGER is a
   struct of the same shape and size as FILETIME, and has no QuadPart field.

   What am I missing?

 - sspi.cpp:651

   651     ts.QuadPart = 0;

   You've already initialized this at 607.

 - sspi.cpp:704-711

   As mentioned above, you could move this all into a utility function.  And
   there's no need to support anything other than unsigned expiration times.
   GSS only uses OM_uint32 for those.  If a credential is expired you'll get
   GSS_S_NO_CRED, not a negative expiration time.  (You might get
   GSS_S_COMPLETE and a zero lifetime_rec, but that would be a race condition.)

 - sspi.cpp:767

   I would prefer that all output parameters be initialized early (to zero,
   GSS_C_NO_NAME, etc.) before any work is done that might cause failure.

 - sspi.cpp:813

   I would prefer that *minor_status is always set to something, even zero.

   Meaningful minor status values would be nice, but that can come later.

 - sspi.cpp:871

   s/CP_ACP/CP_UTF8/

 - sspi.cpp:870-872

   Check errors here:

   870     gss_display_name(&minor, target_name, &tn, NULL);

   Wrong buffer length here:

   871     int len = MultiByteToWideChar(CP_ACP, 0, (LPCCH)tn.value, (int)tn.length,
   872             outName, (int)tn.length);
                            ^^^^^^^^^^^^^^

   Replace that with with `sizeof(outName) - 1' please (the `- 1' is
   important).

   873     if (len == 0) {
   874         return GSS_S_FAILURE;
   875     }

 - sspi.cpp:897-898

   897     } else {
   898         if (!pc->phCred) {
   ...
   932         }
   933     }

   You can de-indent all of of 899-931 by re-writing as:

   897     } else if (!pc->phCred) {
   ...
   931     }

 - sspi.cpp:844

   844     if (input_token->length == 0) {

   Technically-speaking input_token can be NULL here.  Maybe the Java JNI
   bindings for GSS always pass in a token?  But I'd still change this to
   `input_token == NULL || input_token->length == 0'.

   Similarly at 889.  And 936.  ...

 - sspi.cpp:596

   596                  const gss_OID_set desired_mech,

   Nit: since it's an OID set, the name for this argument is `desired_mechs'.

 - sspi.cpp:841

   841     BOOLEAN isSPNEGO = isSameOID(mech_type, &SPNEGO_OID);

   Watch out!  mech_type can be GSS_C_NO_OID (NULL)!

   isSameOid() will dereference that.

   If the Java JNI bindings for GSS just happen to always pass in a mech_type,
   that's OK, but I think you should not assume this and be safe.

 - sspi.cpp:954

   Should you check if there's an output token before doing this:

   954         ss = CompleteAuthToken(&pc->hCtxt, &outBuffDesc);

   ?

 - sspi.cpp:874 and others

   delete `output_token->value' and set it to NULL (and the length to 0) before
   returning errors after 865.

 - sspi.cpp:878,962-963

   The local variable `pfDone' is set but not used.  Drop it.

   (The compiler should have warned about this, no?)

 - sspi.cpp:964-966

   Change this:

   964     outFlag = flagSspi2Gss(outFlag);
   965 
   966     *ret_flags = (OM_uint32)outFlag;

   to:

   964     *ret_flags = flagSspi2Gss(outFlag);

   You don't use outFlag again.  Setting it to a different kind of set of flags
   seems like a trap for someone to fall into in the future.

 - sspi.cpp:948

   948     if (!SEC_SUCCESS(ss)) {
   949         return GSS_S_FAILURE;
   950     }
   951 
   952     if ((SEC_I_COMPLETE_NEEDED == ss)
   953             || (SEC_I_COMPLETE_AND_CONTINUE == ss)) {

   I can't find documentation on SEC_SUCCES().  I don't know if it treats
   SEC_I_COMPLETE_NEEDED as an error or not...  I gues it must not.  Can you
   confirm?

 - sspi.cpp:967-970

   Does QueryContextAttributes() initialize its output parameter if it fails:

   967     // Ignore the result of the next call. Might fail before context established.
   968     QueryContextAttributes(
   969             &pc->hCtxt, SECPKG_ATTR_SIZES, &pc->SecPkgContextSizes);

   ?

   If not then you'll print garbage at 970:

   970     PP("cbMaxToken: %ld. cbMaxSignature: %ld. cbBlockSize: %ld. cbSecurityTrailer: %ld",
   971             pc->SecPkgContextSizes.cbMaxToken,
   972             pc->SecPkgContextSizes.cbMaxSignature,
   973             pc->SecPkgContextSizes.cbBlockSize,
   974             pc->SecPkgContextSizes.cbSecurityTrailer);

   Though if you properly initialize the context, then these should all be
   zeroes.

   Similarly at 979 and 980.

 - sspi.cpp:979-983

   Move 980:

       979         ss = QueryContextAttributes(&pc->hCtxt, SECPKG_ATTR_NATIVE_NAMES, &pc->nnames);
   --->980         PP("Names. %ls %ls", pc->nnames.sClientName, pc->nnames.sServerName);
       981         if (!SEC_SUCCESS(ss)) {
       982             return GSS_S_FAILURE;
       983         }

   to after the SEC_SUCCESS():

   979         ss = QueryContextAttributes(&pc->hCtxt, SECPKG_ATTR_NATIVE_NAMES, &pc->nnames);
   980         if (!SEC_SUCCESS(ss)) {
   981             return GSS_S_FAILURE;
   982         }
   983         PP("Names. %ls %ls", pc->nnames.sClientName, pc->nnames.sServerName);

 - sspi.cpp:1067

   Since you don't initialize output parameters early in this function, you
   might goto err and delete garbage:

   1067 err:
   1068     if (n1 != NULL) {
   1069         if (n1->name != NULL) {
   1070             delete[] n1->name;
   1071         }
   1072         delete n1;
   1073         n1 = NULL;
   1074     }
   1075     if (n2 != NULL) {
   1076         if (n2->name != NULL) {

   Just initialize all output parameters early.

 - sspi.cpp:1002

   It should be simple now to implement this...

   1002     PP(">>>> Calling UNIMPLEMENTED gss_accept_sec_context...");
   1003     return GSS_S_FAILURE;

 - gssapi.h:gss_acquire_cred()

        @@ -387,13 +399,13 @@
         
         /* Function Prototypes */
         
         GSS_DLLIMP OM_uint32 gss_acquire_cred(
                 OM_uint32 *,            /* minor_status */
        -        gss_name_t,             /* desired_name */
        +        gss_const_name_t,       /* desired_name */
                 OM_uint32,              /* time_req */
        -        gss_OID_set,            /* desired_mechs */
   ---->+        const gss_OID_set,      /* desired_mechs */
                 gss_cred_usage_t,       /* cred_usage */
                 gss_cred_id_t *,        /* output_cred_handle */
                 gss_OID_set *,          /* actual_mechs */
                 OM_uint32 *             /* time_rec */
         );

   That needs to be `gss_const_OID_set', not `const gss_OID_set'.

   There's a number of these.  Just search for "const gss_" -- any time you see
   that in a function prototype, it's wrong.

 - gssapi.h:gss_init_sec_context()

        @@ -403,100 +415,100 @@
                 gss_cred_id_t *         /* cred_handle */
         );
         
         GSS_DLLIMP OM_uint32 gss_init_sec_context(
                 OM_uint32 *,            /* minor_status */
        -        gss_cred_id_t,          /* claimant_cred_handle */
        +        gss_const_cred_id_t,    /* claimant_cred_handle */
                 gss_ctx_id_t *,         /* context_handle */
        -        gss_name_t,             /* target_name */
        -        gss_OID,                /* mech_type (used to be const) */
        +        gss_const_name_t,       /* target_name */
   ---->+        const gss_OID,          /* mech_type (used to be const) */
                 OM_uint32,              /* req_flags */
                 OM_uint32,              /* time_req */
        -        gss_channel_bindings_t, /* input_chan_bindings */
        -        gss_buffer_t,           /* input_token */
   ---->+        const gss_channel_bindings_t, /* input_chan_bindings */
   ---->+        const gss_buffer_t,           /* input_token */
                 gss_OID *,              /* actual_mech_type */
                 gss_buffer_t,           /* output_token */
                 OM_uint32 *,            /* ret_flags */
                 OM_uint32 *             /* time_rec */
         );

   These too need to be the correct gss_const_* types.

   I won't list all of these...

 - NativeFunc.h

   Same issues here.  E.g.,

        @@ -55,42 +55,42 @@
                                         (OM_uint32 *minor_status,
                                         gss_name_t *name);
         
         typedef OM_uint32 (*IMPORT_NAME_FN_PTR)
                                         (OM_uint32 *minor_status,
        -                                gss_buffer_t input_name_buffer,
        -                                gss_OID input_name_type,
   ---->+                                const gss_buffer_t input_name_buffer,
   ---->+                                const gss_OID input_name_type,
                                         gss_name_t *output_name);

   Though you got many right too:

         typedef OM_uint32 (*COMPARE_NAME_FN_PTR)
                                         (OM_uint32 *minor_status,
        -                                gss_name_t name1,
        -                                gss_name_t name2,
        +                                gss_const_name_t name1,
        +                                gss_const_name_t name2,
                                         int *name_equal);

Done.

This looks much better.  Just a bit more work!

Nico
-- 


More information about the security-dev mailing list