Re: Proposal to replace ACK block count with ACK length

[Eric Rescorla <[e-mail]>]

On Tue, Jun 19, 2018 at 11:25 AM, Mikkel Fahnøe Jørgensen <
[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]) 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?
>
>