pktline: Reject signed length headers like Git does

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitaly:jmrp-pktline-signed-length into master

What does this MR do?

Read() and the splitter behind NewScanner() in internal/git/pktline now decode the four-byte length header with strconv.ParseUint(..., 16, 16), as Payload() in the same file already does, instead of strconv.ParseInt(..., 16, 0). A header with a leading + or - now fails with the existing decode length error. Before, -000 and every header with a leading + decoded to a valid length and came back as packets, while the other - headers decoded to a negative length and failed with invalid length.

Why

ParseInt accepts a sign, so a header that is not four hex digits was decoded as a length (Read(), pktLineSplitter()):

  • +00ahello\n is returned as a 10-byte packet.
  • -000, +000 and +001 are returned as 4-byte magic packets, and IsFlush() is false for all three, so a caller gets a token that is neither a flush packet nor a data packet.

Git accepts only four hex digits and stops with fatal: protocol error: bad line length character on each of these headers (output below). With base 16, ParseInt already refuses a 0x prefix, underscores and spaces, so the sign was the only character it accepted that Git's length decoding does not.

The callers' own checks do not catch it either. newServerConn in internal/grpc/sidechannel refuses the magic packets 0001 to 0003 with sidechannel: invalid header, but when a client sends +001 or -000 followed by 0005x0000, the server on master skips the signed header as an empty data packet and reads x. With this change it gets the decode length error for both. The other callers are the protocol statistics parsers in internal/git/stats, ReadMonitor in this package (the negotiation timeout of SSHUploadPack and SSHUploadArchive), EachSidebandPacket (used by internal/stream), gitaly-lfs-smudge, and the gitaly-debug and benchmarking tools. For a signed header they now stop at the decode length error, as they already did for any other malformed header; ReadMonitor logs it and stops its timer, and the Git command it feeds (git upload-pack or git upload-archive) reads the same bytes and exits with the protocol error either way. I found no impact beyond the divergence itself.

Since the decoded length can no longer be negative, the two < 0 checks go away. The existing negative input length case of TestScanner (-fff) still fails, now while decoding. The splitter has decoded with ParseInt since ddc6b79d and Read() since it was added in 11b5fe09.

Behavior change: Read() and the scanner return an error for -000 and for any header with a leading +, where they used to return a packet; the other - headers still fail, now with decode length instead of invalid length. Valid headers, 0000 and 0001 included, decode as before.

Closes #7501 (closed)

How was this tested?

  • New cases: three signed headers in TestScanner, and in TestRead one subtest per signed header (+00ahello\n, -000, +000, +001) plus one for a length above MaxPktSize, which Read() had no test for although this change rewrites that condition.
  • Against the base commit's pktline.go, the three TestScanner cases and the four signed-header subtests of TestRead fail with An error is expected but got nil, and the MaxPktSize subtest passes there, as it should. With the change all of them pass, with Go 1.26 and 1.27 and under -race.
  • Each of these mutations makes a test fail: ParseInt back in Read(), ParseInt back in the splitter (the existing -fff case catches that one too), and the MaxPktSize check removed from Read().
  • In the CI image (debian-trixie-golang-1.26-rust-1.73:git-2.51), with bundled Git and as an unprivileged user, the way the test job runs: make test-go and make race-go with TEST_PACKAGES set to internal/git/pktline, internal/git/stats, internal/grpc/sidechannel, internal/stream and cmd/gitaly-lfs-smudge (358 tests, two reftable cases skip), and make test-with-sha256 for the stats and sidechannel packages (250 tests), all pass. make lint reports 0 issues., and make format leaves both files unchanged.
Git on the same headers

Against an empty bare repository, with the bundled Git this tree builds (git version 2.55.0-rc1.g1ea786d; the bundled 2.56.0-rc1.g3bbf434 and Git 2.51.0 answer the same):

$ printf '+00ahello\n' | git upload-pack --stateless-rpc ref.git
fatal: protocol error: bad line length character: +00a
$ printf -- '-000' | git upload-pack --stateless-rpc ref.git
fatal: protocol error: bad line length character: -000
$ printf -- '+000' | git upload-pack --stateless-rpc ref.git
fatal: protocol error: bad line length character: +000
$ printf -- '+001' | git upload-pack --stateless-rpc ref.git
fatal: protocol error: bad line length character: +001
$ printf -- '-000' | git receive-pack ref.git
fatal: protocol error: bad line length character: -000
$ printf '0000' | git upload-pack --stateless-rpc ref.git

Each rejected header exits with status 128; the flush packet 0000 exits with 0.

The new tests against the base commit's pktline.go (trimmed to the result lines) 
--- FAIL: TestScanner/length_prefix_with_plus_sign
    Error: An error is expected but got nil.
--- FAIL: TestScanner/magic_packet_with_minus_sign
    Error: An error is expected but got nil.
--- FAIL: TestScanner/magic_packet_with_plus_sign
    Error: An error is expected but got nil.
--- FAIL: TestRead/errors_on_signed_length_header/+00ahello_
    Error: An error is expected but got nil.
--- FAIL: TestRead/errors_on_signed_length_header/-000
    Error: An error is expected but got nil.
--- FAIL: TestRead/errors_on_signed_length_header/+000
    Error: An error is expected but got nil.
--- FAIL: TestRead/errors_on_signed_length_header/+001
    Error: An error is expected but got nil.
--- PASS: TestRead/errors_on_length_exceeding_the_maximum
FAIL	gitlab.com/gitlab-org/gitaly/v19/internal/git/pktline
The sidechannel on master and with this change

A throwaway test in internal/grpc/sidechannel writes each header followed by 0005x0000 into newServerConn() over net.Pipe() and reads the connection to the end:

master:
"0001": server reads "", err sidechannel: invalid header 1
"+001": server reads "x", err <nil>
"-000": server reads "x", err <nil>
"0000": server reads "", err <nil>

with this change:
"0001": server reads "", err sidechannel: invalid header 1
"+001": server reads "", err pktLineSplitter: decode length: strconv.ParseUint: parsing "+001": invalid syntax
"-000": server reads "", err pktLineSplitter: decode length: strconv.ParseUint: parsing "-000": invalid syntax
"0000": server reads "", err <nil>

Merge request reports