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

Weijun Wang weijun.wang at oracle.com
Mon Jun 10 02:30:50 UTC 2019


Updated webrev at

   http://cr.openjdk.java.net/~weijun/6722928/webrev.08/

@build-dev guys, I realize I've never included you in this code review. Please take a look.

@Valerie, All comments accepted except for one (see below). In fact, I think I found a bug in gss_release_context that it might release a cred handle passed in, so I add a isLocalCred flag. However, I test it on my local Windows and it seems the same handle can be FreeCredentialsHandle and then used and then freed again.

> On Jun 7, 2019, at 10:45 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> Hi, Max,
> 
> <gssapi.h>
> 
> - line 424: the "(used to be const)" comment can now be removed.
> 
> <sspi.cpp>
> 
> - line 396-403: on line 338, there is no need to go to err as no memory has been allocated. What happens when jumping to err but the variables, i.e. value and name, have not been declared? Line 400-401 seems not used as there is no more goto err after line 391.
> 
> - line 528: the size of buffer here is 4*len + 1, but then when calling WideCharToMultiByte, the 6th argument is len. Seems inconsistent? line 534: shouldn't we free "buffer" here?
> 
> - line 596: free cred allocated on line 588? line 610 and 617: free cred and cred->phCredK? line 638 and 644, 648 and 653: free cred, cred->phCredK and cred->phCredS?
> 
> - line 829: free the context handle allocated on line 807? line 879: free newCred? line 901: no memory de-allocation before returning error? line 921: seems redundant given line 918.
> 
> - line 1071: based on gss api doc, context_handle should be set to GSS_C_NO_CONTEXT after deletion.
> 
> - line 1333: what about secBuff[1].pvBuffer?

According to the DecryptMessage spec, "The encrypted message is decrypted in place, overwriting the original contents of its buffer". I've printed out both secBuff[0].pvBuffer and secBuff[1].pvBuffer after the decryption and the latter is indeed inside the former.

> 
> - line 1390, 1393, 1397: call gss_release_oid_set before returning failure?
> 
> - line 1471: should we return an error code here when FormatMessage() call failed?
> 
> Rest looks fine.
> 
> Thanks,
> 
> Valerie

Thanks,
Max

[1] https://docs.microsoft.com/en-us/windows/desktop/api/sspi/nf-sspi-decryptmessage

> 
> On 6/4/2019 2:52 AM, Weijun Wang wrote:
>> I uploaded an updated webrev in place. The only changes are:
>> 
>> 1. s/SSPI_TRACE/SSPI_BRIDGE_TRACE/ in sspi.cpp
>> 2. Several copyright year updates.
>> 3. Remove one unused import.
>> 
>> Thanks,
>> Max
>> 
>>> On May 30, 2019, at 11:18 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>> 
>>> Here is the latest webrev
>>> 
>>>   http://cr.openjdk.java.net/~weijun/6722928/webrev.07/



More information about the security-dev mailing list