RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

Alexey Bakhtin alexey at azul.com
Wed May 27 09:42:20 UTC 2020


Hello Valerie,

Unfortunately, Windows LDAP server with LdapEnforceChannelBinding=2 does not accept GSS_C_AF_NULLADDR address type.
This is exact reason of these changes.
I’ve tried to fix inconsistency of address type value in the latest webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v2/
This fix allows to set GSS_C_AF_UNSPEC address type for the tls-server-end-point channel binding only

Regards
Alexey

> On 26 May 2020, at 23:37, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> I am also concerned about the changes in GSSLibStub.c about the default value being GSS_C_AF_UNSPEC instead of GSS_C_AF_NULLADDR (line 194-195).
> 
> Can you try and see if Window works with GSS_C_AF_NULLADDR? If yes, I'd prefer to not changing the default value for addresstype for the same reason which Michael has already stated.
> 
> Thanks,
> Valerie
> 
> 
> On 5/25/2020 8:33 AM, Alexey Bakhtin wrote:
>> Hello Michael, Thomas,
>> 
>> Thank you a lot for review and suggestions.
>> I’ve fixed most of the issues except of fundamental one
>> I need more time to evaluate suggested usage of UnspecEmptyInetAddress subtype.
>> 
>> Updated webrev is available at:
>> http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/
>> 
>> 
>> Also, please see my comments below.
>> 
>> Regards
>> Alexey
>> 
>> 
>>> On 24 May 2020, at 02:38, Michael Osipov <1983-01-06 at gmx.net>
>>>  wrote:
>>> 
>>> Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:
>>> 
>>>> Hello,
>>>> 
>>>> Could you please review the following patch:
>>>> 
>>>> JBS:
>>>> https://bugs.openjdk.java.net/browse/JDK-8245527
>>>> 
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
>>> Let's go through your changes statically:
>>> 
>>> * The JIRA issue title has a typo
>>> 
>> Thank you. Fixed in Jira
>> 
>>> * The word "cannot" is incorrectly spelled throughout all exception messages
>>> 
>> Fixed from “can not” to “cannot"
>> 
>>>> +            if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
>>>> +                throw new UnsupportedOperationException("LdapCtx: " +
>>>> +                        TlsChannelBindingType.TLS_UNIQUE.getName() + " type is not supported");
>>>> 
>>> 
>>> "LdapCtx: " is redundant because the stacktrace will contain the class
>>> name already. A better message would be: "Channel binding type '%s' is
>>> not supported". Not just the plain value.
>>> 
>> Exception message is corrected
>> 
>>>> +            } else if (cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
>>>> +                if (connectTimeout == -1)
>>>> +                    throw new IllegalArgumentException(CHANNEL_BINDING_TYPE + " property requires " +
>>>> +                            CONNECT_TIMEOUT + " property is set.");
>>>> 
>>> * Same here with the message
>>> 
>> Not sure, What’s wrong with the message ?
>> 
>>> * The IAE is wrong because passed value is correct, but leads to an
>>> invalid state because connection timeout is -1. You need an
>>> IllegalStateException here.
>>> 
>> Thank you. You are right again. Changed to IllegalStateException
>> 
>>> Stupid question: how can one create a GSS security context when the TLS
>>> security context has not been established yet?
>>> 
>> This logic already existed here. It could be a reason for it and I don’t want change it without strong purpose.
>> The only changes here is to prevent double setting of channel binding data.
>> 
>> 
>>>> --- old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java	2020-05-18 19:39:46.000000000 +0300
>>>> +++ new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java	2020-05-18 19:39:46.000000000 +0300
>>>> @@ -531,9 +531,12 @@
>>>>     public void setChannelBinding(ChannelBinding channelBindings)
>>>>         throws GSSException {
>>>> 
>>>> -        if (mechCtxt == null)
>>>> +        if (mechCtxt == null) {
>>>> +            if (this.channelBindings  != null) {
>>>> +                throw new GSSException(GSSException.BAD_BINDINGS);
>>>> +            }
>>>>             this.channelBindings = channelBindings;
>>>> -
>>>> +        }
>>>>     }
>>>> 
>>> I don't understand the purpose of this hunk. Is this safeguard to set
>>> bindings only once?
>>> 
>>> 
>>>>     private static final int CHANNEL_BINDING_AF_INET = 2;
>>>>     private static final int CHANNEL_BINDING_AF_INET6 = 24;
>>>>     private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
>>>> +    private static final int CHANNEL_BINDING_AF_UNSPEC = 0;
>>>> 
>>> This should sort from 0 to 255 and not at the end.
>>> 
>> OK. Moved to the top.
>> 
>> 
>>>>     private int getAddrType(InetAddress addr) {
>>>> -        int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
>>>> +        int addressType = CHANNEL_BINDING_AF_UNSPEC;
>>>> 
>>>>   // initialize addrtype in CB first
>>>> -  cb->initiator_addrtype = GSS_C_AF_NULLADDR;
>>>> -  cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
>>>> +  cb->initiator_addrtype = GSS_C_AF_UNSPEC;
>>>> +  cb->acceptor_addrtype = GSS_C_AF_UNSPEC;
>>>> 
>>> This looks wrong to me -- as you already mentioned -- this violates RFC
>>> 2744, section 3.11, last sentence:
>>> 
>>>> or omit addressing information, specifying
>>>>   GSS_C_AF_NULLADDR as the address-types.
>>>> 
>>>>   /* release initiator address */
>>>> -  if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
>>>> +  if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
>>>> +      cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
>>>>     resetGSSBuffer(&(cb->initiator_address));
>>>>   }
>>>>   /* release acceptor address */
>>>> -  if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
>>>> +  if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
>>>> +      cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
>>>>     resetGSSBuffer(&(cb->acceptor_address));
>>>>   }
>>>> 
>>> Unspecified does not mean that it is null.
>>> 
>>> 
>>>> +                                final byte[] prefix = (TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
>>>> +                                byte[] cbData =  Arrays.copyOf(prefix,
>>>> +                                        prefix.length + tlsCB.getData().length );
>>>> +                                System.arraycopy(tlsCB.getData(), 0, cbData,  prefix.length, tlsCB.getData().length);
>>>> 
>>> Since you are calling "tlsCB.getData()" multiple times, this should go
>>> into a variable.
>>> 
>> Temporary variable is added
>> 
>> 
>>> 
>>>> +                                secCtx.setChannelBinding(new
>>>> 
>>> ChannelBinding(null, null, cbData));
>>> 
>>> Why not use new ChannelBinding(cbData)?
>>> 
>> Right. updated
>> 
>> 
>>>> +            String hashAlg = serverCertificate.getSigAlgName().
>>>> +                    replace("SHA", "SHA-").toUpperCase();
>>>> +            int ind = hashAlg.indexOf("WITH");
>>>> +            if (ind > 0) {
>>>> +                hashAlg = hashAlg.substring(0, ind);
>>>> +                if (hashAlg.equals("MD5") || hashAlg.equals("SHA-1")) {
>>>> +                    hashAlg = "SHA-256";
>>>> +                }
>>>> 
>>> I have several problems with that, some might be hypothetical:
>>> 
>>> * toUpperCase() should be qualified with Locale.ROOT or Locate.ENGLISH
>>> 
>> As you mentioned below AlgorithmId.getName() uses nameTable to return algorithm name.
>> Looking at implementation I do not think it is realistic to get name in the non-English locale.
>> I do not think it should be fixed
>> 
>> 
>>> * Looking at https://tools.ietf.org/html/rfc5280#section-4.1.1.2
>>> , then
>>> at sun.security.x509.AlgorithmId.getName() it uses nameTable to
>>> translate OIDs to readible names.
>>> 
>>> With indexOf("WITH") you are implying that the cert's sig alg may not
>>> use a cryptographic function, but this would mean that if the OID is
>>> 1.3.14.3.2.26 you'd turn "SHA-X" into "SHA--X" according to nameTable,
>>> wouldn't you?
>>> I also don't know how realistic "PBEWith..." is.
>>> 
>>> This likely needs more thought or I am missing something.
>>> 
>>> 
>> I do not think it is realistic scenario to have certificate signature algorithm without crypto function.
>> indexOf(“WITH”) just used to find end out hash algorithm name.
>> 
>> 
>>> * Exception messages are inconsistent. Sometimes "TLS channel binding",
>>> sometimes just "channel binding". To avoid misunderstandings it should
>>> always read "TLS channel binding..".
>>> 
>>> 
>> Messages are updated.
>> 
>> 
>>> Generally, I have two fundemental issues:
>>> * How do you know that address type must be UNSPEC and not NULLADDR?
>>> Trial and error?
>>> 
>> I did not find any strict specification about TLS Channel Binding format in Windows server.
>> So, it was a lot of experiments, reading related forums and docs.
>> One of the doc, that turns me to try UNSPEC type is the following:
>> 
>> https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication
>> 
>> 
>> 
>>> * Changing the default address type against the RFC will break every
>>> installation using channel bindings relying on it with cross GSS-API
>>> implementations.
>>> 
>> I do not like this conflict between UNSPEC and NULLADDR types too, but I do not know How to better solve it.
>> The usage of UnspecEmptyInetAddress subtype is not quite clear to me yet. Who set this value and will it change org.ietf.jgss.ChannelBinding public api ? As I understand, Implementation cannot decide (without pplication request), What channel binding type is enabled on the server.
>> 
>> 
>>> There must be another way to solve this. A system property would also be
>>> problematic because if you have a product which connects to MS and
>>> non-MS backends at the same time, channel bindings would be broken again.
>>> 
>>> What about introducing a UnspecEmptyInetAddress() which gives the proper
>>> AF type, but #getAddress() would return null? This should satisfy the
>>> requirements, InitialToken as well as the RFC. this of course needs to
>>> be properly passed to native providers too. GssKrb5Client would also
>>> need to know that this channel binding is explicitly for Active
>>> Directory and not some other, spec-compliant, SASL peer (property on
>>> LdapCtx?)
>>> 
>>> One could easily use this for custom code with a SSLServerSocket paired
>>> with Java SASL or in C, OpenSSL and Cyrus SASL.
>>> 
>>> Michael
>>> 
>>> PS: Yes, I am a nitpicker.
>>> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200527/137de485/signature.asc>


More information about the security-dev mailing list