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: <CAHx7fy5bSghKONyYSW-4oXbEKLHUxYC7vE=ZiKLXUED-iuuCdw@mail.gmail.com>
Date:   Mon, 10 Jun 2019 16:51:34 -0700
From:   Zhongjie Wang <zwang048@....edu>
To:     Neal Cardwell <ncardwell@...gle.com>
Cc:     Netdev <netdev@...r.kernel.org>, Zhiyun Qian <zhiyunq@...ucr.edu>
Subject: Re: tp->copied_seq used before assignment in tcp_check_urg

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?

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.

Best Regards,
Zhongjie Wang
Ph.D. Candidate 2015 Fall
Department of Computer Science & Engineering
University of California, Riverside

Zhongjie Wang
Ph.D. Candidate 2015 Fall
Department of Computer Science & Engineering
University of California, Riverside

On Mon, Jun 10, 2019 at 5:00 AM Neal Cardwell <ncardwell@...gle.com> wrote:
>
> On Sun, Jun 9, 2019 at 11:12 PM Zhongjie Wang <zwang048@....edu> wrote:
> ...
> > It compares tp->copied_seq with tcp->rcv_nxt.
> > However, tp->copied_seq is only assigned to an appropriate sequence number when
> > it copies data to user space. So here tp->copied_seq could be equal to 0,
> > which is its initial value, if no data are copied yet.
>
> I don't believe that's the case. As far as I can see, the
> tp->copied_seq field is initialized to tp->rcv_nxt in the various
> places where TCP connections are initialized:
>
>   tp->copied_seq = tp->rcv_nxt;
>
> > In this case, the condition becomes 0 != tp->rcv_nxt,
> > and it renders this comparison ineffective.
> >
> > For example, if we send a SYN packet with initial sequence number 0xFF FF FF FF,
> > and after receiving SYN/ACK response, then send a ACK packet with sequence
> > number 0, it will bypass this if-then block.
> >
> > We are not sure how this would affect the TCP logic. Could you please confirm
> > that tp->copied_seq should be assigned to a sequence number before its use?
>
> Yes, the tp->copied_seq  ought to be assigned to a sequence number
> before its use, and AFAICT it is. Can you identify a specific sequence
> of code execution (and ideally construct a packetdrill script) where
> tp->copied_seq is somehow read before it is initialized?
>
> cheers,
> neal