On Wed, Mar 20, 2019 at 10:20:28AM +0800, Weijun Wang wrote:
> 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
Thanks. I'll take a look.
> 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?
First, there's a matter of hygiene: you need to parenthesize `status`,
and you do in the second part but not the first. Second, assignments in
if statements need to be parenthesized to avoid compiler warnings -- and
this one is parenthesized, but who knows if a compiler might be confused
by the comma operator and emit a warning anyways.
> > - 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?
C does not guarantee that (void*)0 is an all-bits-zero pattern, but I
imagine all the supported Windows platforms do (certainly x86 and
x86_64). So ZeroMemory will do. For more portable code you should
ZeroMemory the struct then manually assign all the pointer values.
> > - 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?
No, we'd have to add the Java bindings too. Forget this request.
> > - 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.
Yes. It's just defensive programming.
> > - 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.
UPNs are of the form name\@domain@REALM. You're not checking for `\@`.
You can't search for `@` backwards either because realm names can
actually have `\@` as well in them.
> > You should probably implement proper
> > RFC1864 name format support, including backslash-quoting.
>
> Ahh...
Right :)
> > - 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'@'.
Or stop using wchar_t... :)
> > 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.
You wouldn't have to compute the offset to copy at. I call that safer.
> > - 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?
You don't have to worry about this
if (something)
CHECK_OUTPUT(out_arg);
--->else
other stuff
That `else` will bind to the `if` that `CHECK_OUTPUT()` expands into.
> You mean there is actually no need to check the arguments?
No, I mean that they don't save that much work.
> > - sspi.cpp:527
> >
> > 527 int len = (int)wcslen(fullname);
> >
> > You don't need that cast.
>
> There was a warning.
Yes, but see the comments that followed:
> > 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: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.
(And even this assumes that we Windows won't do normalization on the
string, though if they normalize to NFC, I think the number of
codepoints can't go up.)
> > - 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.
Ok.
> > - 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?
Hmm, probably not.
> > - 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?
It's perfectly safe to gss_release_buffer() after a failure. After all,
the failure might involve an error token.
> > - 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.
Why not.
> > - 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.
Huh?
> > - 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.
Oy, sorry!
> > - 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?
Do we have servers that run on Windows? I think we do. Our shim
(forked from SAP's) works for us.
> > - 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?
Yes. Thanks for noticing. I'll fix it.
Nico
--
