netdev - Re: tp->copied_seq used before assignment in tcp_check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADVnQy=P=P1iPxrgqQ1U5xwY7Wj3H54XF1sfTyi92mQkLgjb6g@mail.gmail.com>
Date:   Mon, 10 Jun 2019 22:19:12 -0400
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Zhongjie Wang <zwang048@....edu>
Cc:     Netdev <netdev@...r.kernel.org>, Zhiyun Qian <zhiyunq@...ucr.edu>
Subject: Re: tp->copied_seq used before assignment in tcp_check_urg

On Mon, Jun 10, 2019 at 7:48 PM Zhongjie Wang <zwang048@....edu> wrote:
>
> Hi Neal,
>
> Thanks for your reply. Sorry, I made a mistake in my previous email.
> After I double checked the source code, I think it should be tp->urg_seq,
> which is used before assignment, instead of tp->copied_seq.
> Still in the same if-statement:
>
> 5189     if (tp->urg_seq == tp->copied_seq && tp->urg_data &&
> 5190         !sock_flag(sk, SOCK_URGINLINE) && tp->copied_seq != tp->rcv_nxt) {
> 5191         struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
> 5192         tp->copied_seq++;
> 5193         if (skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq)) {
> 5194             __skb_unlink(skb, &sk->sk_receive_queue);
> 5195             __kfree_skb(skb);   // wzj(a)
> 5196         }
> 5197     }
> 5198
> 5199     tp->urg_data = TCP_URG_NOTYET;
> 5200     tp->urg_seq = ptr;
>
> It compares tp->copied_seq with tp->urg_seq.
> And I found only 1 assignment of tp->urg_seq in the code base,
> which is after the if-statement in the same tcp_check_urg() function.
>
> So it seems tp->urg_seq is not assigned to any sequence number before
> its first use.
> Is that correct?

I agree, it does seem that tp->urg_seq is not assigned to any sequence
number before its first use.

AFAICT from a quick read of the code, this does not matter. It seems
the idea is for tp->urg_data and tp->urg_seq to be set and used
together, so that tp->urg_seq is never relied upon to be set to
something meaningful unless tp->urg_data has also been verified to be
set to something (something non-zero).

I suppose it might be more clear to structure the code to check urg_data first:

  if (tp->urg_data && tp->urg_seq == tp->copied_seq &&

...but in practice AFAICT it does not make a difference, since no
matter which order the expressions use, both conditions must be true
for the code to have any side effects.

> P.S. In our symbolic execution tool, we found an execution path that
> requires the client initial sequence number (ISN) to be 0xFF FF FF FF.
> And when it traverse that path, the tp->copied_seq is equal to (client
> ISN + 1), and compared with 0 in this if-statatement.
> Therefore the client ISN has to be exactly 0xFF FF FF FF to hit this
> execution path.
>
> To trigger this, we first sent a SYN packet, and then an ACK packet
> with urgent pointer.

Does your test show any invalid behavior by the TCP endpoint? For
example, does the state in tcp_sock become incorrect, or is some
system call return value or outgoing packet incorrect? AFAICT from the
scenario you describe it seems that the "if" condition would fail when
the receiver processes the ACK packet with urgent pointer, because
tp->urg_data was not yet set at this point. Thus it would seem that in
this case it does not matter that tp->urg_seq is not assigned to any
sequence number before being first used.

cheers,
neal