New webrev at https://cr.openjdk.java.net/~weijun/6722928/webrev.05, and interdiff at
https://cr.openjdk.java.net/~weijun/6722928/webrev.05/interdiff.patch.html I'll think more about secondsUntil() and getTGTEndTime(). Other comments below. > - sspi.cpp:61 > > 61 #define SEC_SUCCESS(status) (*minor_status = status, (status) >= > SEC_E_OK) > ^^^^^^^^^^^^^^^^^^^^^^ > > Please add parenthesis around the whole assignment expression. OK, but what kind of bad things could happen? > > - 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; > } OK. > > - sspi.cpp:NewContext() > > Please fully-initialize all fields of the allocated context structure before > returning it. Some of them are of struct types, do I need to go inside and set each to zero? Or ZeroMemory the whole struct? For example: typedef struct _SecHandle { ULONG_PTR dwLower; ULONG_PTR dwUpper; } SecHandle, * PSecHandle; > > 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. Ah yes. > > - 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. How? Always set it with GSS_C_DELEG_FLAG? > > - sspi.cpp:getTGTEndTime() > > Please immediately write to the `endTime' output parameter upon entry. Are you afraid of garbages for "return 1"s? But I won't touch the parameter at all in this case. > > - sspi.cpp:getTGTEndTime() > > You leak the logonHandle. You have to close it with > LsaDeregisterLogonProcess() before returning after LsaConnectUntrusted() > returns successfully. OK. > > - 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. I'll think about it. > > - 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. OK. > > - sspi.cpp:315 > > 314 // input has realm, no need to add one > 315 if (wcschr(input, '@')) { > > This is not going to work with UPNs. Why not? I just want to find out if there is already a realm. > You should probably implement proper > RFC1864 name format support, including backslash-quoting. Ahh... > > - 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. I'll made them all L'@'. > > - 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. There is _wcsdup() but I need to make it longer here. > > 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(). How is this unsafe? If using wcscat_s(), I'll need to add a '\0' after the '@' first. > > - 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 can do that, but why is this safer? > > I would do this for all your macros. > > I'm also not sure it's worth having these macros. You mean there is actually no need to check the arguments? > > - 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. OK. > > 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. OK. > > - sspi.cpp:465 > > 465 // Initialized as different > 466 *name_equal = 0; > > No need to comment the obvious :) Oh. > > - 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. There was a warning. > 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(). OK. > > - 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. OK. > > - 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? It compiles. I think it's a union of one Quad or two DWORD. > > - sspi.cpp:651 > > 651 ts.QuadPart = 0; > > You've already initialized this at 607. Oops. > > - 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.) See above. > > - 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. OK. > > - 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. OK. > > - sspi.cpp:871 > > s/CP_ACP/CP_UTF8/ OK. > > - 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). Yes. > > 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 } Good. > > - 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. ... Yes. > > - 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'. OK. > > - 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. I'll fix isSameOID(). > > - sspi.cpp:954 > > Should you check if there's an output token before doing this: > > 954 ss = CompleteAuthToken(&pc->hCtxt, &outBuffDesc); > > ? Does this matter? Do you think CompleteAuthToken cannot deal with no token? > > - sspi.cpp:874 and others > > delete `output_token->value' and set it to NULL (and the length to 0) before > returning errors after 865. OK. Is this just a good habit or do you know someone using it even at a failure? > > - sspi.cpp:878,962-963 > > The local variable `pfDone' is set but not used. Drop it. OK, > > (The compiler should have warned about this, no?) 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. Good. > > - 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? Not an error. I defined SEC_SUCCESS in this file. > > - 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. I see. Now I initialize them at newContext(). > > - 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); OK. > > - 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. Yes. > > - sspi.cpp:1002 > > It should be simple now to implement this... > > 1002 PP(">>>> Calling UNIMPLEMENTED gss_accept_sec_context..."); > 1003 return GSS_S_FAILURE; Not our goal this time, and I don't know how to test. Does server on Windows only run as services? > > - 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. I think I copied all of them from Heimdal. Do you mean Heimdal is not complete at using the types? Same below. Thanks, Max > > - 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 > --
