refraction-networking / conjure

Conjure Refraction Networking station code
https://refraction.network
Apache License 2.0
70 stars 21 forks source link

Alternate fix for oob slice creation in proxy halfpipe #254

Closed jmwample closed 1 year ago

jmwample commented 1 year ago

It is somehow possible for the proxy to make an out of bounds access in the halfpipe slice creation. It is not clear how this is possible since we are calling read with a fixed size buffer, but the issue seems to have occurred on several stations independently.

12:44:51 conjure: panic: runtime error: slice bounds out of range [:32804] with capacity 32768
12:44:51 conjure: goroutine 192061459 [running]:
12:44:51 conjure: github.com/refraction-networking/conjure/pkg/station/lib.halfPipe({0xbcdb28, 0xc003cac040}, {0xbcd618, 0xc0164223c0}, 0xc009744520, 0xc000246b20, {0xc01682a8b8, 0x13}, 0xc005a89680)
12:44:51 conjure:         /opt/conjure/conjure/pkg/station/lib/proxies.go:158 +0x870
12:44:51 conjure: created by github.com/refraction-networking/conjure/pkg/station/lib.Proxy in goroutine 192061268
12:44:51 conjure:         /opt/conjure/conjure/pkg/station/lib/proxies.go:266 +0x5a5

The offending code is here

    buf := make([]byte, 32*1024)
    for {
        nr, er := src.Read(buf)
        if nr > 0 {
            nw, ew := dst.Write(buf[0:nr])

            // Update stats:
            stats.addBytes(int64(nw), isUpload)
            if isUpload {
                Stat().AddBytesUp(int64(nw))
            } else {
                Stat().AddBytesDown(int64(nw))
            }

            if ew == nil && nw != nr {
                ew = io.ErrShortWrite
            }

This PR provides a simple change that:

  1. checks the error before accessing the buffer
  2. ensures that the slice size is never larger than the buffer size.

Update: while this PR prevents crashes the core issue was the incorrect length returned from reads in the dtls heartbeat package. This was fixed in #256, however this PR is still relevant for stability in case of unreliable readers in the future.


PR #255 implements a more complex, but potentially more complete solution