qmuntal / gltf

Go library for encoding glTF 2.0 files
https://www.khronos.org/gltf/
BSD 2-Clause "Simplified" License
241 stars 32 forks source link

Avoid out of bounds when calculating b.URI[startPos:] #69

Closed pic4xiu closed 1 year ago

pic4xiu commented 1 year ago

Processing illegal base64 content may cause the program to panic, For example, the following gltf file:

{"buffers": [
    {
      "uri": "data:application/octet-stream;base64",
      "byteLength": 1
    }
  ]
}

When our program calculates the index, the split character is counted by default, and 1 is automatically added, resulting in an out-of-bounds problem in the program.

func (b *Buffer) marshalData() ([]byte, error) {
    if !b.IsEmbeddedResource() {
        return nil, nil
    }
    startPos := len(mimetypeApplicationOctet) + 1//startPos is 37
    //But our b.URI is only 36 in length in this processing.
    sl, err := base64.StdEncoding.DecodeString(b.URI[startPos:])
    if len(sl) == 0 || err != nil {
        return nil, err
    }
    return sl, nil
}

what happened

❯ go run main.go
panic: runtime error: slice bounds out of range [37:36]

goroutine 1 [running]:
github.com/qmuntal/gltf.(*Buffer).marshalData(0x1400012c1e0)
        /Users/housihan/go/pkg/mod/github.com/qmuntal/gltf@v0.24.0/gltf.go:136 +0xdc
qmuntal commented 1 year ago

Thanks for reporting and fixing this issue @pic4xiu.

The fix seems OK, I'm just missing a test case.

pic4xiu commented 1 year ago

Maybe no one would write like this, I discovered it by accidental testing. But it does cause the program to panic and cause trouble for users.

qmuntal commented 1 year ago

Maybe no one would write like this, I discovered it by accidental testing. But it does cause the program to panic and cause trouble for users.

I'm still missing a test case that covers the new code. Can you submit it @pic4xiu? Thanks!

pic4xiu commented 1 year ago

Hello, please take a look to see if this added test case meets the standard?

qmuntal commented 1 year ago

Hello, please take a look to see if this added test case meets the standard?

I does. Merging 😄