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
--
