RFR 6722928: Support SSPI as a native GSS-API provider

[Valerie Peng]

Hi Max,

Here are more comments/questions on <sspi.cpp>:

I am a bit unclear on the handling of the gss qop and MS qop. There are 
a few places where you seem to treat them as the same thing. But there 
are also cases where they seem unrelated as you didn't set one using the 
other and vice versa.

For all new/malloc calls, check for null return value before proceeding 
further.

For gss APIs whose arguments are marked const, can we also do that here? 
I think it's clearer and helps code readability.

Below are function-specific comments.

- SecondsUntil: naming seems inconsistent, should start with lower case? 
Based on the MS doc that I found 
https://msdn.microsoft.com/en-us/library/ms724284(v=VS.85).aspx, 
TimeStamp is same as FILETIME which you then copy the two fields into a 
ULARGE_INTEGER to do the 64-bit arithmetic, but through out the code, 
you seems to treat ULARGE_INTEGER and FILETIME as interchangeable and 
directly access the QuadPart field. MS doc has "Do not cast a pointer to 
a *FILETIME* structure to either a *ULARGE_INTEGER** or *__int64** value 
because it can cause alignment faults on 64-bit Windows." In addition, 
the comment about AcquireCredentialsHandle may be incomplete, as 
gss_context_time also uses this method. -1 is returned if the 
FileTimeToLocalFileTime() call failed. Also it seems that this method 
assumes that the passed-in "time" is always beyond the current time 
(line 291 - 296) which may not always be true?

- gss_context_time: Shouldn't this method also check the tsExpiry value 
and return the |GSS_S_CONTEXT_EXPIRED error code if applicable? Also the 
time_rec should be 0 if context has already expired.
|

- gss_wrap_size_limit:  based on the MS doc example: 
https://docs.microsoft.com/en-us/windows/desktop/secauthn/encrypting-a-message, 
|it looks like the value is "cbMaximumMessage" 
of||SecPkgContext_StreamSizes. Where is the documentation for the||| 
context_handle and its cbMaxMessage field?

||

- gss_get_mic: Some places you use SEC _SUCCESS macro and some plain 
if-else block. What is the criteria? In the SEC_SUCCESS macro, it's 
clearer to use SEC_E_OK instead of 0? Are we sure that the context 
handle already has the cbMaxSignature value? I saw that you make this 
call inside gss_init_sec_context() when the context is established. But 
do we need to handle the case that a half-established context is passed 
in? Should not crash even if user does that, right?

- gss_verify_mic: qop_state is optional and may be null. You should 
check the value before dereferencing it. Line 1262, should be 
SECBUFFER_VERSION instead of 0?|Should we also check qop_state? |

- gss_wrap: Line 1347, since the order of data is 0-1-2, why the length 
addition is ordered 1-0-2? Seems more natural to use the same order. 
Move the assignment of conf_state (line 1329) after the if-block (line 
1331). In addition, conf_state is optional and may be null. You should 
check the value before dereferencing it.

|Will continue reviewing the rest.|
|Thanks,|
|Valerie|

|
|

||

On 12/12/2018 2:20 PM, Valerie Peng wrote:
>
> Hi Max,
>
> <sspi.cpp>
>
> - the DER related code is very hard to read... Would be nice to use 
> constants/enum for commonly used tag or use some method to construct them.
>
> - line 449, I think you mean to use "c" instead of "cred_handle"
>
> - gss_unwrap: add "const" to the 2nd and 3rd arguments? Isn't variable 
> naming convention starts with lower case? the argument qop_state may 
> be non-null but is not set?||
>
> - gss_indicate_mechs: the SSPI docs that I found mentioned that you 
> need to call FreeContextBuffer on pkgInfo after calling 
> QuerySecurityPackageInfo(). Local variable "minor" not used and can be 
> removed?
>
> - gss_inquire_names_for_mech: why does the PP output has "IMPLEMENTED" 
> wording, other methods do not. Is this intentional?
>
> - gss_create_empty_oid_set: do we need to check the specified oid_set 
> for existing content and free if not-empty before wiping it out? This 
> is called by a few other gss api methods also, it may be better to 
> defend against user errors.
>
> - gss_add_oid_set_member: add "const" to the 2nd argument?
>
> - gss_release_buffer: maybe set buffer->length = 0 outside the 
> if-block. Do we need to check for GSS_C_NO_BUFFER in addition to null?
>
> - gss_display_status: add "const" to the 4th argument? As for the 
> impl, I have a question, this particular method is for displaying text 
> output for gssapi error codes, but the FormatMessage() call takes 
> window specific message id. Are they the same?
>
> I am still going through the rest of sspi.cpp, but thought that I will 
> send you what I have first.
>
> Good that you have this targeted to 13 as there is almost no time left 
> for RFEs to get into JDK12.
>
> Thanks,
> Valerie
>
>
> On 11/19/2018 5:56 PM, Weijun Wang wrote:
>> Please take a review at
>>
>>     https://cr.openjdk.java.net/~weijun/6722928/webrev.01/
>>
>> We ported [1] the native GSS bridge to Windows in JDK 11, but user still have to download and install a native GSS-API library. This code change provides a native GSS-API library inside JDK that can be used without setting the "sun.security.jgss.lib" system property. It is based on Windows SSPI and now only supports the client side using the default credentials.
>>
>> No regression tests included. A Windows Active Directory server is needed.
>>
>> Thanks
>> Max
>>
>> [1]https://bugs.openjdk.java.net/browse/JDK-8200468
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181219/848f9ce2/attachment.htm>