RE: Proposal to replace ACK block count with ACK length

["Deval, Manasi" <[e-mail]>]

I have the same question as Eric.

From: Eric Rescorla [mailto:[e-mail]]
Sent: Tuesday, June 19, 2018 12:12 PM
To: Mikkel Fahnøe Jørgensen <[e-mail]>
Cc: Kazuho Oku <[e-mail]>; Deval, Manasi <[e-mail]>; Marten Seemann <[e-mail]>; Praveen Balasubramanian <[e-mail]>; Jana Iyengar <[e-mail]>; IETF QUIC WG <[e-mail]>; Martin Thomson <[e-mail]>; Ian Swett <ianswett=[e-mail]>
Subject: Re: Proposal to replace ACK block count with ACK length




On Tue, Jun 19, 2018 at 11:25 AM, Mikkel Fahnøe Jørgensen <[e-mail]<mailto:[e-mail]>> wrote:
I don’t think you addressed e) at all. Clearly there is an end of buffer check, but that is not the same as requiring additonal checks at many other levels.


Not Manasi, but I don't understand (e).

Here's the relevant section of the Minq decoder.

    func (f *ackFrame) unmarshal(buf []byte) (int, error) {
        // Omitted the section that decoded the header

        // Now decode each block
        for i := uint64(0); i < f.AckBlockCount; i++ {
            blk := &ackBlock{}
            n, err := syntax.Unmarshal(buf, blk)
            if err != nil {
                return 0, err
            }
            buf = buf[n:]
            read += n

            f.AckBlockSection = append(f.AckBlockSection, blk)
        }

        return read, nil
    }

You pass in the entire remainder of the packet and then it parses
out the ACK frame and then returns the amount consumed.

If we were to change to what Manasi proposes, it would look like:

    func (f *ackFrame) unmarshal(buf []byte) (int, error) {
        // Omitted the section that decoded the header

        // Trim the buffer to the remainder length
        if f.AckLength > len(buf) {
                return 0, ProtocolInvalidError
        }
        buf = buf[f.AckLength:]

        for len(buf) > 0 {
            blk := &ackBlock{}
            n, err := syntax.Unmarshal(buf, blk)
            if err != nil {
                return 0, err
            }
            buf = buf[n:]
            read += n

            f.AckBlockSection = append(f.AckBlockSection, blk)
        }

        return read, nil
    }

Is your concern here the additional length check after the
comment "Trim the buffer"? Something else?

-Ekr






On 19 June 2018 at 20.22.03, Deval, Manasi ([e-mail]<mailto:[e-mail]>) wrote:



e.      Every encodable value should be valid

Not every length will be valid. This is inherent to lengths. This same issue ails the ‘payload length’ in QUIC header. Not only does the issue exist for small values, it also applies to large values since data stream will be sent after crypto negotiation.  E.g.  - how does one craft a payload with 62 bit payload length in a large header?