Stop ping telling world its pid

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Stop ping telling world its pid

Vadim Zhukov
Hi all!

While working on home job for students, I've come across two
questionable thingies in ping.c:

1. It sends process PID (well, last 16 bits) to the network.
   Maybe I'm a bit paranoid, but this looks like bad idea for me.
   I understand that this worked good when PIDs were 16-bit limited,
   because in that case you'll get 100% guarantee two different
   ping processes won't overlap. But nowadays we have larger PID
   space, so clashes are possible anyway. I propose to go straight
   with arc4random().

2. There is no point in calling ntohs/htons on the ident value.
   Or we should do this in IPv4 too, then: I'm not opposed, but
   at least consistency should be honored, IMHO.

Okay? Comments?

--
WBR,
  Vadim Zhukov

P.S.: I'm not fully back yet, sorry.


Index: ping.c
===================================================================
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.224
diff -u -p -r1.224 ping.c
--- ping.c 8 Nov 2017 17:27:39 -0000 1.224
+++ ping.c 10 Apr 2018 20:03:50 -0000
@@ -586,7 +586,7 @@ main(int argc, char *argv[])
  for (i = ECHOTMLEN; i < datalen; ++i)
  *datap++ = i;
 
- ident = getpid() & 0xFFFF;
+ ident = arc4random() & 0xFFFF;
 
  /*
  * When trying to send large packets, you must increase the
@@ -1033,7 +1033,7 @@ pinger(int s)
  icp6->icmp6_cksum = 0;
  icp6->icmp6_type = ICMP6_ECHO_REQUEST;
  icp6->icmp6_code = 0;
- icp6->icmp6_id = htons(ident);
+ icp6->icmp6_id = ident;
  icp6->icmp6_seq = seq;
  } else {
  icp = (struct icmp *)outpack;
@@ -1151,7 +1151,7 @@ pr_pack(u_char *buf, int cc, struct msgh
  }
 
  if (icp6->icmp6_type == ICMP6_ECHO_REPLY) {
- if (ntohs(icp6->icmp6_id) != ident)
+ if (icp6->icmp6_id != ident)
  return; /* 'Twas not our ECHO */
  seq = icp6->icmp6_seq;
  echo_reply = 1;

Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Florian Obser-2

On Tue, Apr 10, 2018 at 11:11:39PM +0300, Vadim Zhukov wrote:

> Hi all!
>
> While working on home job for students, I've come across two
> questionable thingies in ping.c:
>
> 1. It sends process PID (well, last 16 bits) to the network.
>    Maybe I'm a bit paranoid, but this looks like bad idea for me.
>    I understand that this worked good when PIDs were 16-bit limited,
>    because in that case you'll get 100% guarantee two different
>    ping processes won't overlap. But nowadays we have larger PID
>    space, so clashes are possible anyway. I propose to go straight
>    with arc4random().
>
> 2. There is no point in calling ntohs/htons on the ident value.
>    Or we should do this in IPv4 too, then: I'm not opposed, but
>    at least consistency should be honored, IMHO.
>
> Okay? Comments?

OK florian@

>
> --
> WBR,
>   Vadim Zhukov
>
> P.S.: I'm not fully back yet, sorry.
>
>
> Index: ping.c
> ===================================================================
> RCS file: /cvs/src/sbin/ping/ping.c,v
> retrieving revision 1.224
> diff -u -p -r1.224 ping.c
> --- ping.c 8 Nov 2017 17:27:39 -0000 1.224
> +++ ping.c 10 Apr 2018 20:03:50 -0000
> @@ -586,7 +586,7 @@ main(int argc, char *argv[])
>   for (i = ECHOTMLEN; i < datalen; ++i)
>   *datap++ = i;
>  
> - ident = getpid() & 0xFFFF;
> + ident = arc4random() & 0xFFFF;
>  
>   /*
>   * When trying to send large packets, you must increase the
> @@ -1033,7 +1033,7 @@ pinger(int s)
>   icp6->icmp6_cksum = 0;
>   icp6->icmp6_type = ICMP6_ECHO_REQUEST;
>   icp6->icmp6_code = 0;
> - icp6->icmp6_id = htons(ident);
> + icp6->icmp6_id = ident;
>   icp6->icmp6_seq = seq;
>   } else {
>   icp = (struct icmp *)outpack;
> @@ -1151,7 +1151,7 @@ pr_pack(u_char *buf, int cc, struct msgh
>   }
>  
>   if (icp6->icmp6_type == ICMP6_ECHO_REPLY) {
> - if (ntohs(icp6->icmp6_id) != ident)
> + if (icp6->icmp6_id != ident)
>   return; /* 'Twas not our ECHO */
>   seq = icp6->icmp6_seq;
>   echo_reply = 1;
>

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Jimmy Hess
In reply to this post by Vadim Zhukov
On Tue, Apr 10, 2018 at 3:11 PM, Vadim Zhukov <[hidden email]> wrote:

> While working on home job for students, I've come across two
> questionable thingies in ping.c:
>
> 1. It sends process PID (well, last 16 bits) to the network.
>    Maybe I'm a bit paranoid, but this looks like bad idea for me.

Well, you need a UNIQUE  ID,  because all readers of an ICMP socket are
going to see All the ICMP echo replies,  including ones that belong to other
programs  (maybe not ping),  and  possibly some unsolicited replies.

The standard convention of using the locally-significant PID should be fine.
The only thought would be that if an adversary knows your process id,
such as by running  a  "ps"  command on the host  -- since the PID is not
a secret identifier,  well.

your adversary could then construct and send  Spoofed replies to
your ping commands's ICMP ECHO request  from somewhere else on
the internet,  And knowing your PID,  they would be able to put the
correct Ident value in their spoofed ECHO REPLY packets.

However,  there's no real-world case where it's really a concern ---
unless there were some bug where ping  could misbehave on a malformed reply.

>    I understand that this worked good when PIDs were 16-bit limited,
>    because in that case you'll get 100% guarantee two different
>    ping processes won't overlap. But nowadays we have larger PID
>    space, so clashes are possible anyway. I propose to go straight
>    with arc4random().

Probably using the bottom 16 bits of the PID for most systems is ensuring
uniqueness,  and  arc4random()  is introducing  unreliability/unpredictability
in when Ident values may  overlap.....


Perhaps the ideal situation would be to use the whole  combined
Ident + Sequence    fields  for matching.

In other words:  the ICMP REPLY should be discarded unless both  Ident
 AND  Sequence bits
in the reply  are within a certain range of values    based on packets sent.

Then..... populate the whole 32-bit  PID with upper 16 bits to Ident and
Lower  16 bits  into the   Initial Sequence bits.

And for reply Match Ident,
Then  test  if the  Sequence Bits  in the response are in the valid reply range
 based on    Initial Sequence  --> Final Sequence,

Or  Initial Sequence +  The count of sent pings   where ping would increment
 Sequence for each Ping sent,   with consideration for Sequence
counter wrap/overflow.


--
-Jimmy

Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Theo de Raadt-2
We never claimed that OpenBSD ping conforms to the
'High Availability ICMP ECHO' RFC.

Jimmy Hess <[hidden email]> wrote:

> On Tue, Apr 10, 2018 at 3:11 PM, Vadim Zhukov <[hidden email]> wrote:
>
> > While working on home job for students, I've come across two
> > questionable thingies in ping.c:
> >
> > 1. It sends process PID (well, last 16 bits) to the network.
> >    Maybe I'm a bit paranoid, but this looks like bad idea for me.
>
> Well, you need a UNIQUE  ID,  because all readers of an ICMP socket are
> going to see All the ICMP echo replies,  including ones that belong to other
> programs  (maybe not ping),  and  possibly some unsolicited replies.
>
> The standard convention of using the locally-significant PID should be fine.
> The only thought would be that if an adversary knows your process id,
> such as by running  a  "ps"  command on the host  -- since the PID is not
> a secret identifier,  well.
>
> your adversary could then construct and send  Spoofed replies to
> your ping commands's ICMP ECHO request  from somewhere else on
> the internet,  And knowing your PID,  they would be able to put the
> correct Ident value in their spoofed ECHO REPLY packets.
>
> However,  there's no real-world case where it's really a concern ---
> unless there were some bug where ping  could misbehave on a malformed reply.
>
> >    I understand that this worked good when PIDs were 16-bit limited,
> >    because in that case you'll get 100% guarantee two different
> >    ping processes won't overlap. But nowadays we have larger PID
> >    space, so clashes are possible anyway. I propose to go straight
> >    with arc4random().
>
> Probably using the bottom 16 bits of the PID for most systems is ensuring
> uniqueness,  and  arc4random()  is introducing  unreliability/unpredictability
> in when Ident values may  overlap.....
>
>
> Perhaps the ideal situation would be to use the whole  combined
> Ident + Sequence    fields  for matching.
>
> In other words:  the ICMP REPLY should be discarded unless both  Ident
>  AND  Sequence bits
> in the reply  are within a certain range of values    based on packets sent.
>
> Then..... populate the whole 32-bit  PID with upper 16 bits to Ident and
> Lower  16 bits  into the   Initial Sequence bits.
>
> And for reply Match Ident,
> Then  test  if the  Sequence Bits  in the response are in the valid reply range
>  based on    Initial Sequence  --> Final Sequence,
>
> Or  Initial Sequence +  The count of sent pings   where ping would increment
>  Sequence for each Ping sent,   with consideration for Sequence
> counter wrap/overflow.
>
>
> --
> -Jimmy
>

Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Theo de Raadt-2
In reply to this post by Jimmy Hess
"The standard convention of using the locally-significant PID should be fine."
     Jimmy Hess, 2018

A whole bunch of protocols got holed because of that attitude.

Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Job Snijders
In reply to this post by Theo de Raadt-2
When things arrive out of sequence, that usually is of special interest to
network operator people. Not sure  the sequence field can easily be
overloaded to increase “validity”.

I’m not great at math, with a 16 bit random value, wouldn’t we start
running into ID collisions around 256 concurrent ping processes? Perhaps
that already is the case today and this patch does nothing to improve that,
but also doesn’t make it worse.

Kind regards,

Job
Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Ted Unangst-6
Job Snijders wrote:
> I’m not great at math, with a 16 bit random value, wouldn’t we start
> running into ID collisions around 256 concurrent ping processes? Perhaps
> that already is the case today and this patch does nothing to improve that,
> but also doesn’t make it worse.

what we need is an eventually consistent 16 bit number microservice message
bus that all ping processes can subscribe to.

Reply | Threaded
Open this post in threaded view
|

Re: Stop ping telling world its pid

Miod Vallat
> what we need is an eventually consistent 16 bit number microservice message
> bus that all ping processes can subscribe to.

And it should be available as early as possible during boot, so I think
its place is in init(8).