I've filed https://bugs.openjdk.java.net/browse/JDK-8225687.
> On Jun 11, 2019, at 7:56 AM, Nico Williams <nico.willi...@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 > --
