Harden inbound handling and fix IRCv3/SASL conformance - #5
Draft
SuperManifolds wants to merge 11 commits into
Draft
Harden inbound handling and fix IRCv3/SASL conformance#5SuperManifolds wants to merge 11 commits into
SuperManifolds wants to merge 11 commits into
Conversation
A malformed or hostile server could crash the client by sending messages with fewer parameters than a handler expected: nearly every handler indexed `message.parameters[N]` directly, and an out-of-bounds access is a fatal trap. Add two layers of defence: - A minimum-parameter-count guard at dispatch (`IRCReply.minimumParameterCount`) that drops undersized messages before any handler runs. - `[safe:]` + `guard let` in the handlers themselves, so safety does not depend on the dispatch table being exhaustive. Also fix the remaining traps in the same class: the duration-only STS `["port"]!` unwrap, the `PREFIX` parenthesis unwraps in `IRCChannelUserMode.map`, `IRCUser(fromPrivateMessage:)` (now failable, so a server-sourced PRIVMSG is dropped rather than crashing), the MODE `sender!`, and a tag-parser trap on malformed keys such as `@=;=`. In passing, the tag parser no longer truncates values containing `=`, CHGHOST-for-self is written back to `currentSender`, and the PART reason uses the correct parameter index. Add MalformedMessageTests covering 34 previously-crashing lines, and make the test target compile under Swift 6.
Two advertised features were dead on the receive path: - INVITE was routed to `handleChannelKickEvent`, and the dedicated `handleChannelInviteEvent` was never called. That handler also read the parameters in KICK order; INVITE is `<invited nick> <channel>`. Route INVITE correctly, read the parameters in the right order, and fall back to a transient channel so the notification fires for invites to channels we have not joined. - RPL_MONONLINE (730) and RPL_MONOFFLINE (731) had handlers but no dispatch cases, so MONITOR online/offline notifications were silently dropped. Wire them up. Add EventDispatchTests covering invite-notify field mapping and MONITOR online/offline dispatch.
Tag values were parsed and emitted verbatim, so any value containing a semicolon, space, backslash, CR or LF corrupted the tag frame outbound and was read wrong inbound. Add `String.ircTagValueEscaped()` / `ircTagValueUnescaped()` implementing the message-tags escape table (`\\`, `\:`, `\s`, `\r`, `\n`, with unknown escapes resolving to the bare character and a trailing lone backslash dropped). Unescape values when parsing inbound tags and escape them when building outbound tags. Add MessageTagTests covering the escape table, edge cases, round-tripping, and inbound parsing integration.
The test target previously held only an empty example and could not exercise any of the wire-parsing, authentication, or capability logic. Add: - IRCMessageParsingTests: prefix/command/parameter parsing, server vs user prefixes, trailing parameters with spaces and colons, tags, CTCP/ACTION detection, unknown-command rejection, and server-time parsing. - SASLTests: mechanism selection priority (EXTERNAL > SCRAM > PLAIN, and failure without credentials) and the PLAIN AUTHENTICATE payload, captured via the connection send queue. - ISupportTests: PREFIX/NETWORK/CASEMAPPING/WHOX/MONITOR/length-limit parsing, valueless tokens, prefix-mapping replacement, and malformed/invalid values. - IRCTestSupport: shared offline-client factory.
The previous implementation could not authenticate against any conforming server and did not actually verify the server: - The AuthMessage was assembled from an unordered dictionary rather than the required `client-first-bare , server-first , client-final-without-proof` concatenation, so the proof never matched the server's computation. - The salt was fed to PBKDF2 as raw UTF-8 bytes instead of being base64-decoded (and the attribute parser split on every `=`, truncating the salt's base64 padding). - Server-signature verification compared the base64 *text* bytes of `v=` against the raw signature, so it could never match. Rewrite the exchange to build the messages in the exact order the spec prescribes, retain the client-first-bare and server-first strings verbatim for the AuthMessage, base64-decode the salt, escape the username (`=3D`/`=2C`), verify the server nonce is prefixed by the client nonce, and compare the server signature in constant time after base64-decoding it. Add SCRAMTests driving the canonical RFC 7677 vector (user/pencil): the derived client-final proof and the server-signature acceptance both match the RFC, plus abort paths for a bad signature, a forged nonce, and a server error. Remove the now-unused keyValueString dictionary helpers.
RPL_TOPICWHOTIME (333) and RPL_CREATIONTIME (329) carry the set/creation time as whole seconds since the Unix epoch, but both were parsed with the ISO8601 formatter, which always failed — so topic author/time and channel creation time were silently dropped. Add `Date.fromUnixTimestamp(_:)` and use it for both numerics. Add ChannelInfoTests covering the epoch parsing and a non-numeric value.
The client accepted TLS 1.1 (2006), which is deprecated and exposed to downgrade/BEAST-class attacks. Require TLS 1.2 as the floor.
Outbound line construction only prefixed the trailing parameter with ':' when it contained a space, so a one-word message beginning with ':' (e.g. ":)") was sent unquoted and read by the server as an empty parameter. It also never removed NUL/CR/LF, letting caller-supplied text inject additional commands. Prefix the last parameter with ':' whenever it is empty, contains a space, or begins with ':', and strip NUL/CR/LF from every parameter (over unicode scalars, since Swift treats CR+LF as one grapheme). Drop the now-redundant manual ':' on PONG. Add SendMethodTests for quoting, injection stripping, and PONG.
Nick and channel comparisons were inconsistent — some used exact `==`, one used
`lowercased()` — so a server that varied the case of a name (or used the rfc1459
`[]\~` ⇄ `{}|^` equivalence) could desync membership and channel tracking.
Add `String.ircCaseFolded(mapping:)` implementing the ascii / rfc1459 /
rfc1459-strict mappings (defaulting to rfc1459), plus `IRCClient.caseFold` /
`isSameName`, and route channel lookups (get/add/remove), member lookups, and
self-nick detection through them. Add CaseMappingTests for the folding table and
case-insensitive channel/member lookups.
Capability changes advertised after the initial handshake were ignored: CAP NEW and CAP DEL fell through to the default case, so newly-available capabilities were never requested and withdrawn ones stayed marked enabled. Handle both — CAP NEW records the capabilities and requests any the client understands and has not enabled, CAP DEL drops them from the supported/enabled sets. Track a `capabilityNegotiationComplete` flag (reset per registration) so a CAP ACK/NAK arriving mid-session in response to a CAP NEW updates the enabled set without restarting SASL or re-sending CAP END. Route the handshake's terminal CAP END through a single `endCapabilityNegotiation()`. Add CapNotifyTests for NEW/DEL and the mid-session ACK path.
… verification The flag switches certificate verification to `.none`, disabling hostname validation as well as chain-of-trust — it is not limited to self-signed certificates as the name suggests. Document the man-in-the-middle exposure so callers do not enable it in production expecting a narrower effect.
SuperManifolds
added
bug
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
1 participant
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.
This branch is the result of a deep audit of the library against RFC 1459/2812 and the IRCv3 specs. It fixes a class of remote crashes, corrects three advertised features that were broken on the receive path, brings SASL and message-tags into spec conformance, and adds the library's first real test suite (64 tests) to lock the behaviour in.
The headline issue is that almost every inbound handler indexed
message.parameters[N]directly. A Swift out-of-bounds access is a fatal trap, so any non-conforming or hostile server could crash the client by sending a message with fewer parameters than a handler expected — aCAP *with no subcommand, a004with too few fields, aPRIVMSGfrom a server source with nouser@host, and so on. Inbound handling now goes through a minimum-parameter guard at dispatch plus[safe:]/guard letin the handlers, so a malformed line is dropped rather than aborting the process.On top of that, several things that the README advertises did not actually work:
INVITEwas routed to the KICK handler (with the parameters in the wrong order),MONITORonline/offline replies (730/731) had handlers that were never wired into the dispatcher, and SCRAM-SHA-256 could not authenticate against any conforming server — its AuthMessage was assembled from an unordered dictionary, the salt was never base64-decoded, and the server signature was compared as base64 text rather than raw bytes. IRCv3 message-tag values were also emitted and parsed without escaping, and nick/channel comparisons ignored the server's CASEMAPPING. Each of these is fixed and covered by tests, including the canonical RFC 7677 SCRAM vector.Changes
IRCReply.swift,IRCClient.swift— minimum-parameter-count guard at the dispatch site; route every inbound command through it. FixINVITErouting and wire upRPL_MONONLINE/RPL_MONOFFLINE. Add CASEMAPPING-awarecaseFold/isSameNameand use them for channel/self lookups. Centralise the send path's trailing-parameter quoting and NUL/CR/LF stripping.IRCClient/Handlers/—[safe:]/guard lethardening across CAP, ChannelEvents, ChannelInfo, Privmsg, Notice, UserEvents; parse RPL_TOPICWHOTIME/RPL_CREATIONTIME timestamps as Unix epoch seconds; handle cap-notifyCAP NEW/CAP DELwith acapabilityNegotiationCompleteflag so mid-session ACK/NAK doesn't restart SASL.IRCClient/Authentication/SASLSha256.swift— rewrite SCRAM-SHA-256 to RFC 5802/7677: ordered AuthMessage, base64-decoded salt, dedicated attribute parser (preserves=padding), username escaping, server-nonce prefix check, constant-time signature comparison.[safe:]guards in the PLAIN/EXTERNAL handlers.IRCUser.swift,IRCSender.swift,IRCChannel.swift— failableIRCUser(fromPrivateMessage:)(a server-sourced PRIVMSG no longer crashes); guardedPREFIXparsing; CASEMAPPING-aware member comparisons.IRCMessage.swift,Extensions/String.swift— IRCv3 tag value escape/unescape, parameter sanitisation, case folding, and tag-key parsing that no longer traps on malformed keys. Remove the now-unusedkeyValueStringhelpers.IRCConnection.swift— raise the minimum TLS version to 1.2.IRCClientConfiguration.swift— document thatallowsServerSelfSignedCertificatedisables all verification.IRCServerInfo.swift,Extensions/DateFormatter.swift,IRCClientSendMethods.swift— ISUPPORT bounds guards,Date.fromUnixTimestamp, and per-registration reset of the negotiation flag.Tests
MalformedMessageTests— feeds 34 malformed/hostile lines (missing params, server-sourced PRIVMSG, duration-only STS, garbage tags) through the dispatcher and asserts the process survives and stays usable.SCRAMTests— drives the RFC 7677 vector (user/pencil): the derived client-final proof and the accepted server signature match the RFC byte-for-byte, plus abort paths for a tampered signature, a forged nonce, and a server error.SASLTests— mechanism-selection priority (EXTERNAL > SCRAM > PLAIN, failure without credentials) and the decoded PLAIN AUTHENTICATE payload.IRCMessageParsingTests— prefixes, trailing params with spaces/colons, tags, CTCP/ACTION, unknown-command rejection, server-time.MessageTagTests— the escape table, edge cases, round-tripping, and inbound-parse integration.EventDispatchTests— INVITE field mapping and per-target MONITOR dispatch via NotificationCenter.CaseMappingTests— the ascii/rfc1459/rfc1459-strict fold table and case-insensitive channel/member lookups.CapNotifyTests— CAP NEW request, CAP DEL withdrawal, and that a mid-session ACK enables without a second CAP END.SendMethodTests— trailing-parameter quoting, injection stripping, and PONG echoing.ISupportTests/ChannelInfoTests— ISUPPORT token parsing (PREFIX/NETWORK/CASEMAPPING/WHOX/MONITOR/limits) and epoch-second timestamps.How to test
swift buildswift test— 64 tests, 0 failuresswiftlint lint— clean (also runs green with the pre-existing config)