Input Validation for IpSecService

All of the input to IpSecService over the Binder
interface needs to be validated both for sanity
and for safety.

-Sanity check all the parameters coming from binder.
-Added setters for IpSecConfig to decouple the test
 from the IpSecManager. This was needed because the
 input validation caused the tests to fail due to a
 null parameter that was previously un-tested.
-Added the mode flag to the IpSecConfig bundle this
 oversight was found during testing.
-Expose the getResourceId() methods for testing in
 UdpEncapsulationSocket, SecurityParameterIndex, and
 IpSecTransform classes.
-Remove the unneeded getIpSecConfig() from
 IpSecTransform: unneeded now that we can synthesize
 configs.

Bug: 38397094
Test: runtest frameworks-net
Change-Id: I5241fc7fbfa9816d54219acd8d81a9f7eef10dd4

core/java/android/net/IpSecConfig.java

@@ -17,105 +17,163 @@ package android.net;
 
 import android.os.Parcel;
 import android.os.Parcelable;
-import android.util.Log;
-import java.net.InetAddress;
-import java.net.UnknownHostException;
+
+import com.android.internal.annotations.VisibleForTesting;
 
 /** @hide */
 public final class IpSecConfig implements Parcelable {
     private static final String TAG = "IpSecConfig";
 
-    //MODE_TRANSPORT or MODE_TUNNEL
-    int mode;
+    // MODE_TRANSPORT or MODE_TUNNEL
+    private int mMode = IpSecTransform.MODE_TRANSPORT;
 
-    // For tunnel mode
-    InetAddress localAddress;
+    // Needs to be valid only for tunnel mode
+    // Preventing this from being null simplifies Java->Native binder
+    private String mLocalAddress = "";
 
-    InetAddress remoteAddress;
+    // Preventing this from being null simplifies Java->Native binder
+    private String mRemoteAddress = "";
 
-    // Limit selection by network interface
-    Network network;
+    // The underlying network interface that represents the "gateway" Network
+    // for outbound packets. It may also be used to select packets.
+    private Network mNetwork;
 
     public static class Flow {
         // Minimum requirements for identifying a transform
         // SPI identifying the IPsec flow in packet processing
         // and a remote IP address
-        int spiResourceId;
+        private int mSpiResourceId = IpSecManager.INVALID_RESOURCE_ID;
 
         // Encryption Algorithm
-        IpSecAlgorithm encryption;
+        private IpSecAlgorithm mEncryption;
 
         // Authentication Algorithm
-        IpSecAlgorithm authentication;
+        private IpSecAlgorithm mAuthentication;
 
         @Override
         public String toString() {
             return new StringBuilder()
-                    .append("{spiResourceId=")
-                    .append(spiResourceId)
-                    .append(", encryption=")
-                    .append(encryption)
-                    .append(", authentication=")
-                    .append(authentication)
+                    .append("{mSpiResourceId=")
+                    .append(mSpiResourceId)
+                    .append(", mEncryption=")
+                    .append(mEncryption)
+                    .append(", mAuthentication=")
+                    .append(mAuthentication)
                     .append("}")
                     .toString();
         }
     }
 
-    final Flow[] flow = new Flow[] {new Flow(), new Flow()};
+    private final Flow[] mFlow = new Flow[] {new Flow(), new Flow()};
 
     // For tunnel mode IPv4 UDP Encapsulation
     // IpSecTransform#ENCAP_ESP_*, such as ENCAP_ESP_OVER_UDP_IKE
-    int encapType;
-    int encapLocalPortResourceId;
-    int encapRemotePort;
+    private int mEncapType = IpSecTransform.ENCAP_NONE;
+    private int mEncapSocketResourceId = IpSecManager.INVALID_RESOURCE_ID;
+    private int mEncapRemotePort;
 
     // An interval, in seconds between the NattKeepalive packets
-    int nattKeepaliveInterval;
+    private int mNattKeepaliveInterval;
+
+    /** Set the mode for this IPsec transform */
+    public void setMode(int mode) {
+        mMode = mode;
+    }
+
+    /** Set the local IP address for Tunnel mode */
+    public void setLocalAddress(String localAddress) {
+        if (localAddress == null) {
+            throw new IllegalArgumentException("localAddress may not be null!");
+        }
+        mLocalAddress = localAddress;
+    }
+
+    /** Set the remote IP address for this IPsec transform */
+    public void setRemoteAddress(String remoteAddress) {
+        if (remoteAddress == null) {
+            throw new IllegalArgumentException("remoteAddress may not be null!");
+        }
+        mRemoteAddress = remoteAddress;
+    }
+
+    /** Set the SPI for a given direction by resource ID */
+    public void setSpiResourceId(int direction, int resourceId) {
+        mFlow[direction].mSpiResourceId = resourceId;
+    }
+
+    /** Set the encryption algorithm for a given direction */
+    public void setEncryption(int direction, IpSecAlgorithm encryption) {
+        mFlow[direction].mEncryption = encryption;
+    }
+
+    /** Set the authentication algorithm for a given direction */
+    public void setAuthentication(int direction, IpSecAlgorithm authentication) {
+        mFlow[direction].mAuthentication = authentication;
+    }
+
+    public void setNetwork(Network network) {
+        mNetwork = network;
+    }
+
+    public void setEncapType(int encapType) {
+        mEncapType = encapType;
+    }
+
+    public void setEncapSocketResourceId(int resourceId) {
+        mEncapSocketResourceId = resourceId;
+    }
+
+    public void setEncapRemotePort(int port) {
+        mEncapRemotePort = port;
+    }
+
+    public void setNattKeepaliveInterval(int interval) {
+        mNattKeepaliveInterval = interval;
+    }
 
     // Transport or Tunnel
     public int getMode() {
-        return mode;
+        return mMode;
     }
 
-    public InetAddress getLocalAddress() {
-        return localAddress;
+    public String getLocalAddress() {
+        return mLocalAddress;
     }
 
     public int getSpiResourceId(int direction) {
-        return flow[direction].spiResourceId;
+        return mFlow[direction].mSpiResourceId;
     }
 
-    public InetAddress getRemoteAddress() {
-        return remoteAddress;
+    public String getRemoteAddress() {
+        return mRemoteAddress;
     }
 
     public IpSecAlgorithm getEncryption(int direction) {
-        return flow[direction].encryption;
+        return mFlow[direction].mEncryption;
     }
 
     public IpSecAlgorithm getAuthentication(int direction) {
-        return flow[direction].authentication;
+        return mFlow[direction].mAuthentication;
     }
 
     public Network getNetwork() {
-        return network;
+        return mNetwork;
     }
 
     public int getEncapType() {
-        return encapType;
+        return mEncapType;
     }
 
-    public int getEncapLocalResourceId() {
-        return encapLocalPortResourceId;
+    public int getEncapSocketResourceId() {
+        return mEncapSocketResourceId;
     }
 
     public int getEncapRemotePort() {
-        return encapRemotePort;
+        return mEncapRemotePort;
     }
 
     public int getNattKeepaliveInterval() {
-        return nattKeepaliveInterval;
+        return mNattKeepaliveInterval;
     }
 
     // Parcelable Methods
@@ -127,82 +185,68 @@ public final class IpSecConfig implements Parcelable {
 
     @Override
     public void writeToParcel(Parcel out, int flags) {
-        // TODO: Use a byte array or other better method for storing IPs that can also include scope
-        out.writeString((localAddress != null) ? localAddress.getHostAddress() : null);
-        // TODO: Use a byte array or other better method for storing IPs that can also include scope
-        out.writeString((remoteAddress != null) ? remoteAddress.getHostAddress() : null);
-        out.writeParcelable(network, flags);
-        out.writeInt(flow[IpSecTransform.DIRECTION_IN].spiResourceId);
-        out.writeParcelable(flow[IpSecTransform.DIRECTION_IN].encryption, flags);
-        out.writeParcelable(flow[IpSecTransform.DIRECTION_IN].authentication, flags);
-        out.writeInt(flow[IpSecTransform.DIRECTION_OUT].spiResourceId);
-        out.writeParcelable(flow[IpSecTransform.DIRECTION_OUT].encryption, flags);
-        out.writeParcelable(flow[IpSecTransform.DIRECTION_OUT].authentication, flags);
-        out.writeInt(encapType);
-        out.writeInt(encapLocalPortResourceId);
-        out.writeInt(encapRemotePort);
+        out.writeInt(mMode);
+        out.writeString(mLocalAddress);
+        out.writeString(mRemoteAddress);
+        out.writeParcelable(mNetwork, flags);
+        out.writeInt(mFlow[IpSecTransform.DIRECTION_IN].mSpiResourceId);
+        out.writeParcelable(mFlow[IpSecTransform.DIRECTION_IN].mEncryption, flags);
+        out.writeParcelable(mFlow[IpSecTransform.DIRECTION_IN].mAuthentication, flags);
+        out.writeInt(mFlow[IpSecTransform.DIRECTION_OUT].mSpiResourceId);
+        out.writeParcelable(mFlow[IpSecTransform.DIRECTION_OUT].mEncryption, flags);
+        out.writeParcelable(mFlow[IpSecTransform.DIRECTION_OUT].mAuthentication, flags);
+        out.writeInt(mEncapType);
+        out.writeInt(mEncapSocketResourceId);
+        out.writeInt(mEncapRemotePort);
     }
 
-    // Package Private: Used by the IpSecTransform.Builder;
-    // there should be no public constructor for this object
-    IpSecConfig() {}
-
-    private static InetAddress readInetAddressFromParcel(Parcel in) {
-        String addrString = in.readString();
-        if (addrString == null) {
-            return null;
-        }
-        try {
-            return InetAddress.getByName(addrString);
-        } catch (UnknownHostException e) {
-            Log.wtf(TAG, "Invalid IpAddress " + addrString);
-            return null;
-        }
-    }
+    @VisibleForTesting
+    public IpSecConfig() {}
 
     private IpSecConfig(Parcel in) {
-        localAddress = readInetAddressFromParcel(in);
-        remoteAddress = readInetAddressFromParcel(in);
-        network = (Network) in.readParcelable(Network.class.getClassLoader());
-        flow[IpSecTransform.DIRECTION_IN].spiResourceId = in.readInt();
-        flow[IpSecTransform.DIRECTION_IN].encryption =
+        mMode = in.readInt();
+        mLocalAddress = in.readString();
+        mRemoteAddress = in.readString();
+        mNetwork = (Network) in.readParcelable(Network.class.getClassLoader());
+        mFlow[IpSecTransform.DIRECTION_IN].mSpiResourceId = in.readInt();
+        mFlow[IpSecTransform.DIRECTION_IN].mEncryption =
                 (IpSecAlgorithm) in.readParcelable(IpSecAlgorithm.class.getClassLoader());
-        flow[IpSecTransform.DIRECTION_IN].authentication =
+        mFlow[IpSecTransform.DIRECTION_IN].mAuthentication =
                 (IpSecAlgorithm) in.readParcelable(IpSecAlgorithm.class.getClassLoader());
-        flow[IpSecTransform.DIRECTION_OUT].spiResourceId = in.readInt();
-        flow[IpSecTransform.DIRECTION_OUT].encryption =
+        mFlow[IpSecTransform.DIRECTION_OUT].mSpiResourceId = in.readInt();
+        mFlow[IpSecTransform.DIRECTION_OUT].mEncryption =
                 (IpSecAlgorithm) in.readParcelable(IpSecAlgorithm.class.getClassLoader());
-        flow[IpSecTransform.DIRECTION_OUT].authentication =
+        mFlow[IpSecTransform.DIRECTION_OUT].mAuthentication =
                 (IpSecAlgorithm) in.readParcelable(IpSecAlgorithm.class.getClassLoader());
-        encapType = in.readInt();
-        encapLocalPortResourceId = in.readInt();
-        encapRemotePort = in.readInt();
+        mEncapType = in.readInt();
+        mEncapSocketResourceId = in.readInt();
+        mEncapRemotePort = in.readInt();
     }
 
     @Override
     public String toString() {
         StringBuilder strBuilder = new StringBuilder();
         strBuilder
-                .append("{mode=")
-                .append(mode == IpSecTransform.MODE_TUNNEL ? "TUNNEL" : "TRANSPORT")
-                .append(", localAddress=")
-                .append(localAddress)
-                .append(", remoteAddress=")
-                .append(remoteAddress)
-                .append(", network=")
-                .append(network)
-                .append(", encapType=")
-                .append(encapType)
-                .append(", encapLocalPortResourceId=")
-                .append(encapLocalPortResourceId)
-                .append(", encapRemotePort=")
-                .append(encapRemotePort)
-                .append(", nattKeepaliveInterval=")
-                .append(nattKeepaliveInterval)
-                .append(", flow[OUT]=")
-                .append(flow[IpSecTransform.DIRECTION_OUT])
-                .append(", flow[IN]=")
-                .append(flow[IpSecTransform.DIRECTION_IN])
+                .append("{mMode=")
+                .append(mMode == IpSecTransform.MODE_TUNNEL ? "TUNNEL" : "TRANSPORT")
+                .append(", mLocalAddress=")
+                .append(mLocalAddress)
+                .append(", mRemoteAddress=")
+                .append(mRemoteAddress)
+                .append(", mNetwork=")
+                .append(mNetwork)
+                .append(", mEncapType=")
+                .append(mEncapType)
+                .append(", mEncapSocketResourceId=")
+                .append(mEncapSocketResourceId)
+                .append(", mEncapRemotePort=")
+                .append(mEncapRemotePort)
+                .append(", mNattKeepaliveInterval=")
+                .append(mNattKeepaliveInterval)
+                .append(", mFlow[OUT]=")
+                .append(mFlow[IpSecTransform.DIRECTION_OUT])
+                .append(", mFlow[IN]=")
+                .append(mFlow[IpSecTransform.DIRECTION_IN])
                 .append("}");
 
         return strBuilder.toString();

core/java/android/net/IpSecManager.java

@@ -26,6 +26,8 @@ import android.os.RemoteException;
 import android.util.AndroidException;
 import android.util.Log;
 
+import com.android.internal.annotations.VisibleForTesting;
+
 import dalvik.system.CloseGuard;
 
 import java.io.FileDescriptor;
@@ -188,7 +190,8 @@ public final class IpSecManager {
         }
 
         /** @hide */
-        int getResourceId() {
+        @VisibleForTesting
+        public int getResourceId() {
             return mResourceId;
         }
     }
@@ -489,7 +492,8 @@ public final class IpSecManager {
         }
 
         /** @hide */
-        int getResourceId() {
+        @VisibleForTesting
+        public int getResourceId() {
             return mResourceId;
         }
     };

core/java/android/net/IpSecTransform.java

@@ -68,10 +68,10 @@ public final class IpSecTransform implements AutoCloseable {
     public @interface TransformDirection {}
 
     /** @hide */
-    public static final int MODE_TUNNEL = 0;
+    public static final int MODE_TRANSPORT = 0;
 
     /** @hide */
-    public static final int MODE_TRANSPORT = 1;
+    public static final int MODE_TUNNEL = 1;
 
     /** @hide */
     public static final int ENCAP_NONE = 0;
@@ -113,7 +113,11 @@ public final class IpSecTransform implements AutoCloseable {
         return IIpSecService.Stub.asInterface(b);
     }
 
-    private void checkResultStatusAndThrow(int status)
+    /**
+     * Checks the result status and throws an appropriate exception if
+     * the status is not Status.OK.
+     */
+    private void checkResultStatus(int status)
             throws IOException, IpSecManager.ResourceUnavailableException,
                     IpSecManager.SpiUnavailableException {
         switch (status) {
@@ -141,7 +145,7 @@ public final class IpSecTransform implements AutoCloseable {
                 IpSecTransformResponse result =
                         svc.createTransportModeTransform(mConfig, new Binder());
                 int status = result.status;
-                checkResultStatusAndThrow(status);
+                checkResultStatus(status);
                 mResourceId = result.resourceId;
 
                 /* Keepalive will silently fail if not needed by the config; but, if needed and
@@ -243,61 +247,20 @@ public final class IpSecTransform implements AutoCloseable {
 
     /* Package */
     void startKeepalive(Context c) {
-        // FIXME: NO_KEEPALIVE needs to be a constant
-        if (mConfig.getNattKeepaliveInterval() == 0) {
-            return;
-        }
-
-        ConnectivityManager cm =
-                (ConnectivityManager) c.getSystemService(Context.CONNECTIVITY_SERVICE);
-
-        if (mKeepalive != null) {
-            Log.wtf(TAG, "Keepalive already started for this IpSecTransform.");
-            return;
-        }
-
-        synchronized (mKeepaliveSyncLock) {
-            mKeepalive =
-                    cm.startNattKeepalive(
-                            mConfig.getNetwork(),
-                            mConfig.getNattKeepaliveInterval(),
-                            mKeepaliveCallback,
-                            mConfig.getLocalAddress(),
-                            0x1234, /* FIXME: get the real port number again,
-                                    which we need to retrieve from the provided
-                                    EncapsulationSocket, and which isn't currently
-                                    stashed in IpSecConfig */
-                            mConfig.getRemoteAddress());
-            try {
-                // FIXME: this is still a horrible way to fudge the synchronous callback
-                mKeepaliveSyncLock.wait(2000);
-            } catch (InterruptedException e) {
-            }
-        }
-        if (mKeepaliveStatus != ConnectivityManager.PacketKeepalive.SUCCESS) {
-            throw new UnsupportedOperationException("Packet Keepalive cannot be started");
+        if (mConfig.getNattKeepaliveInterval() != 0) {
+            Log.wtf(TAG, "Keepalive not yet supported.");
         }
     }
 
-    /* Package */
-    int getResourceId() {
+    /** @hide */
+    @VisibleForTesting
+    public int getResourceId() {
         return mResourceId;
     }
 
     /* Package */
     void stopKeepalive() {
-        if (mKeepalive == null) {
-            return;
-        }
-        mKeepalive.stop();
-        synchronized (mKeepaliveSyncLock) {
-            if (mKeepaliveStatus == ConnectivityManager.PacketKeepalive.SUCCESS) {
-                try {
-                    mKeepaliveSyncLock.wait(2000);
-                } catch (InterruptedException e) {
-                }
-            }
-        }
+        return;
     }
 
     /**
@@ -323,7 +286,7 @@ public final class IpSecTransform implements AutoCloseable {
          */
         public IpSecTransform.Builder setEncryption(
                 @TransformDirection int direction, IpSecAlgorithm algo) {
-            mConfig.flow[direction].encryption = algo;
+            mConfig.setEncryption(direction, algo);
             return this;
         }
 
@@ -338,7 +301,7 @@ public final class IpSecTransform implements AutoCloseable {
          */
         public IpSecTransform.Builder setAuthentication(
                 @TransformDirection int direction, IpSecAlgorithm algo) {
-            mConfig.flow[direction].authentication = algo;
+            mConfig.setAuthentication(direction, algo);
             return this;
         }
 
@@ -361,9 +324,7 @@ public final class IpSecTransform implements AutoCloseable {
          */
         public IpSecTransform.Builder setSpi(
                 @TransformDirection int direction, IpSecManager.SecurityParameterIndex spi) {
-            // TODO: convert to using the resource Id of the SPI. Then build() can validate
-            // the owner in the IpSecService
-            mConfig.flow[direction].spiResourceId = spi.getResourceId();
+            mConfig.setSpiResourceId(direction, spi.getResourceId());
             return this;
         }
 
@@ -378,7 +339,7 @@ public final class IpSecTransform implements AutoCloseable {
          */
         @SystemApi
         public IpSecTransform.Builder setUnderlyingNetwork(Network net) {
-            mConfig.network = net;
+            mConfig.setNetwork(net);
             return this;
         }
 
@@ -395,10 +356,9 @@ public final class IpSecTransform implements AutoCloseable {
          */
         public IpSecTransform.Builder setIpv4Encapsulation(
                 IpSecManager.UdpEncapsulationSocket localSocket, int remotePort) {
-            // TODO: check encap type is valid.
-            mConfig.encapType = ENCAP_ESPINUDP;
-            mConfig.encapLocalPortResourceId = localSocket.getResourceId();
-            mConfig.encapRemotePort = remotePort;
+            mConfig.setEncapType(ENCAP_ESPINUDP);
+            mConfig.setEncapSocketResourceId(localSocket.getResourceId());
+            mConfig.setEncapRemotePort(remotePort);
             return this;
         }
 
@@ -416,7 +376,7 @@ public final class IpSecTransform implements AutoCloseable {
          */
         @SystemApi
         public IpSecTransform.Builder setNattKeepalive(int intervalSeconds) {
-            mConfig.nattKeepaliveInterval = intervalSeconds;
+            mConfig.setNattKeepaliveInterval(intervalSeconds);
             return this;
         }
 
@@ -451,8 +411,8 @@ public final class IpSecTransform implements AutoCloseable {
                         IpSecManager.SpiUnavailableException, IOException {
             //FIXME: argument validation here
             //throw new IllegalArgumentException("Natt Keepalive requires UDP Encapsulation");
-            mConfig.mode = MODE_TRANSPORT;
-            mConfig.remoteAddress = remoteAddress;
+            mConfig.setMode(MODE_TRANSPORT);
+            mConfig.setRemoteAddress(remoteAddress.getHostAddress());
             return new IpSecTransform(mContext, mConfig).activate();
         }
 
@@ -473,9 +433,9 @@ public final class IpSecTransform implements AutoCloseable {
                 InetAddress localAddress, InetAddress remoteAddress) {
             //FIXME: argument validation here
             //throw new IllegalArgumentException("Natt Keepalive requires UDP Encapsulation");
-            mConfig.localAddress = localAddress;
-            mConfig.remoteAddress = remoteAddress;
-            mConfig.mode = MODE_TUNNEL;
+            mConfig.setLocalAddress(localAddress.getHostAddress());
+            mConfig.setRemoteAddress(remoteAddress.getHostAddress());
+            mConfig.setMode(MODE_TUNNEL);
             return new IpSecTransform(mContext, mConfig);
         }
 
@@ -489,14 +449,5 @@ public final class IpSecTransform implements AutoCloseable {
             mContext = context;
             mConfig = new IpSecConfig();
         }
-
-        /**
-         * Return an {@link IpSecConfig} object for testing purposes.
-         * @hide
-         */
-        @VisibleForTesting
-        public IpSecConfig getIpSecConfig() {
-            return mConfig;
-        }
     }
 }

services/core/java/com/android/server/IpSecService.java

@@ -33,6 +33,7 @@ import android.net.IpSecSpiResponse;
 import android.net.IpSecTransform;
 import android.net.IpSecTransformResponse;
 import android.net.IpSecUdpEncapResponse;
+import android.net.NetworkUtils;
 import android.net.util.NetdService;
 import android.os.Binder;
 import android.os.IBinder;
@@ -42,11 +43,14 @@ import android.os.ServiceSpecificException;
 import android.system.ErrnoException;
 import android.system.Os;
 import android.system.OsConstants;
+import android.text.TextUtils;
 import android.util.Log;
 import android.util.Slog;
 import android.util.SparseArray;
+
 import com.android.internal.annotations.GuardedBy;
 import com.android.internal.annotations.VisibleForTesting;
+
 import java.io.FileDescriptor;
 import java.io.IOException;
 import java.io.PrintWriter;
@@ -54,6 +58,7 @@ import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.UnknownHostException;
 import java.util.concurrent.atomic.AtomicInteger;
+
 import libcore.io.IoUtils;
 
 /** @hide */
@@ -252,7 +257,11 @@ public class IpSecService extends IIpSecService.Stub {
             return (mReferenceCount.get() > 0);
         }
 
-        public void checkOwnerOrSystemAndThrow() {
+        /**
+         * Ensures that the caller is either the owner of this resource or has the system UID and
+         * throws a SecurityException otherwise.
+         */
+        public void checkOwnerOrSystem() {
             if (uid != Binder.getCallingUid()
                     && android.os.Process.SYSTEM_UID != Binder.getCallingUid()) {
                 throw new SecurityException("Only the owner may access managed resources!");
@@ -340,7 +349,7 @@ public class IpSecService extends IIpSecService.Stub {
             // The value should never be null unless the resource doesn't exist
             // (since we do not allow null resources to be added).
             if (val != null) {
-                val.checkOwnerOrSystemAndThrow();
+                val.checkOwnerOrSystem();
             }
             return val;
         }
@@ -405,12 +414,8 @@ public class IpSecService extends IIpSecService.Stub {
                             .ipSecDeleteSecurityAssociation(
                                     mResourceId,
                                     direction,
-                                    (mConfig.getLocalAddress() != null)
-                                            ? mConfig.getLocalAddress().getHostAddress()
-                                            : "",
-                                    (mConfig.getRemoteAddress() != null)
-                                            ? mConfig.getRemoteAddress().getHostAddress()
-                                            : "",
+                                    mConfig.getLocalAddress(),
+                                    mConfig.getRemoteAddress(),
                                     spi);
                 } catch (ServiceSpecificException e) {
                     // FIXME: get the error code and throw is at an IOException from Errno Exception
@@ -638,11 +643,45 @@ public class IpSecService extends IIpSecService.Stub {
         }
     }
 
+    /**
+     * Checks that the provided InetAddress is valid for use in an IPsec SA. The address must not be
+     * a wildcard address and must be in a numeric form such as 1.2.3.4 or 2001::1.
+     */
+    private static void checkInetAddress(String inetAddress) {
+        if (TextUtils.isEmpty(inetAddress)) {
+            throw new IllegalArgumentException("Unspecified address");
+        }
+
+        InetAddress checkAddr = NetworkUtils.numericToInetAddress(inetAddress);
+
+        if (checkAddr.isAnyLocalAddress()) {
+            throw new IllegalArgumentException("Inappropriate wildcard address: " + inetAddress);
+        }
+    }
+
+    /**
+     * Checks the user-provided direction field and throws an IllegalArgumentException if it is not
+     * DIRECTION_IN or DIRECTION_OUT
+     */
+    private static void checkDirection(int direction) {
+        switch (direction) {
+            case IpSecTransform.DIRECTION_OUT:
+            case IpSecTransform.DIRECTION_IN:
+                return;
+        }
+        throw new IllegalArgumentException("Invalid Direction: " + direction);
+    }
+
     @Override
     /** Get a new SPI and maintain the reservation in the system server */
     public synchronized IpSecSpiResponse reserveSecurityParameterIndex(
             int direction, String remoteAddress, int requestedSpi, IBinder binder)
             throws RemoteException {
+        checkDirection(direction);
+        checkInetAddress(remoteAddress);
+        /* requestedSpi can be anything in the int range, so no check is needed. */
+        checkNotNull(binder, "Null Binder passed to reserveSecurityParameterIndex");
+
         int resourceId = mNextResourceId.getAndIncrement();
 
         int spi = IpSecManager.INVALID_SECURITY_PARAMETER_INDEX;
@@ -651,9 +690,7 @@ public class IpSecService extends IIpSecService.Stub {
         try {
             if (!mUserQuotaTracker.getUserRecord(Binder.getCallingUid()).spi.isAvailable()) {
                 return new IpSecSpiResponse(
-                        IpSecManager.Status.RESOURCE_UNAVAILABLE,
-                        INVALID_RESOURCE_ID,
-                        spi);
+                        IpSecManager.Status.RESOURCE_UNAVAILABLE, INVALID_RESOURCE_ID, spi);
             }
             spi =
                     mSrvConfig
@@ -751,6 +788,8 @@ public class IpSecService extends IIpSecService.Stub {
             throw new IllegalArgumentException(
                     "Specified port number must be a valid non-reserved UDP port");
         }
+        checkNotNull(binder, "Null Binder passed to openUdpEncapsulationSocket");
+
         int resourceId = mNextResourceId.getAndIncrement();
         FileDescriptor sockFd = null;
         try {
@@ -791,6 +830,67 @@ public class IpSecService extends IIpSecService.Stub {
         releaseManagedResource(mUdpSocketRecords, resourceId, "UdpEncapsulationSocket");
     }
 
+    /**
+     * Checks an IpSecConfig parcel to ensure that the contents are sane and throws an
+     * IllegalArgumentException if they are not.
+     */
+    private void checkIpSecConfig(IpSecConfig config) {
+        if (config.getLocalAddress() == null) {
+            throw new IllegalArgumentException("Invalid null Local InetAddress");
+        }
+
+        if (config.getRemoteAddress() == null) {
+            throw new IllegalArgumentException("Invalid null Remote InetAddress");
+        }
+
+        switch (config.getMode()) {
+            case IpSecTransform.MODE_TRANSPORT:
+                if (!config.getLocalAddress().isEmpty()) {
+                    throw new IllegalArgumentException("Non-empty Local Address");
+                }
+                // Must be valid, and not a wildcard
+                checkInetAddress(config.getRemoteAddress());
+                break;
+            case IpSecTransform.MODE_TUNNEL:
+                break;
+            default:
+                throw new IllegalArgumentException(
+                        "Invalid IpSecTransform.mode: " + config.getMode());
+        }
+
+        switch (config.getEncapType()) {
+            case IpSecTransform.ENCAP_NONE:
+                break;
+            case IpSecTransform.ENCAP_ESPINUDP:
+            case IpSecTransform.ENCAP_ESPINUDP_NON_IKE:
+                if (mUdpSocketRecords.get(config.getEncapSocketResourceId()) == null) {
+                    throw new IllegalStateException(
+                            "No Encapsulation socket for Resource Id: "
+                                    + config.getEncapSocketResourceId());
+                }
+
+                int port = config.getEncapRemotePort();
+                if (port <= 0 || port > 0xFFFF) {
+                    throw new IllegalArgumentException("Invalid remote UDP port: " + port);
+                }
+                break;
+            default:
+                throw new IllegalArgumentException("Invalid Encap Type: " + config.getEncapType());
+        }
+
+        for (int direction : DIRECTIONS) {
+            IpSecAlgorithm crypt = config.getEncryption(direction);
+            IpSecAlgorithm auth = config.getAuthentication(direction);
+            if (crypt == null && auth == null) {
+                throw new IllegalArgumentException("Encryption and Authentication are both null");
+            }
+
+            if (mSpiRecords.get(config.getSpiResourceId(direction)) == null) {
+                throw new IllegalStateException("No SPI for specified Resource Id");
+            }
+        }
+    }
+
     /**
      * Create a transport mode transform, which represent two security associations (one in each
      * direction) in the kernel. The transform will be cached by the system server and must be freed
@@ -801,17 +901,19 @@ public class IpSecService extends IIpSecService.Stub {
     @Override
     public synchronized IpSecTransformResponse createTransportModeTransform(
             IpSecConfig c, IBinder binder) throws RemoteException {
+        checkIpSecConfig(c);
+        checkNotNull(binder, "Null Binder passed to createTransportModeTransform");
         int resourceId = mNextResourceId.getAndIncrement();
         if (!mUserQuotaTracker.getUserRecord(Binder.getCallingUid()).transform.isAvailable()) {
             return new IpSecTransformResponse(IpSecManager.Status.RESOURCE_UNAVAILABLE);
         }
         SpiRecord[] spis = new SpiRecord[DIRECTIONS.length];
-        // TODO: Basic input validation here since it's coming over the Binder
+
         int encapType, encapLocalPort = 0, encapRemotePort = 0;
         UdpSocketRecord socketRecord = null;
         encapType = c.getEncapType();
         if (encapType != IpSecTransform.ENCAP_NONE) {
-            socketRecord = mUdpSocketRecords.get(c.getEncapLocalResourceId());
+            socketRecord = mUdpSocketRecords.get(c.getEncapSocketResourceId());
             encapLocalPort = socketRecord.getPort();
             encapRemotePort = c.getEncapRemotePort();
         }
@@ -823,20 +925,15 @@ public class IpSecService extends IIpSecService.Stub {
             spis[direction] = mSpiRecords.get(c.getSpiResourceId(direction));
             int spi = spis[direction].getSpi();
             try {
-                mSrvConfig.getNetdInstance()
+                mSrvConfig
+                        .getNetdInstance()
                         .ipSecAddSecurityAssociation(
                                 resourceId,
                                 c.getMode(),
                                 direction,
-                                (c.getLocalAddress() != null)
-                                        ? c.getLocalAddress().getHostAddress()
-                                        : "",
-                                (c.getRemoteAddress() != null)
-                                        ? c.getRemoteAddress().getHostAddress()
-                                        : "",
-                                (c.getNetwork() != null)
-                                        ? c.getNetwork().getNetworkHandle()
-                                        : 0,
+                                c.getLocalAddress(),
+                                c.getRemoteAddress(),
+                                (c.getNetwork() != null) ? c.getNetwork().getNetworkHandle() : 0,
                                 spi,
                                 (auth != null) ? auth.getName() : "",
                                 (auth != null) ? auth.getKey() : null,
@@ -899,12 +996,8 @@ public class IpSecService extends IIpSecService.Stub {
                                 socket.getFileDescriptor(),
                                 resourceId,
                                 direction,
-                                (c.getLocalAddress() != null)
-                                        ? c.getLocalAddress().getHostAddress()
-                                        : "",
-                                (c.getRemoteAddress() != null)
-                                        ? c.getRemoteAddress().getHostAddress()
-                                        : "",
+                                c.getLocalAddress(),
+                                c.getRemoteAddress(),
                                 info.getSpiRecord(direction).getSpi());
             }
         } catch (ServiceSpecificException e) {