[13] RFR 8225687: Newly added sspi.cpp in JDK-6722928 still contains some small errors

[Weijun Wang]

"[13]" added to the subject.

> On Jul 2, 2019, at 10:22 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> Please take a review at
> 
>   http://cr.openjdk.java.net/~weijun/8225687/webrev.00/
> 
> Most changes are around const usage in gssapi.h. Note that this is internal so the change could only prevent any coding error and will not be visible outside JDK.
> 
> Other changes are inside gss_import_name(), gss_compare_name(). I also changed some names like get_oid_set_desc -> show_oid_set.
> 
> Thanks,
> Max
> 
>> On Jun 13, 2019, at 10:48 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
>> 
>> I've filed https://bugs.openjdk.java.net/browse/JDK-8225687.
>> 
>>> On Jun 11, 2019, at 7:56 AM, Nico Williams <Nico.Williams at twosigma.com> wrote:
>>> 
>>> On Mon, Jun 10, 2019 at 10:30:50AM +0800, Weijun Wang wrote:
>>>> Updated webrev at
>>>> 
>>>> http://cr.openjdk.java.net/~weijun/6722928/webrev.08/
>>> 
>>> - src/java.security.jgss/share/classes/sun/security/jgss/spnego/NegTokenTarg.java
>>> 
>>> Ugh, I never noticed GSSUtil.useMSInterop().  This really should not
>>> be needed.  I believe a proper implementation of RFC4178 wouldn't
>>> need it.
>> 
>> I tried but scared away by MS-SPNG's appendix on which version of Windows supports which style of mechListMic. Now that Java only supports one underlying mechanism this field is looks not so crucial.
>> 
>>> 
>>> I can't expect you to do that, but can we have a bug opened for this?
>> 
>> There was one at https://bugs.openjdk.java.net/browse/JDK-6773898.
>> 
>>> 
>>> - src/java.security.jgss/share/classes/sun/security/jgss/wrapper/SunNativeProvider.java
>>> 
>>> Shouldn't the sspi_bridge.dll be in the lib directory?
>> 
>> The DLLs have always been in /bin. Maybe it's more easily loadable by java.exe.
>> 
>>> 
>>> - src/java.security.jgss/share/native/libj2gss/NativeFunc.h
>>> - src/java.security.jgss/share/native/libj2gss/gssapi.h
>>> 
>>> These continue to have improper constification.
>>> 
>>> There should be no function arguments of type "const gss_...".
>>> 
>>> I've a PR for Heimdal to fix this in Heimdal.  It was very
>>> mechanical: search the prototypes and function definitions for
>>> 
>>>     \<const[^I ]*gss_
>>> 
>>> then change that to gss_const_... then fix all the warnings and
>>> errors that came up when building the result.
>> 
>> OK.
>> 
>>> 
>>> - All remaining commentary will be about
>>> src/java.security.jgss/windows/native/libsspi_bridge/sspi.cpp
>>> 
>>> - CHECK_*() macros
>>> 
>>>  Macro bodies should not end in a semi-colon.
>> 
>> OK.
>> 
>>> 
>>>  If these were public and since these macro bodies are all if
>>>  statements, you should wrap them in do { } while (0), but since
>>>  they're private we can make sure that all uses are correct.
>>> 
>>> - gss_import_name() doesn't check that the first two bytes of the
>>>  input buffer are the expected token ID when the name-type is
>>>  GSS_C_NT_EXPORTED_NAME.
>> 
>> OK.
>> 
>>> 
>>> - gss_import_name() doesn't check that the third byte of the input
>>>  buffer is 0 when the name-type is GSS_C_NT_EXPORTED_NAME.
>> 
>> OK.
>> 
>>> 
>>> - gss_compare_name(), this code will not distinguish a name of the
>>>  form 'foo@' from 'foo\@'
>>> 
>>>     434     if (l1 < l2 && n2[l1] != L'@'
>>>     435             || l2 < l1 && n1[l2] != L'@') {
>>>     436         return GSS_S_COMPLETE; // different
>>>     437     }
>>> 
>>>  Honestly, this is not the most serious problem because nothing
>>>  really should be using gss_compare_name(), but you do use it... and
>>>  anyways, it's wrong.
>> 
>> Yes, it's called in GSSLibStub.c and those are the functions I must support.
>> 
>>> 
>>>  Perhaps the gss_name_struct struct should have a length of realm or
>>>  length-of-not-realm-part field to make this check easier.
>>> 
>>> - gss_compare_name(), do not use NORM_IGNORECASE
>> 
>> But I do notice that "Administrator" can also be named "administrator", and "HTTP/host" can also be "http/host".
>> 
>>> 
>>> - gss_canonicalize_name() should check that mech_type is Kerberos
>> 
>> OK.
>> 
>>> 
>>> I'll continue later.  I'm in gss_init_sec_context(), about 58% of the
>>> way through.
>> 
>> Please.
>> 
>> Thanks,
>> Max
>> 
>>> 
>>> Nico
>>> -- 
>> 
>