Fix five bugs found in a verified review of the tree - #9
Merged
Conversation
…ion support, and update version to 2.0.0; improve README and example configuration
ZRLE (encoding 16): - Per-connection continuous zlib stream (zrle.go) with Z_SYNC_FLUSH after each framebuffer update, as the RFB spec requires. - 64x64 tiling with solid (subencoding 1) and raw (subencoding 0) tiles; flat desktop areas collapse to a single CPIXEL. - 3-byte CPIXEL packing for 32bpp true-colour formats whose colour bits fit in the low 3 bytes; falls back to Raw otherwise. - Client encoding list is now parsed in SetEncodings to detect ZRLE support; FramebufferUpdateRequest picks ZRLE when negotiated. - Round-trip test decodes the zlib/tile stream and verifies pixels, including partial edge tiles and both subencodings. Fixes: - Sequential rotation no longer skips the first image: startup logging peeked via GetImage(), which advanced the sequential counter. - KeyEvent/PointerEvent/ClientCutText are now drained with their correct lengths instead of a blind 255-byte read that desynced the protocol stream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extend the ZRLE tile encoder beyond solid/raw with the full set of subencodings: packed palette (2-16 colours, 1/2/4 bits per index), palette RLE (130-255) and plain RLE (128). Each tile picks the cheapest option by pre-zlib byte cost, the heuristic real encoders use. The round-trip test now decodes every subencoding and the test image is structured to force each path. A bandwidth test measures real images: on a busy desktop screenshot palette/RLE shaves ~9% off the solid+raw stream, ~23% on simpler images, and the whole ZRLE path stays at a few percent of the original uncompressed Raw encoding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- framebuffer: converter writes into a caller buffer instead of allocating a 3-4 byte slice per pixel; sendFramebuffer no longer copies through a temporary. Removes ~w*h allocations per update. - addIPOverlay: load the BGRX source into the NRGBA buffer as RGBA with opaque alpha and convert back afterwards. Previously the base was treated as fully transparent, so the semi-transparent IP banner rendered as a solid black bar and channels were only correct by luck. - main: build port-range listen addresses with net.SplitHostPort/ JoinHostPort so IPv6 hosts no longer break (strings.Split on ":"). - server: recover() in the per-connection goroutine so malformed client input can't take down the whole process. - Tests: Raw framebuffer round-trip (locks the converter refactor) and an IP-overlay test (background darkened, pixels below the banner untouched). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Remove the duplicated Features section and the mojibake (replacement characters) left by a bad encoding round-trip. - Add a ZRLE bullet and an Encodings section explaining ZRLE/Raw negotiation and view-only input handling. - Document build.sh for multi-platform release builds; drop the stale clone URL from the build steps. Note the ~3MB binary size. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- github.com/BurntSushi/toml v1.5.0 -> v1.6.0 - golang.org/x/image v0.27.0 -> v0.41.0 (resolves Dependabot advisory) - golang.org/x/text v0.25.0 -> v0.37.0 (indirect) The go directive moves to 1.25.0 because the updated x/image and x/text both require it. Verified: go vet clean, all tests pass, and build.sh cross-compiles all seven release targets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bug fixes:
- install.sh built only main.go, so the package never compiled
("undefined: appVersion" and 7 more). Build the package instead, and
fix the FLAGS array so -w is not dropped from the release ldflags.
- The release workflow pinned Go 1.24 while go.mod requires 1.25, so
every tagged build failed. It also copied a config.toml that does not
exist in the repo, with the error swallowed by "|| true", shipping
archives without a config. Package config.example.toml as config.toml.
- Connections had no write deadline, so a client that stopped reading
(TCP zero-window) pinned a goroutine and, with the IP overlay enabled,
a full framebuffer copy forever. Bound every send.
- Incremental FramebufferUpdateRequests were answered with a full frame.
Clients re-request as soon as each update lands, so this spun at 100%
CPU and saturated the link for an image that never changes. Only the
first update is now unconditional; later incremental requests are
ignored, as a real server would.
- The client IP overlay split RemoteAddr on ":", yielding "[" for IPv6
peers. Use net.SplitHostPort.
- Image paths resolved against the working directory while the config
path defaulted to the executable's directory, so a service started
outside the install dir found its config but not its images. Resolve
images against the config file's directory; absolute paths are kept.
- With no server able to start, "select {}" left the process alive and
silent forever. Listeners are now bound synchronously, a zero-listener
start exits non-zero, and SIGINT/SIGTERM shut down cleanly.
- getSequential did a non-atomic load-then-increment and handed the same
image to concurrent connections. Claim and advance in one step.
- ClientCutText trusted a uint32 length: int(n) goes negative on the
386 targets build.sh produces, desyncing the stream. Cap at 1 MiB.
- An unknown message type drained one byte and kept parsing a stream it
could no longer interpret. Close the connection instead.
Config keys:
- show_ip -> show_client_ip
- no_brand -> branding (inverted, defaults to true)
- server_name -> name (matching the global key)
The 2.0 spellings still load and log a deprecation warning. The global
name is now used as the branding prefix; previously it was unreachable
because the per-server fallback to the section id always won.
Adds tests for config loading and key deprecation, listen address
expansion, incremental-request suppression, cut-text limits, unknown
message handling, IPv6 peer parsing, and sequential rotation under
concurrency (the last one fails against the previous implementation).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
- golang.org/x/image v0.41.0 -> v0.44.0 - actions/checkout v4 -> v7 - actions/setup-go v4 -> v7 - softprops/action-gh-release v2 -> v3 github.com/BurntSushi/toml is already at the latest release (v1.6.0). The Go version pin stays at "1.25", which resolves to the newest 1.25.x patch and matches the go directive in go.mod. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
Bumps the go directive and the CI toolchain pin. The full cross-platform build.sh matrix (linux/windows/darwin, amd64/arm64/386) and go test -race both pass on the new toolchain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
setup-go now resolves the newest 1.26.x at build time, so security patches land without a commit. go.mod keeps go 1.26.5 as the minimum the module requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
The overlay was a hardcoded 360x22 box with the fixed 7x13 bitmap face: unreadably small on a wallpaper-sized image, and either cutting the text off or leaving a wide empty bar depending on the address length. The banner is now measured from its contents. The font is Go Mono at a size proportional to the image height (clamped to 11-40px), the box is the widest line plus a proportional padding, and the size steps down until the box fits the image width — so a full IPv6 address or a long hostname shrinks to fit instead of running off the edge. Go Mono and opentype both ship inside golang.org/x/image, so this adds no module. The banner takes any number of lines, driven by three global options: show_client_ip the client address (unchanged default: off) show_rdns the client's reverse-DNS name show_time the connection timestamp show_rdns and show_time default to off. The PTR lookup only runs when it will be displayed, so the default configuration generates no DNS traffic; when enabled it is bounded at 700ms and falls back to "(no PTR record)". README documents the tradeoff: the lookup precedes the handshake and queries the client's own DNS authority. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
Replaces 43 log.Printf calls with slog. No new dependency: log/slog is stdlib, and the binary grows 3.58 -> 3.88 MB. The important change is what gets logged. A connection used to produce six unrelated lines prefixed with "[Acme - Reception]", which cannot be correlated and cannot be parsed. It now produces one event carrying the whole session: peer_ip, peer_port, rdns, client_version, security_type, handshake, encodings, encoding_used, pixel_bpp, pixel_depth, image, updates, bytes_sent, duration_ms, outcome Three of those were already being read and thrown away: the client's version string, the security type it picked, and the full encoding list. The encoding list in the client's own order is the best fingerprint of which VNC software is on the other end, which is the whole point of running a honeypot. Per-message protocol detail moves to debug level, so info is exactly one record per connection. outcome is a small stable vocabulary (client_eof, idle_timeout, unknown_message, version_read_failed, update_write_failed, ...) so it groups cleanly, and handshake separates real clients from probes that open a socket and vanish. Bytes are counted by wrapping the connection. New [logging] section: level, format (json|text) and output (stdout, stderr or a file path). Shipping to Elasticsearch or Loki needs no code here — JSON on stdout is what every collector already consumes. A file sink is reopened on SIGHUP so logrotate works; without it the server would keep writing to the rotated-away inode. loadConfig now returns deprecation warnings instead of logging them, since the logger does not exist until the config has been read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
The only workflow in the repository triggered on v* tags, so nothing was
ever checked before a release: tests, vet and formatting never ran on a
pull request or a push.
Adds a CI workflow on pull_request and pushes to main/dev with two jobs:
test gofmt check, go vet, go test -race, and a go mod tidy diff so a
stale go.sum cannot land
build the full build.sh matrix, because a cross-compile break on the
386 or darwin targets would otherwise only surface at tag time
In-flight runs for a branch are cancelled when it is pushed again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
Closes the three items left open by the original review. Handshake validation. The client's version string and its chosen security type were both read and then ignored. A greeting that is not a well-formed "RFB xxx.yyy" is now refused instead of being parsed as though the rest of the stream were protocol, and RFB 3.3 is refused explicitly: those revisions have the server choose the security type over a different message flow, so serving them a 3.7+ negotiation desyncs. A client that selects a type that was never offered gets a proper RFB 3.8 failure result — status 1 with a reason — rather than a silent success. Each case has its own outcome (malformed_version, unsupported_version, bad_security_type), so misbehaving scanners separate cleanly from real clients in the log. Connection limit. New global max_connections, default 512, 0 for unlimited. The limiter is process-wide rather than per listener because the resource at risk is memory: with the info banner enabled every connection holds a private framebuffer copy, which for a 1080p image is about 8 MB. Clients over the cap are closed before any greeting and recorded with outcome connection_limit. An absent key takes the default while an explicit 0 survives, so switching the cap off stays possible. Removes ImageRotator.GetStats, which was never called. Tests cover version parsing and support boundaries, all three rejection paths end to end, the limiter semantics including the nil (unlimited) case, slot release after a client disconnects, and max_connections defaulting. The three rejection tests were checked against the previous behaviour and fail there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
The overlay was built as soon as a connection arrived: a full framebuffer copy (~8 MB at 1080p) plus, with show_rdns, a PTR lookup — all before a single protocol byte was exchanged. Any TCP connect paid that price, and under the default max_connections of 512 the worst case was ~4 GB of overlay copies held by clients that might never request a frame. The pre-handshake lookup also delayed the RFB greeting, which is itself a tell for whoever is probing. The overlay (and its lookup) is now built on the first FramebufferUpdateRequest and reused for later updates. ServerInit is answered from the original image, whose dimensions the overlay preserves. Scanners that connect and vanish — most traffic on an exposed honeypot — now cost neither the copy nor any DNS traffic, and the greeting is never delayed. Image selection stays per-connection so sequential rotation semantics are unchanged. lookupRDNS becomes a stubable variable so tests can observe when a resolution actually happens. New tests pin the behaviour: a client that handshakes without requesting a frame triggers no lookup (fails against the eager build), and a client that does request one gets a frame with the banner actually drawn, with exactly one lookup across repeated updates. README and the example config no longer describe the pre-handshake lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
Actions were disabled repository-wide when the CI workflow first landed, so no run was ever created for it. This push both adds a manual trigger for future re-runs and, as a PR synchronize event, kicks off the first real run now that Actions are enabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
Every push to main now builds the full platform matrix and publishes it as a pre-release under a rolling "dev" tag, so the latest main build is always downloadable without cutting a version tag. The tag moves to the newest commit rather than accumulating one release per push. Version stamping. appVersion becomes a var so build.sh can inject a value via -ldflags -X when VERSION is set; a plain build still reports the baseline. Dev builds report e.g. 2.1.0-dev.gabc1234 (the "g" keeps the semver pre-release identifier alphanumeric), and tagged releases now report the tag instead of whatever was hardcoded. The base version is read from config.go, so it stays the single source of truth. Packaging moves out of release.yml into package.sh, shared by both the tagged-release and dev-build workflows instead of being duplicated. The dev workflow also gets a workflow_dispatch trigger for manual runs, and a concurrency group so a newer push cancels an in-flight dev build. dist/ is gitignored since package.sh writes there. build.sh, package.sh and version injection were exercised locally: the binary reports the injected version, a plain build reports the baseline, and the archives unpack to binary + config.toml + images/default.png. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
show_client_ip, show_rdns and show_time were global only, so a config with several servers had to enable the banner everywhere or nowhere. That is a real limitation for a multi-honeypot setup: you generally want the banner on the hosts you are watching and nothing on the ones meant to look untouched. The three keys are now accepted on a server too, where they override the [global] default. Server fields are *bool rather than bool so that three states are distinguishable — inherit, force on, force off. A plain bool could not express "switch a globally enabled line back off", since its zero value is indistinguishable from an absent key. Verified end to end: two servers in one config, one inheriting a global show_client_ip = true and one overriding it to false, serve frames that differ in exactly the banner pixels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
A mistyped key was silently dropped, so the option simply looked broken
with nothing in the log to explain it; a mistyped section name produced a
misleading "check listen addresses and image paths". Both are now caught
through the decoder's Undecoded(), which reports typos at every level:
[global], [logging], a section name, a key inside a server, and a key
inside an inline image table. Deprecated keys are real struct fields, so
they keep producing their own notices rather than being flagged as typos.
Also warns where settings silently override one another — rotation_mode
outside {random, sequential}, image together with images, and a port in
listen together with a port range — since each of those otherwise just
discards what was written.
--check parses the config, loads every image and prints a summary
without binding a port, so it can run against a live host. It exits
non-zero for anything that would stop a server starting: a missing or
corrupt image, no listen address, no [server.*] sections, or two servers
claiming the same address, which previously only surfaced as a bind
failure at startup. Warnings alone do not fail it.
Warnings are emitted in a stable order; they are collected while walking
a map, so server ids are sorted to keep runs reproducible.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RaJTvGD9ysDpoMxa6kNTL5
An adversarial, verified review of the branch surfaced five real defects. Each fix comes with a test that fails against the old code. - RFB 3.7 handshake desync: the server always sent the RFB 3.8 SecurityResult (4 zero bytes) for the None type, even to a 3.7 client that negotiated 3.7. 3.7 has no SecurityResult for None, so those 4 bytes shifted every ServerInit field and desynced the client. Gate the result (and the reason-string failure) on the negotiated version via usesRFB38Handshake. - 24bpp raw colours swapped: converter()'s 24bpp path emitted R,G,B verbatim, ignoring the negotiated shifts and endianness, so a standard little-endian client saw red and blue transposed. Build the value from the pixel format like the 32bpp and ZRLE paths do. - --check missed invalid logging config: a bad logging.level or logging.format passed --check (exit 0) but crashed the real start. runCheck now validates both, matching setupLogging. - Unbounded port range: an out-of-range or huge start_port/end_port passed --check and either failed to bind silently or drove a multi-gigabyte slice allocation (even under --check). Bound the range to 1..65535 in listenAddrs and warn in loadConfig. - Redundant re-encode of the immutable frame: every non-incremental FramebufferUpdateRequest re-ran the whole encode pipeline. Cache the encoded body per (frame, pixel format, encoding) and reuse it; the cache invalidates on a pixel-format change and the zlib stream is still flushed each update. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NcMUkcFtUzutgo95s1c9B7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A request to "check that everything is OK and there are no new bugs" turned into an adversarial, verified review of the current tree. Six candidate defects were found; five were confirmed against the code and one (an alleged eager ~700 KB zlib allocation per connection) was refuted —
zlib.NewWriterbuilds the flate compressor lazily on first write, so idle handshakes don't pay for it.Baseline before and after:
go build,go vet,gofmt -l, andgo test -raceall pass. Each fix ships with a test that fails against the old code.Fixes
1. RFB 3.7 handshake desync (
server.go,protocol.go) — The server always sent the RFB 3.8SecurityResult(4 zero bytes) for theNonesecurity type, even when the client negotiated 3.7. RFB 3.7 has noSecurityResultforNoneand goes straight toClientInit/ServerInit, so those 4 bytes shifted everyServerInitfield by four and desynced a conformant 3.7 client. Now gated on the negotiated version viausesRFB38Handshake(the reason-string failure path is gated too).2. 24bpp colours swapped (
framebuffer.go) —converter()'s 24bpp path wroteR,G,Bverbatim, ignoring the negotiated shifts and endianness (unlike the 32bpp and ZRLE paths). A standard little-endian 24bpp client saw red and blue transposed. Now builds the pixel value from the pixel format and serialises per endianness.3.
--checkskipped logging validation (check.go) — An invalidlogging.levelorlogging.formatpassed--checkwith exit 0 but then crashed the real start insetupLogging.runChecknow validates both, restoring its contract that a config it green-lights can actually boot.4. Unbounded port range (
main.go,config.go) — An out-of-range or hugestart_port/end_portpassed--checkand either failed to bind silently or drove a multi-gigabyte slice allocation — even under--check. The range is now bounded to1..65535inlistenAddrs(so it never expands out of range) andloadConfigwarns on out-of-range or inverted ranges.5. Redundant re-encode of the immutable frame (
server.go,zrle.go,framebuffer.go) — Every non-incrementalFramebufferUpdateRequestre-ran the whole encode pipeline for a framebuffer that never changes. The encoded body is now cached per(frame, pixel format, encoding)and reused; the cache invalidates on a pixel-format change, and the ZRLE zlib stream is still flushed on every update so the continuous stream stays valid. This is a pure memoization — the wire bytes are byte-identical to before.Tests added
TestConverter24bppHonorsPixelFormat— 24bpp byte order for both endiannessesTestRFB37HandshakeSkipsSecurityResult,TestRFB37BadSecurityTypeClosesWithoutResultTestCheckRejectsBadLogLevel,TestCheckRejectsBadLogFormat,TestCheckAcceptsValidLoggingTestPortRangeValidationWarns+ newTestListenAddrscases (boundary, out-of-range, absurd value)TestFrameCacheInvalidatesOnPixelFormatChange,TestZRLESessionUpdateDecodesSeverity note
All five are real but modest in blast radius for a honeypot: the handshake and 24bpp issues affect only the less-common RFB 3.7 / 24bpp-on-the-wire clients (fidelity/detectability, not the core logging), the two validation gaps are operator-config footguns, and the re-encode is bounded by the attacker's own download bandwidth. None affect connection logging, which keeps working throughout.
🤖 Generated with Claude Code
Generated by Claude Code