Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b0762eb3 authored by Remi NGUYEN VAN's avatar Remi NGUYEN VAN
Browse files

Ignore DHCP packet sent from non-68 client port 

This differs from previous behavior where dnsmasq would reply to port 68
if the client had no configured address (ciaddr empty in request), or
send replies to the client port if the request ciaddr matched the
assigned lease.
Not all DHCP servers preserve this behavior, and there is no good known
use-case for it. Not replying to such packets is less error-prone and
closer to the standard.

Bug: b/109584964
Test: Added test in DhcpServerTest.py passes
Change-Id: I88d467336cc4f4e4c9498c3787ec22fdef5e1cdd
parent a13007ad

services/net/java/android/net/dhcp/DhcpPacketListener.java

+14 −10
Original line number Diff line number Diff line
@@ -16,15 +16,14 @@ 


package android.net.dhcp;



import android.annotation.NonNull;

import android.annotation.Nullable;

import android.net.util.FdEventsReader;

import android.net.util.PacketReader;

import android.os.Handler;

import android.system.Os;



import java.io.FileDescriptor;

import java.net.Inet4Address;

import java.net.InetAddress;

import java.net.InetSocketAddress;



/**
@@ -35,19 +34,20 @@ abstract class DhcpPacketListener extends FdEventsReader<DhcpPacketListener.Payl 
    static final class Payload {

        final byte[] bytes = new byte[DhcpPacket.MAX_LENGTH];

        Inet4Address srcAddr;

        int srcPort;

    }



    public DhcpPacketListener(Handler handler) {

    public DhcpPacketListener(@NonNull Handler handler) {

        super(handler, new Payload());

    }



    @Override

    protected int recvBufSize(Payload buffer) {

    protected int recvBufSize(@NonNull Payload buffer) {

        return buffer.bytes.length;

    }



    @Override

    protected final void handlePacket(Payload recvbuf, int length) {

    protected final void handlePacket(@NonNull Payload recvbuf, int length) {

        if (recvbuf.srcAddr == null) {

            return;

        }
@@ -55,30 +55,34 @@ abstract class DhcpPacketListener extends FdEventsReader<DhcpPacketListener.Payl 
        try {

            final DhcpPacket packet = DhcpPacket.decodeFullPacket(recvbuf.bytes, length,

                    DhcpPacket.ENCAP_BOOTP);

            onReceive(packet, recvbuf.srcAddr);

            onReceive(packet, recvbuf.srcAddr, recvbuf.srcPort);

        } catch (DhcpPacket.ParseException e) {

            logParseError(recvbuf.bytes, length, e);

        }

    }



    @Override

    protected int readPacket(FileDescriptor fd, Payload packetBuffer) throws Exception {

    protected int readPacket(@NonNull FileDescriptor fd, @NonNull Payload packetBuffer)

            throws Exception {

        final InetSocketAddress addr = new InetSocketAddress();

        final int read = Os.recvfrom(

                fd, packetBuffer.bytes, 0, packetBuffer.bytes.length, 0 /* flags */, addr);



        // Buffers with null srcAddr will be dropped in handlePacket()

        packetBuffer.srcAddr = inet4AddrOrNull(addr);

        packetBuffer.srcPort = addr.getPort();

        return read;

    }



    @Nullable

    private static Inet4Address inet4AddrOrNull(InetSocketAddress addr) {

    private static Inet4Address inet4AddrOrNull(@NonNull InetSocketAddress addr) {

        return addr.getAddress() instanceof Inet4Address

                ? (Inet4Address) addr.getAddress()

                : null;

    }



    protected abstract void onReceive(DhcpPacket packet, Inet4Address srcAddr);

    protected abstract void logParseError(byte[] packet, int length, DhcpPacket.ParseException e);

    protected abstract void onReceive(@NonNull DhcpPacket packet, @NonNull Inet4Address srcAddr,

            int srcPort);

    protected abstract void logParseError(@NonNull byte[] packet, int length,

            @NonNull DhcpPacket.ParseException e);

}

services/net/java/android/net/dhcp/DhcpServer.java

+11 −4
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ package android.net.dhcp; 
import static android.net.NetworkUtils.getBroadcastAddress;

import static android.net.NetworkUtils.getPrefixMaskAsInet4Address;

import static android.net.TrafficStats.TAG_SYSTEM_DHCP_SERVER;

import static android.net.dhcp.DhcpPacket.DHCP_CLIENT;

import static android.net.dhcp.DhcpPacket.DHCP_SERVER;

import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;

import static android.net.dhcp.DhcpPacket.INFINITE_LEASE;
@@ -238,8 +239,14 @@ public class DhcpServer { 
    }



    @VisibleForTesting

    void processPacket(@NonNull DhcpPacket packet) {

        mLog.log("Received packet of type " + packet.getClass().getSimpleName());

    void processPacket(@NonNull DhcpPacket packet, int srcPort) {

        final String packetType = packet.getClass().getSimpleName();

        if (srcPort != DHCP_CLIENT) {

            mLog.logf("Ignored packet of type %s sent from client port %d", packetType, srcPort);

            return;

        }



        mLog.log("Received packet of type " + packetType);

        final Inet4Address sid = packet.mServerIdentifier;

        if (sid != null && !sid.equals(mServingParams.serverAddr.getAddress())) {

            mLog.log("Packet ignored due to wrong server identifier: " + sid);
@@ -469,8 +476,8 @@ public class DhcpServer { 
        }



        @Override

        protected void onReceive(DhcpPacket packet, Inet4Address srcAddr) {

            processPacket(packet);

        protected void onReceive(DhcpPacket packet, Inet4Address srcAddr, int srcPort) {

            processPacket(packet, srcPort);

        }



        @Override

tests/net/java/android/net/dhcp/DhcpServerTest.java

+28 −15
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package android.net.dhcp;



import static android.net.dhcp.DhcpPacket.DHCP_CLIENT;

import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;

import static android.net.dhcp.DhcpPacket.INADDR_ANY;

import static android.net.dhcp.DhcpPacket.INADDR_BROADCAST;
@@ -27,9 +28,11 @@ import static junit.framework.Assert.assertNull; 
import static junit.framework.Assert.assertTrue;



import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyBoolean;

import static org.mockito.ArgumentMatchers.eq;

import static org.mockito.ArgumentMatchers.isNull;

import static org.mockito.Mockito.doNothing;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.times;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;
@@ -178,7 +181,7 @@ public class DhcpServerTest { 
        final DhcpDiscoverPacket discover = new DhcpDiscoverPacket(TEST_TRANSACTION_ID,

                (short) 0 /* secs */, INADDR_ANY /* relayIp */, TEST_CLIENT_MAC_BYTES,

                false /* broadcast */, INADDR_ANY /* srcIp */);

        mServer.processPacket(discover);

        mServer.processPacket(discover, DHCP_CLIENT);



        assertResponseSentTo(TEST_CLIENT_ADDR);

        final DhcpOfferPacket packet = assertOffer(getPacket());
@@ -194,13 +197,22 @@ public class DhcpServerTest { 
        final DhcpDiscoverPacket discover = new DhcpDiscoverPacket(TEST_TRANSACTION_ID,

                (short) 0 /* secs */, INADDR_ANY /* relayIp */, TEST_CLIENT_MAC_BYTES,

                false /* broadcast */, INADDR_ANY /* srcIp */);

        mServer.processPacket(discover);

        mServer.processPacket(discover, DHCP_CLIENT);



        assertResponseSentTo(INADDR_BROADCAST);

        final DhcpNakPacket packet = assertNak(getPacket());

        assertMatchesClient(packet);

    }



    private DhcpRequestPacket makeRequestSelectingPacket() {

        final DhcpRequestPacket request = new DhcpRequestPacket(TEST_TRANSACTION_ID,

                (short) 0 /* secs */, INADDR_ANY /* clientIp */, INADDR_ANY /* relayIp */,

                TEST_CLIENT_MAC_BYTES, false /* broadcast */);

        request.mServerIdentifier = TEST_SERVER_ADDR;

        request.mRequestedIp = TEST_CLIENT_ADDR;

        return request;

    }



    @Test

    public void testRequest_Selecting_Ack() throws Exception {

        when(mRepository.requestLease(isNull() /* clientId */, eq(TEST_CLIENT_MAC),
@@ -208,12 +220,8 @@ public class DhcpServerTest { 
                eq(true) /* sidSet */, isNull() /* hostname */))

                .thenReturn(TEST_LEASE);



        final DhcpRequestPacket request = new DhcpRequestPacket(TEST_TRANSACTION_ID,

                (short) 0 /* secs */, INADDR_ANY /* clientIp */, INADDR_ANY /* relayIp */,

                TEST_CLIENT_MAC_BYTES, false /* broadcast */);

        request.mServerIdentifier = TEST_SERVER_ADDR;

        request.mRequestedIp = TEST_CLIENT_ADDR;

        mServer.processPacket(request);

        final DhcpRequestPacket request = makeRequestSelectingPacket();

        mServer.processPacket(request, DHCP_CLIENT);



        assertResponseSentTo(TEST_CLIENT_ADDR);

        final DhcpAckPacket packet = assertAck(getPacket());
@@ -227,24 +235,29 @@ public class DhcpServerTest { 
                eq(true) /* sidSet */, isNull() /* hostname */))

                .thenThrow(new InvalidAddressException("Test error"));



        final DhcpRequestPacket request = new DhcpRequestPacket(TEST_TRANSACTION_ID,

                (short) 0 /* secs */, INADDR_ANY /* clientIp */, INADDR_ANY /* relayIp */,

                TEST_CLIENT_MAC_BYTES, false /* broadcast */);

        request.mServerIdentifier = TEST_SERVER_ADDR;

        request.mRequestedIp = TEST_CLIENT_ADDR;

        mServer.processPacket(request);

        final DhcpRequestPacket request = makeRequestSelectingPacket();

        mServer.processPacket(request, DHCP_CLIENT);



        assertResponseSentTo(INADDR_BROADCAST);

        final DhcpNakPacket packet = assertNak(getPacket());

        assertMatchesClient(packet);

    }



    @Test

    public void testRequest_Selecting_WrongClientPort() throws Exception {

        final DhcpRequestPacket request = makeRequestSelectingPacket();

        mServer.processPacket(request, 50000);



        verify(mRepository, never()).requestLease(any(), any(), any(), any(), anyBoolean(), any());

        verify(mDeps, never()).sendPacket(any(), any(), any());

    }



    @Test

    public void testRelease() throws Exception {

        final DhcpReleasePacket release = new DhcpReleasePacket(TEST_TRANSACTION_ID,

                TEST_SERVER_ADDR, TEST_CLIENT_ADDR,

                INADDR_ANY /* relayIp */, TEST_CLIENT_MAC_BYTES);

        mServer.processPacket(release);

        mServer.processPacket(release, DHCP_CLIENT);



        verify(mRepository, times(1))

                .releaseLease(isNull(), eq(TEST_CLIENT_MAC), eq(TEST_CLIENT_ADDR));