From 6476cef307b1d03f3410ba48f7075846c4e1c564 Mon Sep 17 00:00:00 2001 From: andrew Date: Sun, 12 Jan 2020 05:59:10 +0000 Subject: [PATCH] 8201627: Kerberos sequence number issues Reviewed-by: mbalao --- .../security/action/GetPropertyAction.java | 23 ++++++ .../jgss/krb5/InitSecContextToken.java | 46 ++++++++++-- .../security/jgss/krb5/MessageToken_v2.java | 12 +-- test/sun/security/krb5/auto/Basic.java | 7 +- .../sun/security/krb5/auto/BasicKrb5Test.java | 2 +- test/sun/security/krb5/auto/BasicProc.java | 73 ++++++++----------- test/sun/security/krb5/auto/Context.java | 20 +++-- 7 files changed, 120 insertions(+), 63 deletions(-) diff --git a/src/share/classes/sun/security/action/GetPropertyAction.java b/src/share/classes/sun/security/action/GetPropertyAction.java index 24ecc91a6..59bf32486 100644 --- a/src/share/classes/sun/security/action/GetPropertyAction.java +++ b/src/share/classes/sun/security/action/GetPropertyAction.java @@ -108,4 +108,27 @@ public class GetPropertyAction implements PrivilegedAction { new GetPropertyAction(theProp)); } } + + /** + * Convenience method to get a property without going through doPrivileged + * if no security manager is present. This is unsafe for inclusion in a + * public API but allowable here since this class is now encapsulated. + * + * Note that this method performs a privileged action using caller-provided + * inputs. The caller of this method should take care to ensure that the + * inputs are not tainted and the returned property is not made accessible + * to untrusted code if it contains sensitive information. + * + * @param theProp the name of the system property. + * @param defaultVal the default value. + */ + public static String privilegedGetProperty(String theProp, + String defaultVal) { + if (System.getSecurityManager() == null) { + return System.getProperty(theProp, defaultVal); + } else { + return AccessController.doPrivileged( + new GetPropertyAction(theProp, defaultVal)); + } + } } diff --git a/src/share/classes/sun/security/jgss/krb5/InitSecContextToken.java b/src/share/classes/sun/security/jgss/krb5/InitSecContextToken.java index 0077e7edb..1688d5cee 100644 --- a/src/share/classes/sun/security/jgss/krb5/InitSecContextToken.java +++ b/src/share/classes/sun/security/jgss/krb5/InitSecContextToken.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2000, 2018, 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 @@ -29,6 +29,8 @@ import com.sun.security.jgss.AuthorizationDataEntry; import org.ietf.jgss.*; import java.io.InputStream; import java.io.IOException; + +import sun.security.action.GetPropertyAction; import sun.security.krb5.*; import java.net.InetAddress; import sun.security.krb5.internal.AuthorizationData; @@ -36,6 +38,33 @@ import sun.security.krb5.internal.KerberosTime; class InitSecContextToken extends InitialToken { + // If non-mutual authentication is requested, there is no AP-REP message. + // The acceptor thus has no chance to send the seq-number field to the + // initiator. In this case, the initiator and acceptor should has an + // agreement to derive acceptor's initial seq-number if the acceptor wishes + // to send messages to the initiator. + + // If this flag is true, it will the same as the initiator's initial + // seq-number (as MIT krb5 and Windows SSPI do). Otherwise, it will be zero + // (as Heimdal does). The default value is true. + private static final boolean ACCEPTOR_USE_INITIATOR_SEQNUM; + + static { + // The ACCEPTOR_USE_INITIATOR_SEQNUM value is determined by the system + // property "sun.security.krb5.acceptor.sequence.number.nonmutual", + // which can be set to "initiator", "zero" or "0". + String propName = "sun.security.krb5.acceptor.sequence.number.nonmutual"; + String s = GetPropertyAction.privilegedGetProperty(propName, "initiator"); + if (s.equals("initiator")) { + ACCEPTOR_USE_INITIATOR_SEQNUM = true; + } else if (s.equals("zero") || s.equals("0")) { + ACCEPTOR_USE_INITIATOR_SEQNUM = false; + } else { + throw new AssertionError("Unrecognized value for " + propName + + ": " + s); + } + } + private KrbApReq apReq = null; /** @@ -79,7 +108,10 @@ class InitSecContextToken extends InitialToken { context.setKey(Krb5Context.SESSION_KEY, serviceTicket.getSessionKey()); if (!mutualRequired) - context.resetPeerSequenceNumber(0); + context.resetPeerSequenceNumber( + ACCEPTOR_USE_INITIATOR_SEQNUM + ? apReq.getSeqNumber().intValue() + : 0); } /** @@ -144,10 +176,12 @@ class InitSecContextToken extends InitialToken { apReqSeqNumber.intValue() : 0); context.resetPeerSequenceNumber(peerSeqNumber); - if (!context.getMutualAuthState()) - // Use the same sequence number as the peer - // (Behaviour exhibited by the Windows SSPI server) - context.resetMySequenceNumber(peerSeqNumber); + if (!context.getMutualAuthState()) { + context.resetMySequenceNumber( + ACCEPTOR_USE_INITIATOR_SEQNUM + ? peerSeqNumber + : 0); + } context.setAuthTime( new KerberosTime(apReq.getCreds().getAuthTime()).toString()); context.setTktFlags(apReq.getCreds().getFlags()); diff --git a/src/share/classes/sun/security/jgss/krb5/MessageToken_v2.java b/src/share/classes/sun/security/jgss/krb5/MessageToken_v2.java index 7d293ce5c..808c2014a 100644 --- a/src/share/classes/sun/security/jgss/krb5/MessageToken_v2.java +++ b/src/share/classes/sun/security/jgss/krb5/MessageToken_v2.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004, 2011, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2004, 2018, 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 @@ -499,11 +499,11 @@ abstract class MessageToken_v2 extends Krb5Token { */ class MessageTokenHeader { - private int tokenId; - private byte[] bytes = new byte[TOKEN_HEADER_SIZE]; + private int tokenId; + private byte[] bytes = new byte[TOKEN_HEADER_SIZE]; - // Writes a new token header - public MessageTokenHeader(int tokenId, boolean conf) throws GSSException { + // Writes a new token header + public MessageTokenHeader(int tokenId, boolean conf) throws GSSException { this.tokenId = tokenId; @@ -609,7 +609,7 @@ abstract class MessageToken_v2 extends Krb5Token { prop.setQOP(0); // sequence number - seqNumber = readBigEndian(bytes, 0, 8); + seqNumber = readBigEndian(bytes, 12, 4); } /** diff --git a/test/sun/security/krb5/auto/Basic.java b/test/sun/security/krb5/auto/Basic.java index 7ec3873fb..cc934ff5b 100644 --- a/test/sun/security/krb5/auto/Basic.java +++ b/test/sun/security/krb5/auto/Basic.java @@ -23,10 +23,12 @@ /* * @test - * @bug 7152176 + * @bug 7152176 8201627 * @summary More krb5 tests * @compile -XDignore.symbol.file Basic.java - * @run main/othervm -Dsun.net.spi.nameservice.provider.1=ns,mock Basic + * @run main/othervm -Dsun.net.spi.nameservice.provider.1=ns,mock + * -Dsun.security.krb5.acceptor.sequence.number.nonmutual=zero + * Basic */ import sun.security.jgss.GSSUtil; @@ -45,6 +47,7 @@ public class Basic { c.startAsClient(OneKDC.SERVER, GSSUtil.GSS_KRB5_MECH_OID); c.x().requestCredDeleg(true); + c.x().requestMutualAuth(false); s.startAsServer(GSSUtil.GSS_KRB5_MECH_OID); Context.handshake(c, s); diff --git a/test/sun/security/krb5/auto/BasicKrb5Test.java b/test/sun/security/krb5/auto/BasicKrb5Test.java index ade0b54fe..f18ae1642 100644 --- a/test/sun/security/krb5/auto/BasicKrb5Test.java +++ b/test/sun/security/krb5/auto/BasicKrb5Test.java @@ -79,7 +79,7 @@ public class BasicKrb5Test { String etype = null; for (String arg: args) { if (arg.equals("-s")) Context.usingStream = true; - else if(arg.equals("-C")) conf = false; + else if (arg.equals("-C")) conf = false; else etype = arg; } diff --git a/test/sun/security/krb5/auto/BasicProc.java b/test/sun/security/krb5/auto/BasicProc.java index d9d7ef97b..50f65eab5 100644 --- a/test/sun/security/krb5/auto/BasicProc.java +++ b/test/sun/security/krb5/auto/BasicProc.java @@ -23,10 +23,10 @@ /* * @test - * @bug 8009977 8186884 + * @bug 8009977 8186884 8201627 * @summary A test to launch multiple Java processes using either Java GSS * or native GSS - * @library ../../../../java/security/testlibrary/ + * @library ../../../../java/security/testlibrary /lib/testlibrary * @compile -XDignore.symbol.file BasicProc.java * @run main/othervm -Dsun.net.spi.nameservice.provider.1=ns,mock BasicProc launcher */ @@ -38,9 +38,9 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.PropertyPermission; -import java.util.Random; import java.util.Set; +import jdk.testlibrary.Asserts; import org.ietf.jgss.Oid; import sun.security.krb5.Config; @@ -84,6 +84,7 @@ public class BasicProc { private static final String REALM = "REALM"; private static final int MSGSIZE = 1024; + private static final byte[] MSG = new byte[MSGSIZE]; public static void main(String[] args) throws Exception { @@ -165,9 +166,11 @@ public class BasicProc { Context.fromUserPass(USER, PASS, false); c.startAsClient(SERVER, oid); c.x().requestCredDeleg(true); + c.x().requestMutualAuth(true); Proc.binOut(c.take(new byte[0])); // AP-REQ - token = Proc.binIn(); // AP-REP - c.take(token); + c.take(Proc.binIn()); // AP-REP + Proc.binOut(c.wrap(MSG, true)); + Proc.binOut(c.getMic(MSG)); break; case "server": Context s = args[1].equals("n") ? @@ -175,41 +178,27 @@ public class BasicProc { Context.fromUserKtab(SERVER, KTAB_S, true); s.startAsServer(oid); token = Proc.binIn(); // AP-REQ - token = s.take(token); - Proc.binOut(token); // AP-REP + Proc.binOut(s.take(token)); // AP-REP + msg = s.unwrap(Proc.binIn(), true); + Asserts.assertTrue(Arrays.equals(msg, MSG)); + s.verifyMic(Proc.binIn(), msg); Context s2 = s.delegated(); s2.startAsClient(BACKEND, oid); + s2.x().requestMutualAuth(false); Proc.binOut(s2.take(new byte[0])); // AP-REQ - token = Proc.binIn(); - s2.take(token); // AP-REP - Random r = new Random(); - msg = new byte[MSGSIZE]; - r.nextBytes(msg); - Proc.binOut(s2.wrap(msg, true)); // enc1 - Proc.binOut(s2.wrap(msg, true)); // enc2 - Proc.binOut(s2.wrap(msg, true)); // enc3 - s2.verifyMic(Proc.binIn(), msg); // mic - byte[] msg2 = Proc.binIn(); // msg - if (!Arrays.equals(msg, msg2)) { - throw new Exception("diff msg"); - } + msg = s2.unwrap(Proc.binIn(), true); + Asserts.assertTrue(Arrays.equals(msg, MSG)); + s2.verifyMic(Proc.binIn(), msg); break; case "backend": Context b = args[1].equals("n") ? Context.fromThinAir() : Context.fromUserKtab(BACKEND, KTAB_B, true); b.startAsServer(oid); - token = Proc.binIn(); // AP-REQ - Proc.binOut(b.take(token)); // AP-REP - msg = b.unwrap(Proc.binIn(), true); // enc1 - if (!Arrays.equals(msg, b.unwrap(Proc.binIn(), true))) { // enc2 - throw new Exception("diff msg"); - } - if (!Arrays.equals(msg, b.unwrap(Proc.binIn(), true))) { // enc3 - throw new Exception("diff msg"); - } - Proc.binOut(b.getMic(msg)); // mic - Proc.binOut(msg); // msg + token = b.take(Proc.binIn()); // AP-REQ + Asserts.assertTrue(token == null); + Proc.binOut(b.wrap(MSG, true)); + Proc.binOut(b.getMic(MSG)); break; } } @@ -281,20 +270,18 @@ public class BasicProc { } pb.start(); - // Client and server handshake - ps.println(pc.readData()); - pc.println(ps.readData()); + // Client and server + ps.println(pc.readData()); // AP-REQ + pc.println(ps.readData()); // AP-REP + + ps.println(pc.readData()); // KRB-PRIV + ps.println(pc.readData()); // KRB-SAFE - // Server and backend handshake - pb.println(ps.readData()); - ps.println(pb.readData()); + // Server and backend + pb.println(ps.readData()); // AP-REQ - // wrap/unwrap/getMic/verifyMic and plain text - pb.println(ps.readData()); - pb.println(ps.readData()); - pb.println(ps.readData()); - ps.println(pb.readData()); - ps.println(pb.readData()); + ps.println(pb.readData()); // KRB-PRIV + ps.println(pb.readData()); // KRB-SAFE if ((pc.waitFor() | ps.waitFor() | pb.waitFor()) != 0) { throw new Exception("Process failed"); diff --git a/test/sun/security/krb5/auto/Context.java b/test/sun/security/krb5/auto/Context.java index cefbaccbf..2c4915393 100644 --- a/test/sun/security/krb5/auto/Context.java +++ b/test/sun/security/krb5/auto/Context.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2008, 2018, 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 @@ -475,18 +475,21 @@ public class Context { out = me.x.wrap(input, 0, input.length, p1); } System.out.println(printProp(p1)); + if ((x.getConfState() && privacy) != p1.getPrivacy()) { + throw new Exception("unexpected privacy status"); + } return out; } }, t); } - public byte[] unwrap(byte[] t, final boolean privacy) + public byte[] unwrap(byte[] t, final boolean privacyExpected) throws Exception { return doAs(new Action() { @Override public byte[] run(Context me, byte[] input) throws Exception { - System.out.printf("unwrap %s privacy from %s: ", privacy?"with":"without", me.name); - MessageProp p1 = new MessageProp(0, privacy); + System.out.printf("unwrap from %s", me.name); + MessageProp p1 = new MessageProp(0, true); byte[] bytes; if (usingStream) { ByteArrayOutputStream os = new ByteArrayOutputStream(); @@ -496,6 +499,9 @@ public class Context { bytes = me.x.unwrap(input, 0, input.length, p1); } System.out.println(printProp(p1)); + if (p1.getPrivacy() != privacyExpected) { + throw new Exception("Unexpected privacy: " + p1.getPrivacy()); + } return bytes; } }, t); @@ -537,6 +543,10 @@ public class Context { p1); } System.out.println(printProp(p1)); + if (p1.isUnseqToken() || p1.isOldToken() + || p1.isDuplicateToken() || p1.isGapToken()) { + throw new Exception("Wrong sequence number detected"); + } return null; } }, t); @@ -572,7 +582,7 @@ public class Context { System.out.printf("-------------------- TRANSMIT from %s to %s------------------------\n", s1.name, s2.name); byte[] wrapped = s1.wrap(messageBytes, true); - byte[] unwrapped = s2.unwrap(wrapped, true); + byte[] unwrapped = s2.unwrap(wrapped, s2.x.getConfState()); if (!Arrays.equals(messageBytes, unwrapped)) { throw new Exception("wrap/unwrap mismatch"); } -- GitLab