From 8e38d8c24d77a03de9e6f1c699bfb257832d6eea Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Wed, 1 Mar 2023 16:54:29 +0100 Subject: [PATCH] Fix ReadAt with offset==EOF (#51) * Fix ReadAt with offset==EOF Fix a problem when a `ReadAt` requests a read at the offset that in plaintext corresponds exactly to a block starting at the EOF. We forward to where the block *would* be, thereby we never read the previous block header, and therefore do not get the "final" marker set We fix by reading (and skipping) the previous block if a request starts exactly at a block boundary to pick up the "final" marker. We calculate `t`, to be the number of full blocks to skip and `k` to be the number of bytes to skip before the response starts: ``` t := offset / int64(maxPayloadSize) k := offset % int64(maxPayloadSize) ``` We add the following if we start at a block boundary after the first block: ``` if offset > 0 && k == 0 { k = maxPayloadSize t-- } ``` This will read the previous block to read the "final" flag and skip its content. --- .github/workflows/codeql.yml | 47 ++++++++++ .github/workflows/go.yml | 35 ++++--- .golangci.yml | 4 +- cmd/ncrypt/main.go | 8 +- go.mod | 8 +- go.sum | 14 ++- reader-v1.go | 3 +- reader-v2.go | 10 +- sio_test.go | 175 ++++++++++++++++++----------------- writer-v1.go | 6 +- 10 files changed, 188 insertions(+), 122 deletions(-) create mode 100644 .github/workflows/codeql.yml diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..4ee967f --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,47 @@ +name: "Code scanning - action" + +on: + push: + pull_request: + schedule: + - cron: '0 19 * * 0' + +jobs: + CodeQL-Build: + + # CodeQL runs on ubuntu-latest and windows-latest + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v2 + with: + # We must fetch at least the immediate parents so that if this is + # a pull request then we can checkout the head. + fetch-depth: 2 + + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v2 + # Override language selection by uncommenting this and choosing your languages + # with: + # languages: go, javascript, csharp, python, cpp, java + + # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). + # If this step fails, then you should remove it and run the build manually (see below) + - name: Autobuild + uses: github/codeql-action/autobuild@v2 + + # ℹī¸ Command-line programs to run using the OS shell. + # 📚 https://git.io/JvXDl + + # ✏ī¸ If the Autobuild fails above, remove it and uncomment the following three lines + # and modify them (or add more) to build your code if your project + # uses a compiled language + + #- run: | + # make bootstrap + # make release + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v2 diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 4cdd173..773bf4e 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -10,44 +10,48 @@ on: jobs: build: - name: Build Go ${{ matrix.go-version }} + name: Lint ${{ matrix.go-version }} runs-on: ubuntu-latest strategy: matrix: - go-version: [1.15.x, 1.16.x] + go-version: [1.20.x] steps: - name: Set up Go ${{ matrix.go-version }} - uses: actions/setup-go@v1 + uses: actions/setup-go@v2 with: go-version: ${{ matrix.go-version }} id: go - name: Check out code into the Go module directory - uses: actions/checkout@v1 + uses: actions/checkout@v2 - - name: Build - env: - GO111MODULE: on + - name: go vet + run: go vet ./... + + - name: go fmt + run: diff <(gofmt -d .) <(printf "") + + - name: golangci-lint run: | - curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.33.0 - $(go env GOPATH)/bin/golangci-lint run --config ./.golangci.yml - go vet ./... + curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.51.2 + $(go env GOPATH)/bin/golangci-lint run --config ./.golangci.yml + test: name: Testing Go ${{ matrix.go-version }} on ${{ matrix.os }} runs-on: ${{ matrix.os }} strategy: matrix: - go-version: [1.16.x] + go-version: [1.18.x, 1.19.x, 1.20.x] os: [ubuntu-latest, windows-latest, macos-latest] steps: - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }} - uses: actions/setup-go@v1 + uses: actions/setup-go@v2 with: go-version: ${{ matrix.go-version }} id: go - name: Check out code into the Go module directory - uses: actions/checkout@v1 + uses: actions/checkout@v2 - name: Test on ${{ matrix.os }} env: @@ -55,3 +59,8 @@ jobs: run: | go test ./... + - name: Test on race ${{ matrix.os }} + env: + GO111MODULE: on + run: | + go test -race ./... \ No newline at end of file diff --git a/.golangci.yml b/.golangci.yml index 0cf218c..7c28294 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -12,14 +12,12 @@ linters: - goimports - misspell - govet - - golint - ineffassign - gosimple - - deadcode - unused - - structcheck - prealloc - unconvert + - revive issues: exclude-use-default: false diff --git a/cmd/ncrypt/main.go b/cmd/ncrypt/main.go index a4f2afc..769c2bc 100644 --- a/cmd/ncrypt/main.go +++ b/cmd/ncrypt/main.go @@ -19,10 +19,10 @@ // // Usage: ncrypt [FLAGS] [ARGUMENTS...] // -// -cipher string Specify cipher - default: platform depended -// -d Decrypt -// -list List supported algorithms -// -p string Specify the password - default: prompt for password +// -cipher string Specify cipher - default: platform depended +// -d Decrypt +// -list List supported algorithms +// -p string Specify the password - default: prompt for password // // Examples: // diff --git a/go.mod b/go.mod index 5dd3b8f..f7d66e6 100644 --- a/go.mod +++ b/go.mod @@ -1,8 +1,10 @@ module github.com/minio/sio require ( - golang.org/x/crypto v0.0.0-20190513172903-22d7a77e9e5f - golang.org/x/sys v0.0.0-20190412213103-97732733099d + golang.org/x/crypto v0.6.0 + golang.org/x/sys v0.5.0 ) -go 1.13 +require golang.org/x/term v0.5.0 // indirect + +go 1.18 diff --git a/go.sum b/go.sum index 99e1dc3..7dc8bcc 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,6 @@ -golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= -golang.org/x/crypto v0.0.0-20190513172903-22d7a77e9e5f h1:R423Cnkcp5JABoeemiGEPlt9tHXFfw5kvc0yqlxRPWo= -golang.org/x/crypto v0.0.0-20190513172903-22d7a77e9e5f/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= -golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= -golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= -golang.org/x/sys v0.0.0-20190412213103-97732733099d h1:+R4KGOnez64A81RvjARKc4UT5/tI9ujCIVX+P5KiHuI= -golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +golang.org/x/crypto v0.6.0 h1:qfktjS5LUO+fFKeJXZ+ikTRijMmljikvG68fpMMruSc= +golang.org/x/crypto v0.6.0/go.mod h1:OFC/31mSvZgRz0V1QTNCzfAI1aIRzbiufJtkMIlEp58= +golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= +golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/term v0.5.0 h1:n2a8QNdAb0sZNpU9R1ALUXBbY+w51fCQDN+7EdxNBsY= +golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= diff --git a/reader-v1.go b/reader-v1.go index 433ee60..e20233d 100644 --- a/reader-v1.go +++ b/reader-v1.go @@ -17,7 +17,6 @@ package sio import ( "errors" "io" - "io/ioutil" ) type encReaderV10 struct { @@ -237,7 +236,7 @@ func (r *decReaderAtV10) ReadAt(p []byte, offset int64) (n int, err error) { } decReader.SeqNum = uint32(t) if k := offset % int64(maxPayloadSize); k > 0 { - if _, err := io.CopyN(ioutil.Discard, &decReader, k); err != nil { + if _, err := io.CopyN(io.Discard, &decReader, k); err != nil { return 0, err } } diff --git a/reader-v2.go b/reader-v2.go index 23cdf44..b8794bc 100644 --- a/reader-v2.go +++ b/reader-v2.go @@ -17,7 +17,6 @@ package sio import ( "errors" "io" - "io/ioutil" "sync" ) @@ -291,6 +290,11 @@ func (r *decReaderAtV20) ReadAt(p []byte, offset int64) (n int, err error) { if t+1 > (1<<32)-1 { return 0, errUnexpectedSize } + k := offset % int64(maxPayloadSize) + if offset > 0 && k == 0 { + k = maxPayloadSize + t-- + } decReader := decReaderV20{ authDecV20: r.ad, @@ -300,8 +304,8 @@ func (r *decReaderAtV20) ReadAt(p []byte, offset int64) (n int, err error) { } defer decReader.recycle() decReader.SeqNum = uint32(t) - if k := offset % int64(maxPayloadSize); k > 0 { - if _, err := io.CopyN(ioutil.Discard, &decReader, k); err != nil { + if k > 0 { + if _, err := io.CopyN(io.Discard, &decReader, k); err != nil { return 0, err } } diff --git a/sio_test.go b/sio_test.go index 3e6f55f..96bd8d9 100644 --- a/sio_test.go +++ b/sio_test.go @@ -20,7 +20,6 @@ import ( "encoding/hex" "fmt" "io" - "io/ioutil" "os" "path/filepath" "testing" @@ -178,50 +177,52 @@ func TestReader(t *testing.T) { for _, version := range versions { config.MinVersion, config.MaxVersion = version, version for i, test := range ioTests { - data, buffer := make([]byte, test.datasize), make([]byte, test.buffersize) - if _, err := io.ReadFull(rand.Reader, data); err != nil { - t.Fatalf("Version %d: Test %d: Failed to generate random data: %v", version, i, err) - } - - encReader, err := EncryptReader(bytes.NewReader(data), config) - if err != nil { - t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) - } - decReader, err := DecryptReader(encReader, config) - if err != nil { - t.Fatalf("Version %d: Test %d: Failed to create decrypted reader: %v", version, i, err) - } - - _, err = io.ReadFull(decReader, buffer) - if err == io.ErrUnexpectedEOF && test.buffersize < test.datasize { - t.Errorf("Version %d: Test %d: Reading failed: %v", version, i, err) - } - if err != nil && err != io.ErrUnexpectedEOF { - t.Errorf("Version %d: Test %d: Reading failed: %v", version, i, err) - } + t.Run(fmt.Sprintf("v%x-%d", version, test.datasize), func(t *testing.T) { + data, buffer := make([]byte, test.datasize), make([]byte, test.buffersize) + if _, err := io.ReadFull(rand.Reader, data); err != nil { + t.Fatalf("Version %d: Test %d: Failed to generate random data: %v", version, i, err) + } - if version == Version20 { - ciphertext := bytes.NewBuffer(nil) - encReader, err = EncryptReader(bytes.NewReader(data), config) + encReader, err := EncryptReader(bytes.NewReader(data), config) if err != nil { t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) } - if _, err = io.Copy(ciphertext, encReader); err != nil { - t.Fatalf("Version %d: Test %d: Failed to encrypted data: %v", version, i, err) - } - - plaintext := bytes.NewBuffer(nil) - decReaderAt, err := DecryptReaderAt(bytes.NewReader(ciphertext.Bytes()), config) + decReader, err := DecryptReader(encReader, config) if err != nil { t.Fatalf("Version %d: Test %d: Failed to create decrypted reader: %v", version, i, err) } - if _, err = io.Copy(plaintext, io.NewSectionReader(decReaderAt, 0, int64(ciphertext.Len()))); err != nil { - t.Fatalf("Version %d: Test %d: Failed to encrypted data: %v", version, i, err) + + _, err = io.ReadFull(decReader, buffer) + if err == io.ErrUnexpectedEOF && test.buffersize < test.datasize { + t.Errorf("Version %d: Test %d: Reading failed: %v", version, i, err) } - if !bytes.Equal(data, plaintext.Bytes()) { - t.Fatalf("Version %d: Test %d: The plaintexts do not match: %v", version, i, err) + if err != nil && err != io.ErrUnexpectedEOF { + t.Errorf("Version %d: Test %d: Reading failed: %v", version, i, err) } - } + + if version == Version20 { + ciphertext := bytes.NewBuffer(nil) + encReader, err = EncryptReader(bytes.NewReader(data), config) + if err != nil { + t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) + } + if _, err = io.Copy(ciphertext, encReader); err != nil { + t.Fatalf("Version %d: Test %d: Failed to encrypted data: %v", version, i, err) + } + + plaintext := bytes.NewBuffer(nil) + decReaderAt, err := DecryptReaderAt(bytes.NewReader(ciphertext.Bytes()), config) + if err != nil { + t.Fatalf("Version %d: Test %d: Failed to create decrypted reader: %v", version, i, err) + } + if _, err = io.Copy(plaintext, io.NewSectionReader(decReaderAt, 0, int64(ciphertext.Len()))); err != nil { + t.Fatalf("Version %d: Test %d: Failed to encrypted data: %v", version, i, err) + } + if !bytes.Equal(data, plaintext.Bytes()) { + t.Fatalf("Version %d: Test %d: The plaintexts do not match: %v", version, i, err) + } + } + }) } } } @@ -233,31 +234,33 @@ func TestReaderAt(t *testing.T) { for _, version := range versions { config.MinVersion, config.MaxVersion = version, version for i, test := range ioTests { - plaintext.Reset() - ciphertext.Reset() + t.Run(fmt.Sprintf("v%x-%d", version, test.datasize), func(t *testing.T) { + plaintext.Reset() + ciphertext.Reset() - data := make([]byte, test.datasize) - encReader, err := EncryptReader(bytes.NewReader(data), config) - if err != nil { - t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) - } - if _, err = io.Copy(ciphertext, encReader); err != nil { - t.Fatalf("Version %d: Test %d: Failed to encrypted data: %v", version, i, err) - } + data := make([]byte, test.datasize) + encReader, err := EncryptReader(bytes.NewReader(data), config) + if err != nil { + t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) + } + if _, err = io.Copy(ciphertext, encReader); err != nil { + t.Fatalf("Version %d: Test %d: Failed to encrypted data: %v", version, i, err) + } - decReaderAt, err := DecryptReaderAt(bytes.NewReader(ciphertext.Bytes()), config) - if err != nil { - t.Fatalf("Version %d: Test %d: Failed to create decrypted reader: %v", version, i, err) - } - if _, err = io.Copy(plaintext, io.NewSectionReader(decReaderAt, 0, int64(test.datasize/2))); err != nil { - t.Fatalf("Version %d: Test %d: Failed to decrypted data: %v", version, i, err) - } - if _, err = io.Copy(plaintext, io.NewSectionReader(decReaderAt, int64(test.datasize/2), int64(test.datasize))); err != nil { - t.Fatalf("Version %d: Test %d: Failed to decrypted data: %v", version, i, err) - } - if !bytes.Equal(data, plaintext.Bytes()) { - t.Fatalf("Version %d: Test %d: The plaintexts do not match", version, i) - } + decReaderAt, err := DecryptReaderAt(bytes.NewReader(ciphertext.Bytes()), config) + if err != nil { + t.Fatalf("Version %d: Test %d: Failed to create decrypted reader: %v", version, i, err) + } + if _, err = io.Copy(plaintext, io.NewSectionReader(decReaderAt, 0, int64(test.datasize/2))); err != nil { + t.Fatalf("Version %d: Test %d: Failed to decrypted data: %v", version, i, err) + } + if _, err = io.Copy(plaintext, io.NewSectionReader(decReaderAt, int64(test.datasize/2), int64(test.datasize))); err != nil { + t.Fatalf("Version %d: Test %d: Failed to decrypted data: %v", version, i, err) + } + if !bytes.Equal(data, plaintext.Bytes()) { + t.Fatalf("Version %d: Test %d: The plaintexts do not match", version, i) + } + }) } } } @@ -342,31 +345,33 @@ func TestCopy(t *testing.T) { for _, version := range versions { config.MinVersion, config.MaxVersion = version, version for i, test := range ioTests { - data, buffer := make([]byte, test.datasize), make([]byte, test.buffersize) - if _, err := io.ReadFull(rand.Reader, data); err != nil { - t.Fatalf("Version %d: Test %d: Failed to generate random data: %v", version, i, err) - } + t.Run(fmt.Sprintf("v%x-%d", version, test.datasize), func(t *testing.T) { + data, buffer := make([]byte, test.datasize), make([]byte, test.buffersize) + if _, err := io.ReadFull(rand.Reader, data); err != nil { + t.Fatalf("Version %d: Test %d: Failed to generate random data: %v", version, i, err) + } - output := bytes.NewBuffer(nil) + output := bytes.NewBuffer(nil) - decWriter, err := DecryptWriter(output, config) - if err != nil { - t.Fatalf("Version %d: Test %d: Failed to create decrypted writer: %v", version, i, err) - } - encReader, err := EncryptReader(bytes.NewReader(data), config) - if err != nil { - t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) - } + decWriter, err := DecryptWriter(output, config) + if err != nil { + t.Fatalf("Version %d: Test %d: Failed to create decrypted writer: %v", version, i, err) + } + encReader, err := EncryptReader(bytes.NewReader(data), config) + if err != nil { + t.Fatalf("Version %d: Test %d: Failed to create encrypted reader: %v", version, i, err) + } - if _, err := io.CopyBuffer(decWriter, encReader, buffer); err != nil { - t.Fatalf("Version %d: Test: %d: Failed to copy: %v", version, i, err) - } - if err := decWriter.Close(); err != nil { - t.Fatalf("Version %d: Test: %d: Failed to close writer: %v", version, i, err) - } - if !bytes.Equal(data, output.Bytes()) { - t.Fatalf("Version %d: Test: %d: Failed to encrypt and decrypt data", version, i) - } + if _, err := io.CopyBuffer(decWriter, encReader, buffer); err != nil { + t.Fatalf("Version %d: Test: %d: Failed to copy: %v", version, i, err) + } + if err := decWriter.Close(); err != nil { + t.Fatalf("Version %d: Test: %d: Failed to close writer: %v", version, i, err) + } + if !bytes.Equal(data, output.Bytes()) { + t.Fatalf("Version %d: Test: %d: Failed to encrypt and decrypt data", version, i) + } + }) } } } @@ -435,7 +440,7 @@ func TestVerifySequenceNumbers(t *testing.T) { } func testFile(t *testing.T, file string) { - data, err := ioutil.ReadFile(file) + data, err := os.ReadFile(file) if err != nil { t.Errorf("Failed to read file: %s - %v", file, err) } @@ -560,7 +565,7 @@ func TestLargeStream(t *testing.T) { if err != nil { t.Fatalf("Failed to create decrypted reader %v", err) } - decWriter, err := DecryptWriter(ioutil.Discard, config) + decWriter, err := DecryptWriter(io.Discard, config) if err != nil { t.Fatalf("Failed to create decrypted writer %v", err) } @@ -676,7 +681,7 @@ func benchmarkEncryptRead(size int64, b *testing.B) { if err != nil { b.Fatal(err) } - _, err = io.Copy(ioutil.Discard, reader) + _, err = io.Copy(io.Discard, reader) if err != nil && err != io.ErrUnexpectedEOF { b.Fatal(err) } @@ -708,7 +713,7 @@ func benchmarkDecryptRead(size int64, b *testing.B) { if err != nil { b.Fatal(err) } - _, err = io.Copy(ioutil.Discard, reader) + _, err = io.Copy(io.Discard, reader) if err != nil && err != io.EOF { b.Fatal(err) } diff --git a/writer-v1.go b/writer-v1.go index e97a711..5aa247a 100644 --- a/writer-v1.go +++ b/writer-v1.go @@ -35,10 +35,14 @@ func decryptWriterV10(dst io.Writer, config *Config) (*decWriterV10, error) { if err != nil { return nil, err } + buf := packageBufferPool.Get().([]byte)[:maxPackageSize] + for i := range buf { + buf[i] = 0 + } return &decWriterV10{ authDecV10: ad, dst: dst, - buffer: packageBufferPool.Get().([]byte)[:maxPackageSize], + buffer: buf, }, nil }