[PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Salam Noureddine]

Gratuitous arp packets are useful in switchover scenarios to update
client arp tables as quickly as possible. Currently, the mac address
of a neighbour is only updated after a locktime period has elapsed
since the last update. In most use cases such delays are unacceptable
for network admins. Moreover, the "updated" field of the neighbour
stucture doesn't record the last time the address of a neighbour
changed but records any change that happens to theneighbour. This is
clearly a bug since locktime uses that field as meaning "addr_updated".
With this observation, I was able to perpetuate a stale address by
sending a stream of gratuitous arp packets spaced less than locktime
apart.

Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
---
 net/ipv4/arp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 7808093..ab13347 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -910,7 +910,10 @@ static int arp_process(struct sk_buff *skb)
 		   agents are active. Taking the first reply prevents
 		   arp trashing and chooses the fastest router.
 		 */
-		override = time_after(jiffies, n->updated + n->parms->locktime);
+		override = time_after(jiffies,
+				      n->updated + n->parms->locktime) ||
+			   (tip == sip &&
+			    inet_addr_type(net, sip) == RTN_UNICAST);
 
 		/* Broadcast replies and request packets
 		   do not assert neighbour reachability.
-- 
1.7.4.4

Re: [PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Hannes Frederic Sowa]

On Thu, Dec 19, 2013 at 09:41:12PM -0800, Salam Noureddine wrote:
> Gratuitous arp packets are useful in switchover scenarios to update
> client arp tables as quickly as possible. Currently, the mac address
> of a neighbour is only updated after a locktime period has elapsed
> since the last update. In most use cases such delays are unacceptable
> for network admins. Moreover, the "updated" field of the neighbour
> stucture doesn't record the last time the address of a neighbour
> changed but records any change that happens to theneighbour. This is
> clearly a bug since locktime uses that field as meaning "addr_updated".
> With this observation, I was able to perpetuate a stale address by
> sending a stream of gratuitous arp packets spaced less than locktime
> apart.
> 
> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
> ---
>  net/ipv4/arp.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 7808093..ab13347 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -910,7 +910,10 @@ static int arp_process(struct sk_buff *skb)
>  		   agents are active. Taking the first reply prevents
>  		   arp trashing and chooses the fastest router.
>  		 */
> -		override = time_after(jiffies, n->updated + n->parms->locktime);
> +		override = time_after(jiffies,
> +				      n->updated + n->parms->locktime) ||
> +			   (tip == sip &&
> +			    inet_addr_type(net, sip) == RTN_UNICAST);
>  
>  		/* Broadcast replies and request packets
>  		   do not assert neighbour reachability.

Hm, that is hard to decipher in a few months (or years). Maybe a small
function like

static bool is_garp(...)
{
	...
}

would self-document the code. They normally get inlined so there is no
additional runtime overhead.

Thanks,

  Hannes

[PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Salam Noureddine]

Gratuitous arp packets are useful in switchover scenarios to update
client arp tables as quickly as possible. Currently, the mac address
of a neighbour is only updated after a locktime period has elapsed
since the last update. In most use cases such delays are unacceptable
for network admins. Moreover, the "updated" field of the neighbour
stucture doesn't record the last time the address of a neighbour
changed but records any change that happens to the neighbour. This is
clearly a bug since locktime uses that field as meaning "addr_updated".
With this observation, I was able to perpetuate a stale address by
sending a stream of gratuitous arp packets spaced less than locktime
apart.

Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
---
 net/ipv4/arp.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 7808093..6597708 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -904,13 +904,19 @@ static int arp_process(struct sk_buff *skb)
 	if (n) {
 		int state = NUD_REACHABLE;
 		int override;
+		bool is_garp = tip == sip &&
+			       inet_addr_type(net, sip) == RTN_UNICAST;
 
 		/* If several different ARP replies follows back-to-back,
 		   use the FIRST one. It is possible, if several proxy
 		   agents are active. Taking the first reply prevents
-		   arp trashing and chooses the fastest router.
+		   arp trashing and chooses the fastest router. In the
+		   case of gratuitous arp we always set override in order
+		   to update the address.
 		 */
-		override = time_after(jiffies, n->updated + n->parms->locktime);
+		override = time_after(jiffies,
+				      n->updated + n->parms->locktime) ||
+			   is_garp;
 
 		/* Broadcast replies and request packets
 		   do not assert neighbour reachability.
-- 
1.7.4.4

Re: [PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Stephen Hemminger]

On Fri, 20 Dec 2013 10:59:22 -0800
Salam Noureddine <noureddine@aristanetworks.com> wrote:

> Gratuitous arp packets are useful in switchover scenarios to update
> client arp tables as quickly as possible. Currently, the mac address
> of a neighbour is only updated after a locktime period has elapsed
> since the last update. In most use cases such delays are unacceptable
> for network admins. Moreover, the "updated" field of the neighbour
> stucture doesn't record the last time the address of a neighbour
> changed but records any change that happens to the neighbour. This is
> clearly a bug since locktime uses that field as meaning "addr_updated".
> With this observation, I was able to perpetuate a stale address by
> sending a stream of gratuitous arp packets spaced less than locktime
> apart.
> 
> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>

Doesn't this make the system more vulnerable to ARP spoofing?

Re: [PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Salam Noureddine]

I could make is_garp conditional on ARP_ACCEPT. Would that be a more
acceptable solution?
Otherwise, if some sort of rate limiting is needed we would have to
add an addr_updated field
to the neighbour structure that would save the time when the address
was updated and not just
any update to the neighbour.

Thanks,

Salam

On Fri, Dec 20, 2013 at 2:06 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Fri, 20 Dec 2013 10:59:22 -0800
> Salam Noureddine <noureddine@aristanetworks.com> wrote:
>
>> Gratuitous arp packets are useful in switchover scenarios to update
>> client arp tables as quickly as possible. Currently, the mac address
>> of a neighbour is only updated after a locktime period has elapsed
>> since the last update. In most use cases such delays are unacceptable
>> for network admins. Moreover, the "updated" field of the neighbour
>> stucture doesn't record the last time the address of a neighbour
>> changed but records any change that happens to the neighbour. This is
>> clearly a bug since locktime uses that field as meaning "addr_updated".
>> With this observation, I was able to perpetuate a stale address by
>> sending a stream of gratuitous arp packets spaced less than locktime
>> apart.
>>
>> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
>
> Doesn't this make the system more vulnerable to ARP spoofing?

Re: [PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Hannes Frederic Sowa]

On Fri, Dec 20, 2013 at 02:06:17PM -0800, Stephen Hemminger wrote:
> On Fri, 20 Dec 2013 10:59:22 -0800
> Salam Noureddine <noureddine@aristanetworks.com> wrote:
> 
> > Gratuitous arp packets are useful in switchover scenarios to update
> > client arp tables as quickly as possible. Currently, the mac address
> > of a neighbour is only updated after a locktime period has elapsed
> > since the last update. In most use cases such delays are unacceptable
> > for network admins. Moreover, the "updated" field of the neighbour
> > stucture doesn't record the last time the address of a neighbour
> > changed but records any change that happens to the neighbour. This is
> > clearly a bug since locktime uses that field as meaning "addr_updated".
> > With this observation, I was able to perpetuate a stale address by
> > sending a stream of gratuitous arp packets spaced less than locktime
> > apart.
> > 
> > Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
> 
> Doesn't this make the system more vulnerable to ARP spoofing?

Yes it does, but also it is supposed to work like this with garp, I fear.
Making this conditional on some sysctl would not help either, because
normal behaviour for clients should be, that e.g. the router switches
and sends out garp, that the client should pick those announcements up
as soon as possible. That is also how arp_notify is supposed to work.

Greetings,

  Hannes

Re: [PATCH 1/1] ipv4: arp: Always update neighbour address when a gratuitous arp is received

[Hannes Frederic Sowa]

On Fri, Dec 20, 2013 at 10:59:22AM -0800, Salam Noureddine wrote:
> Gratuitous arp packets are useful in switchover scenarios to update
> client arp tables as quickly as possible. Currently, the mac address
> of a neighbour is only updated after a locktime period has elapsed
> since the last update. In most use cases such delays are unacceptable
> for network admins. Moreover, the "updated" field of the neighbour
> stucture doesn't record the last time the address of a neighbour
> changed but records any change that happens to the neighbour. This is
> clearly a bug since locktime uses that field as meaning "addr_updated".
> With this observation, I was able to perpetuate a stale address by
> sending a stream of gratuitous arp packets spaced less than locktime
> apart.
> 
> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
> ---
>  net/ipv4/arp.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 7808093..6597708 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -904,13 +904,19 @@ static int arp_process(struct sk_buff *skb)
>  	if (n) {
>  		int state = NUD_REACHABLE;
>  		int override;
> +		bool is_garp = tip == sip &&
> +			       inet_addr_type(net, sip) == RTN_UNICAST;

Would it make sense to add the following?

				&& arp->arp_op == htons(ARPOP_REQUEST);

Maybe we could also check that target hardware address is the broadcast
address, so we don't accept targetted arp spoofing attempts?

Just extract *tha like we did for sha.

Otherwise I am fine with that.

Thanks,

  Hannes