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).

Re: Stop ping telling world its pid

[Ted Unangst]

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.

Re: Stop ping telling world its pid

[Job Snijders]

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

Re: Stop ping telling world its pid

[Theo de Raadt]

"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.

Re: Stop ping telling world its pid

[Theo de Raadt]

We never claimed that OpenBSD ping conforms to the
'High Availability ICMP ECHO' RFC.

Jimmy Hess  wrote:

> On Tue, Apr 10, 2018 at 3:11 PM, Vadim Zhukov  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 + Sequencefields  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 valuesbased 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 onInitial 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
> 

Re: Stop ping telling world its pid

[Jimmy Hess]

On Tue, Apr 10, 2018 at 3:11 PM, Vadim Zhukov  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 + Sequencefields  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 valuesbased 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 onInitial 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

Re: Stop ping telling world its pid

[Florian Obser]

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.c8 Nov 2017 17:27:39 -   1.224
> +++ ping.c10 Apr 2018 20:03:50 -
> @@ -586,7 +586,7 @@ main(int argc, char *argv[])
>   for (i = ECHOTMLEN; i < datalen; ++i)
>   *datap++ = i;
>  
> - ident = getpid() & 0x;
> + ident = arc4random() & 0x;
>  
>   /*
>* 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.