7067974: multiple ETYPE-INFO-ENTRY with same etype and different salt

Reviewed-by: valeriep

src/share/classes/sun/security/krb5/EncryptionKey.java

@@ -150,12 +150,37 @@ public class EncryptionKey
         return ktab.readServiceKeys(princ);
     }
 
+    /**
+     * Obtains a key for a given etype of a principal with possible new salt
+     * and s2kparams
+     * @param cname NOT null
+     * @param password NOT null
+     * @param etype
+     * @param snp can be NULL
+     * @returns never null
+     */
+    public static EncryptionKey acquireSecretKey(PrincipalName cname,
+            char[] password, int etype, PAData.SaltAndParams snp)
+            throws KrbException {
+        String salt;
+        byte[] s2kparams;
+        if (snp != null) {
+            salt = snp.salt != null ? snp.salt : cname.getSalt();
+            s2kparams = snp.params;
+        } else {
+            salt = cname.getSalt();
+            s2kparams = null;
+        }
+        return acquireSecretKey(password, salt, etype, s2kparams);
+    }
+
     /**
      * Obtains a key for a given etype with salt and optional s2kparams
      * @param password NOT null
      * @param salt NOT null
      * @param etype
      * @param s2kparams can be NULL
+     * @returns never null
      */
     public static EncryptionKey acquireSecretKey(char[] password,
             String salt, int etype, byte[] s2kparams)

src/share/classes/sun/security/krb5/KrbAsRep.java

@@ -131,13 +131,11 @@ class KrbAsRep extends KrbKdcRep {
             KrbAsReq asReq, PrincipalName cname)
             throws KrbException, Asn1Exception, IOException {
         int encPartKeyType = rep.encPart.getEType();
-        PAData.SaltAndParams snp =
-                PAData.getSaltAndParams(encPartKeyType, rep.pAData);
-        EncryptionKey dkey = null;
-        dkey = EncryptionKey.acquireSecretKey(password,
-                snp.salt == null ? cname.getSalt() : snp.salt,
+        EncryptionKey dkey = EncryptionKey.acquireSecretKey(
+                cname,
+                password,
                 encPartKeyType,
-                snp.params);
+                PAData.getSaltAndParams(encPartKeyType, rep.pAData));
         decrypt(dkey, asReq);
     }
 

src/share/classes/sun/security/krb5/KrbAsReqBuilder.java

@@ -169,35 +169,45 @@ public final class KrbAsReqBuilder {
              * from a keytab on acceptor, but unfortunately (?) Java supports
              * acceptor using password. In this case, if the service ticket is
              * encrypted using an etype which we don't have PA-DATA new salt,
-             * using the default salt is normally wrong (say, case-insensitive
+             * using the default salt might be wrong (say, case-insensitive
              * user name). Instead, we would use the new salt of another etype.
              */
 
             String salt = null;     // the saved new salt
+            try {
                 for (int i=0; i<eTypes.length; i++) {
+                    // First round, only calculate those have a PA entry
                     PAData.SaltAndParams snp =
                             PAData.getSaltAndParams(eTypes[i], paList);
-                // First round, only calculate those with new salt
-                if (snp.salt != null) {
+                    if (snp != null) {
+                        // Never uses a salt for rc4-hmac, it does not use
+                        // a salt at all
+                        if (eTypes[i] != EncryptedData.ETYPE_ARCFOUR_HMAC &&
+                                snp.salt != null) {
                             salt = snp.salt;
-                    result[i] = EncryptionKey.acquireSecretKey(password,
-                            snp.salt,
+                        }
+                        result[i] = EncryptionKey.acquireSecretKey(cname,
+                                password,
                                 eTypes[i],
-                            snp.params);
+                                snp);
                     }
                 }
+                // No new salt from PA, maybe empty, maybe only rc4-hmac
                 if (salt == null) salt = cname.getSalt();
                 for (int i=0; i<eTypes.length; i++) {
-                // Second round, calculate those with no new salt
+                    // Second round, calculate those with no PA entry
                     if (result[i] == null) {
-                    PAData.SaltAndParams snp =
-                            PAData.getSaltAndParams(eTypes[i], paList);
                         result[i] = EncryptionKey.acquireSecretKey(password,
                                 salt,
                                 eTypes[i],
-                            snp.params);
+                                null);
                     }
                 }
+            } catch (IOException ioe) {
+                KrbException ke = new KrbException(Krb5.ASN1_PARSE_ERROR);
+                ke.initCause(ioe);
+                throw ke;
+            }
             return result;
         } else {
             throw new IllegalStateException("Required password not provided");
@@ -315,27 +325,19 @@ public final class KrbAsReqBuilder {
                     }
                     preAuthFailedOnce = true;
                     KRBError kerr = ke.getError();
+                    int paEType = PAData.getPreferredEType(kerr.getPA(),
+                            EType.getDefaults("default_tkt_enctypes")[0]);
                     if (password == null) {
                         EncryptionKey[] ks = Krb5Util.keysFromJavaxKeyTab(ktab, cname);
-                        pakey = EncryptionKey.findKey(kerr.getEType(), ks);
+                        pakey = EncryptionKey.findKey(paEType, ks);
                         if (pakey != null) pakey = (EncryptionKey)pakey.clone();
                         for (EncryptionKey k: ks) k.destroy();
                     } else {
-                        PAData.SaltAndParams snp = PAData.getSaltAndParams(
-                                kerr.getEType(), kerr.getPA());
-                        if (kerr.getEType() == 0) {
-                            // Possible if PA-PW-SALT is in KRB-ERROR. RFC
-                            // does not recommend this
-                            pakey = EncryptionKey.acquireSecretKey(password,
-                                    snp.salt == null ? cname.getSalt() : snp.salt,
-                                    EType.getDefaults("default_tkt_enctypes")[0],
-                                    null);
-                        } else {
-                            pakey = EncryptionKey.acquireSecretKey(password,
-                                    snp.salt == null ? cname.getSalt() : snp.salt,
-                                    kerr.getEType(),
-                                    snp.params);
-                        }
+                        pakey = EncryptionKey.acquireSecretKey(cname,
+                                password,
+                                paEType,
+                                PAData.getSaltAndParams(
+                                    paEType, kerr.getPA()));
                     }
                     paList = kerr.getPA();  // Update current paList
                 } else {

src/share/classes/sun/security/krb5/internal/KRBError.java

@@ -99,7 +99,6 @@ public class KRBError implements java.io.Serializable {
     private Checksum eCksum; //optional
 
     private PAData[] pa;    // PA-DATA in eData
-    private int pa_eType;   // The 1st etype appeared in salt-related PAData
 
     private static boolean DEBUG = Krb5.DEBUG;
 
@@ -266,50 +265,8 @@ public class KRBError implements java.io.Serializable {
             DerValue tmp = derPA.data.getDerValue();
             PAData pa_data = new PAData(tmp);
             paList.add(pa_data);
-            int pa_type = pa_data.getType();
-            byte[] pa_value = pa_data.getValue();
             if (DEBUG) {
-                System.out.println(">>>Pre-Authentication Data:");
-                System.out.println("\t PA-DATA type = " + pa_type);
-            }
-
-            switch(pa_type) {
-                case Krb5.PA_ENC_TIMESTAMP:
-                    if (DEBUG) {
-                        System.out.println("\t PA-ENC-TIMESTAMP");
-                    }
-                    break;
-                case Krb5.PA_ETYPE_INFO:
-                    if (pa_value != null) {
-                        DerValue der = new DerValue(pa_value);
-                        while (der.data.available() > 0) {
-                            DerValue value = der.data.getDerValue();
-                            ETypeInfo info = new ETypeInfo(value);
-                            if (pa_eType == 0) pa_eType = info.getEType();
-                            if (DEBUG) {
-                                System.out.println("\t PA-ETYPE-INFO etype = " + info.getEType());
-                                System.out.println("\t PA-ETYPE-INFO salt = " + info.getSalt());
-                            }
-                        }
-                    }
-                    break;
-                case Krb5.PA_ETYPE_INFO2:
-                    if (pa_value != null) {
-                        DerValue der = new DerValue(pa_value);
-                        while (der.data.available() > 0) {
-                            DerValue value = der.data.getDerValue();
-                            ETypeInfo2 info2 = new ETypeInfo2(value);
-                            if (pa_eType == 0) pa_eType = info2.getEType();
-                            if (DEBUG) {
-                                System.out.println("\t PA-ETYPE-INFO2 etype = " + info2.getEType());
-                                System.out.println("\t PA-ETYPE-INFO2 salt = " + info2.getSalt());
-                            }
-                        }
-                    }
-                    break;
-                default:
-                    // Unknown Pre-auth type
-                    break;
+                System.out.println(pa_data);
             }
         }
         pa = paList.toArray(new PAData[paList.size()]);
@@ -340,10 +297,6 @@ public class KRBError implements java.io.Serializable {
         return pa;
     }
 
-    public final int getEType() {
-        return pa_eType;
-    }
-
     public final String getErrorString() {
         return eText;
     }

src/share/classes/sun/security/krb5/internal/PAData.java

@@ -138,10 +138,57 @@ public class PAData {
         return ((pADataValue == null) ? null : pADataValue.clone());
     }
 
+    /**
+     * Gets the preferred etype from the PAData array.
+     * 1. ETYPE-INFO2-ENTRY with unknown s2kparams ignored
+     * 2. ETYPE-INFO2 preferred to ETYPE-INFO
+     * 3. multiple entries for same etype in one PA-DATA, use the first one.
+     * 4. Multiple PA-DATA with same type, choose the last one
+     * (This is useful when PA-DATAs from KRB-ERROR and AS-REP are combined).
+     * @return the etype, or defaultEType if not enough info
+     * @throws Asn1Exception|IOException if there is an encoding error
+     */
+    public static int getPreferredEType(PAData[] pas, int defaultEType)
+            throws IOException, Asn1Exception {
+
+        if (pas == null) return defaultEType;
+
+        DerValue d = null, d2 = null;
+        for (PAData p: pas) {
+            if (p.getValue() == null) continue;
+            switch (p.getType()) {
+                case Krb5.PA_ETYPE_INFO:
+                    d = new DerValue(p.getValue());
+                    break;
+                case Krb5.PA_ETYPE_INFO2:
+                    d2 = new DerValue(p.getValue());
+                    break;
+            }
+        }
+        if (d2 != null) {
+            while (d2.data.available() > 0) {
+                DerValue value = d2.data.getDerValue();
+                ETypeInfo2 tmp = new ETypeInfo2(value);
+                if (tmp.getParams() == null) {
+                    // we don't support non-null s2kparams
+                    return tmp.getEType();
+                }
+            }
+        }
+        if (d != null) {
+            while (d.data.available() > 0) {
+                DerValue value = d.data.getDerValue();
+                ETypeInfo tmp = new ETypeInfo(value);
+                return tmp.getEType();
+            }
+        }
+        return defaultEType;
+    }
+
     /**
      * A place to store a pair of salt and s2kparams.
-     * An empty salt is changed to null, to be interopable
-     * with Windows 2000 server.
+     * An empty salt is changed to null, to be interoperable
+     * with Windows 2000 server. This is in fact not correct.
      */
     public static class SaltAndParams {
         public final String salt;
@@ -155,57 +202,120 @@ public class PAData {
 
     /**
      * Fetches salt and s2kparams value for eType in a series of PA-DATAs.
-     * The preference order is PA-ETYPE-INFO2 > PA-ETYPE-INFO > PA-PW-SALT.
-     * If multiple PA-DATA for the same etype appears, use the last one.
+     * 1. ETYPE-INFO2-ENTRY with unknown s2kparams ignored
+     * 2. PA-ETYPE-INFO2 preferred to PA-ETYPE-INFO preferred to PA-PW-SALT.
+     * 3. multiple entries for same etype in one PA-DATA, use the first one.
+     * 4. Multiple PA-DATA with same type, choose the last one
      * (This is useful when PA-DATAs from KRB-ERROR and AS-REP are combined).
-     * @return salt and s2kparams. never null, its field might be null.
+     * @return salt and s2kparams. can be null if not found
      */
     public static SaltAndParams getSaltAndParams(int eType, PAData[] pas)
-            throws Asn1Exception, KrbException {
+            throws Asn1Exception, IOException {
 
-        if (pas == null || pas.length == 0) {
-            return new SaltAndParams(null, null);
-        }
+        if (pas == null) return null;
 
+        DerValue d = null, d2 = null;
         String paPwSalt = null;
-        ETypeInfo2 info2 = null;
-        ETypeInfo info = null;
 
         for (PAData p: pas) {
-            if (p.getValue() != null) {
-                try {
+            if (p.getValue() == null) continue;
             switch (p.getType()) {
                 case Krb5.PA_PW_SALT:
                     paPwSalt = new String(p.getValue(),
                             KerberosString.MSNAME?"UTF8":"8859_1");
                     break;
                 case Krb5.PA_ETYPE_INFO:
-                            DerValue der = new DerValue(p.getValue());
-                            while (der.data.available() > 0) {
-                                DerValue value = der.data.getDerValue();
+                    d = new DerValue(p.getValue());
+                    break;
+                case Krb5.PA_ETYPE_INFO2:
+                    d2 = new DerValue(p.getValue());
+                    break;
+            }
+        }
+        if (d2 != null) {
+            while (d2.data.available() > 0) {
+                DerValue value = d2.data.getDerValue();
+                ETypeInfo2 tmp = new ETypeInfo2(value);
+                if (tmp.getParams() == null && tmp.getEType() == eType) {
+                    // we don't support non-null s2kparams
+                    return new SaltAndParams(tmp.getSalt(), tmp.getParams());
+                }
+            }
+        }
+        if (d != null) {
+            while (d.data.available() > 0) {
+                DerValue value = d.data.getDerValue();
                 ETypeInfo tmp = new ETypeInfo(value);
-                                if (tmp.getEType() == eType) info = tmp;
+                if (tmp.getEType() == eType) {
+                    return new SaltAndParams(tmp.getSalt(), null);
+                }
             }
+        }
+        if (paPwSalt != null) {
+            return new SaltAndParams(paPwSalt, null);
+        }
+        return null;
+    }
+
+    @Override
+    public String toString(){
+        StringBuilder sb = new StringBuilder();
+        sb.append(">>>Pre-Authentication Data:\n\t PA-DATA type = ")
+                .append(pADataType).append('\n');
+
+        switch(pADataType) {
+            case Krb5.PA_ENC_TIMESTAMP:
+                sb.append("\t PA-ENC-TIMESTAMP");
                 break;
-                        case Krb5.PA_ETYPE_INFO2:
-                            der = new DerValue(p.getValue());
+            case Krb5.PA_ETYPE_INFO:
+                if (pADataValue != null) {
+                    try {
+                        DerValue der = new DerValue(pADataValue);
                         while (der.data.available() > 0) {
                             DerValue value = der.data.getDerValue();
-                                ETypeInfo2 tmp = new ETypeInfo2(value);
-                                if (tmp.getEType() == eType) info2 = tmp;
+                            ETypeInfo info = new ETypeInfo(value);
+                            sb.append("\t PA-ETYPE-INFO etype = ")
+                                    .append(info.getEType())
+                                    .append(", salt = ")
+                                    .append(info.getSalt())
+                                    .append('\n');
+                        }
+                    } catch (IOException|Asn1Exception e) {
+                        sb.append("\t <Unparseable PA-ETYPE-INFO>\n");
+                    }
                 }
                 break;
+            case Krb5.PA_ETYPE_INFO2:
+                if (pADataValue != null) {
+                    try {
+                        DerValue der = new DerValue(pADataValue);
+                        while (der.data.available() > 0) {
+                            DerValue value = der.data.getDerValue();
+                            ETypeInfo2 info2 = new ETypeInfo2(value);
+                            sb.append("\t PA-ETYPE-INFO2 etype = ")
+                                    .append(info2.getEType())
+                                    .append(", salt = ")
+                                    .append(info2.getSalt())
+                                    .append(", s2kparams = ");
+                            byte[] s2kparams = info2.getParams();
+                            if (s2kparams == null) {
+                                sb.append("null\n");
+                            } else if (s2kparams.length == 0) {
+                                sb.append("empty\n");
+                            } else {
+                                sb.append(new sun.misc.HexDumpEncoder()
+                                        .encodeBuffer(s2kparams));
                             }
-                } catch (IOException ioe) {
-                    // Ignored
                         }
+                    } catch (IOException|Asn1Exception e) {
+                        sb.append("\t <Unparseable PA-ETYPE-INFO>\n");
                     }
                 }
-        if (info2 != null) {
-            return new SaltAndParams(info2.getSalt(), info2.getParams());
-        } else if (info != null) {
-            return new SaltAndParams(info.getSalt(), null);
+                break;
+            default:
+                // Unknown Pre-auth type
+                break;
         }
-        return new SaltAndParams(paPwSalt, null);
+        return sb.toString();
     }
 }

test/sun/security/krb5/auto/DupEtypes.java

+/*
+ * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 7067974
+ * @summary multiple ETYPE-INFO-ENTRY with same etype and different salt
+ * @compile -XDignore.symbol.file DupEtypes.java
+ * @run main/othervm DupEtypes 1
+ * @run main/othervm DupEtypes 2
+ * @run main/othervm/fail DupEtypes 3
+ * @run main/othervm DupEtypes 4
+ * @run main/othervm DupEtypes 5
+ */
+
+import sun.security.jgss.GSSUtil;
+
+public class DupEtypes {
+
+    public static void main(String[] args) throws Exception {
+
+        OneKDC kdc = new OneKDC(null);
+        kdc.writeJAASConf();
+
+        // Different test cases, read KDC.processAsReq for details
+        kdc.setOption(KDC.Option.DUP_ETYPE, Integer.parseInt(args[0]));
+
+        Context c, s;
+        c = Context.fromJAAS("client");
+        s = Context.fromJAAS("server");
+
+        c.startAsClient(OneKDC.SERVER, GSSUtil.GSS_KRB5_MECH_OID);
+        s.startAsServer(GSSUtil.GSS_KRB5_MECH_OID);
+
+        Context.handshake(c, s);
+
+        Context.transmit("i say high --", c, s);
+        Context.transmit("   you say low", s, c);
+
+        s.dispose();
+        c.dispose();
+    }
+}

test/sun/security/krb5/auto/KDC.java

@@ -174,6 +174,10 @@ public class KDC {
          * Set all name-type to a value in response
          */
         RESP_NT,
+        /**
+         * Multiple ETYPE-INFO-ENTRY with same etype but different salt
+         */
+        DUP_ETYPE,
     };
 
     static {
@@ -881,6 +885,57 @@ public class KDC {
             bFlags[Krb5.TKT_OPTS_INITIAL] = true;
 
             // Creating PA-DATA
+            DerValue[] pas2 = null, pas = null;
+            if (options.containsKey(KDC.Option.DUP_ETYPE)) {
+                int n = (Integer)options.get(KDC.Option.DUP_ETYPE);
+                switch (n) {
+                    case 1:     // customer's case in 7067974
+                        pas2 = new DerValue[] {
+                            new DerValue(new ETypeInfo2(1, null, null).asn1Encode()),
+                            new DerValue(new ETypeInfo2(1, "", null).asn1Encode()),
+                            new DerValue(new ETypeInfo2(1, OneKDC.REALM, new byte[]{1}).asn1Encode()),
+                        };
+                        pas = new DerValue[] {
+                            new DerValue(new ETypeInfo(1, null).asn1Encode()),
+                            new DerValue(new ETypeInfo(1, "").asn1Encode()),
+                            new DerValue(new ETypeInfo(1, OneKDC.REALM).asn1Encode()),
+                        };
+                        break;
+                    case 2:     // we still reject non-null s2kparams and prefer E2 over E
+                        pas2 = new DerValue[] {
+                            new DerValue(new ETypeInfo2(1, OneKDC.REALM, new byte[]{1}).asn1Encode()),
+                            new DerValue(new ETypeInfo2(1, null, null).asn1Encode()),
+                            new DerValue(new ETypeInfo2(1, "", null).asn1Encode()),
+                        };
+                        pas = new DerValue[] {
+                            new DerValue(new ETypeInfo(1, OneKDC.REALM).asn1Encode()),
+                            new DerValue(new ETypeInfo(1, null).asn1Encode()),
+                            new DerValue(new ETypeInfo(1, "").asn1Encode()),
+                        };
+                        break;
+                    case 3:     // but only E is wrong
+                        pas = new DerValue[] {
+                            new DerValue(new ETypeInfo(1, OneKDC.REALM).asn1Encode()),
+                            new DerValue(new ETypeInfo(1, null).asn1Encode()),
+                            new DerValue(new ETypeInfo(1, "").asn1Encode()),
+                        };
+                        break;
+                    case 4:     // we also ignore rc4-hmac
+                        pas = new DerValue[] {
+                            new DerValue(new ETypeInfo(23, "ANYTHING").asn1Encode()),
+                            new DerValue(new ETypeInfo(1, null).asn1Encode()),
+                            new DerValue(new ETypeInfo(1, "").asn1Encode()),
+                        };
+                        break;
+                    case 5:     // "" should be wrong, but we accept it now
+                                // See s.s.k.internal.PAData$SaltAndParams
+                        pas = new DerValue[] {
+                            new DerValue(new ETypeInfo(1, "").asn1Encode()),
+                            new DerValue(new ETypeInfo(1, null).asn1Encode()),
+                        };
+                        break;
+                }
+            } else {
                 int[] epas = eTypes;
                 if (options.containsKey(KDC.Option.RC4_FIRST_PREAUTH)) {
                     for (int i=1; i<epas.length; i++) {
@@ -893,20 +948,14 @@ public class KDC {
                 } else if (options.containsKey(KDC.Option.ONLY_ONE_PREAUTH)) {
                     epas = new int[] { eTypes[0] };
                 }
-
-            DerValue[] pas = new DerValue[epas.length];
+                pas2 = new DerValue[epas.length];
                 for (int i=0; i<epas.length; i++) {
-                pas[i] = new DerValue(new ETypeInfo2(
+                    pas2[i] = new DerValue(new ETypeInfo2(
                             epas[i],
                             epas[i] == EncryptedData.ETYPE_ARCFOUR_HMAC ?
                                 null : getSalt(body.cname),
                             null).asn1Encode());
                 }
-            DerOutputStream eid = new DerOutputStream();
-            eid.putSequence(pas);
-
-            outPAs.add(new PAData(Krb5.PA_ETYPE_INFO2, eid.toByteArray()));
-
                 boolean allOld = true;
                 for (int i: eTypes) {
                     if (i == EncryptedData.ETYPE_AES128_CTS_HMAC_SHA1_96 ||
@@ -916,6 +965,7 @@ public class KDC {
                     }
                 }
                 if (allOld) {
+                    pas = new DerValue[epas.length];
                     for (int i=0; i<epas.length; i++) {
                         pas[i] = new DerValue(new ETypeInfo(
                                 epas[i],
@@ -923,6 +973,16 @@ public class KDC {
                                     null : getSalt(body.cname)
                                 ).asn1Encode());
                     }
+                }
+            }
+
+            DerOutputStream eid;
+            if (pas2 != null) {
+                eid = new DerOutputStream();
+                eid.putSequence(pas2);
+                outPAs.add(new PAData(Krb5.PA_ETYPE_INFO2, eid.toByteArray()));
+            }
+            if (pas != null) {
                 eid = new DerOutputStream();
                 eid.putSequence(pas);
                 outPAs.add(new PAData(Krb5.PA_ETYPE_INFO, eid.toByteArray()));