Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternate fix for oob slice creation in proxy halfpipe #254

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

jmwample
Copy link
Member

@jmwample jmwample commented Oct 19, 2023

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

@jmwample jmwample merged commit d9d5826 into master Oct 23, 2023
11 checks passed
@jmwample jmwample deleted the halfipe-simple branch October 23, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant