RFR 6722928: Support SSPI as a native GSS-API provider
Weijun Wang
weijun.wang at oracle.com
Wed Mar 20 02:20:28 UTC 2019
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
> --
More information about the security-dev
mailing list