Backport the security fixes to v0.4 for a 0.4.25 release#707
Open
hsbt wants to merge 20 commits into
Open
Conversation
`Atom` and `Flag` have only been used for argument validation since v0.6.4 (as well as v0.5.14 and v0.4.24), and they validated for absense of `atom-specials`. But they failed to check that the strings are not empty. While this could be used to create syntax errors, I don't believe it amounts a security vulnerability. The result would be no different from any other `BAD` server response, which an application must be prepared to handle.
Previously, integer arguments had to be 32-bit unsigned, and a special case exception was made for the number in a `["CHANGEDSINCE", number]` array. But several other command arguments can be larger: `UNCHANGEDSINCE`, quota `resource-limit`, and `tagged-ext-simple`. So, generic Integer arguments should allow 64 bit numbers. We can add specific argument validation where it makes sense to do so.
This guards against numbers like `99999999999999999999`. This isn't a security issue unless `max_response_size` is `nil`. But it's reasonable to block too large numbers in both the response reader and the response parser, regardless of that config setting. Note also that this still _allows_ strings like `000000000000000001`. This is goofy, but it's how the RFCs are written! ---- 🍒 pick note: `NumValidator` in v0.5 doesn't have `coerce_number64`. Rather than backport _that_, I opted to simply copy a simplified version into both `ResponseReader` and `ResponseParser`.
`#disconnect` could deadlock when called inside the mutex, because the receiver thread could be signaling a condition variable and it would thus be unable to resume until the current thread releases its lock. Also, `#disconnect` shouldn't raise `IO#shutdown` exceptions on the receiver thread prior to closing the socket, when it is being called from the receiver thread. The exception is still raised, _after_ the socket is closed.
This shares most of the validation implementation with RawText, via an extracted shared superclass. Please note: in currently released `net-imap`, QuotedString is _not_ used to quote regular string arguments. It's currently only used by `Net::IMAP#id` (the `ID` extension). Without this patch, `Net::IMAP#id` is vulnerable to the same CRLF injection issues in GHSA-75xq-5h9v-w6px and GHSA-hm49-wcqc-g2xg. The string will be quoted, which may limit the ability to inject some commands. Presumably, `Net::IMAP#id` is unlikely to be called with user-provided input, making this less likely to be exploitable. Nevertheless, CRLF injection should be prevented!
This still allows the argument to be a single string with multiple space-delimited arguments. It splits the string first, and validates the substrings.
Zero-length literals are explicitly allowed by the RFCs and this did not
catch text that ends with `{0}` or `{0+}`. This leaves RawData able to
absorb the `CRLF` that ends the command, and thus absorb the following
command into itself.
Ultimately, we don't care if the `number64` is encoded correctly nor
whether it claims to be a binary literal. So I've simplified the regexp
by dropping `~?` and using `\d+` for the number.
It's bad behavior to send a non-synchronizing literal that's too large. This forces the server to choose between reading but ignoring the bytes or closing the connection. But, for servers that _don't_ support non-synchronizing literals, this could be another CRLF/command injection attack vector. If a server sees the `}\r\n` but can't parse the literal bytesize, it may decide to close the connection, and all is fine. But, a server _might_ respond to any unparseable command line (ending in `CRLF`) with `BAD`, then interpret the literal as the next command. In that case, a CRLF/command injection could succeed. Fortunately, `LITERAL-` is supported by most IMAP servers. So this is not expected to be widely exploitable.
hsbt
changed the title
The non-synchronizing literal tests from the #701 backport used endless method definitions, which require Ruby 3.0. The v0.4 line still supports Ruby 2.7.3, so this broke CI on 2.7.
hsbt
force-pushed
the
backport/v0.4/security-patches
branch
from
e90f822 to
36489fb
Compare
When a client closes the socket abruptly, the server thread's `run` ensure and the test helper's `shutdown` can call `close` concurrently. Without a mutex the `state.logout?` check races, so both callers run `state.logout` and the second raises "already logged out", flaking the send_literal tests on ubuntu CI.
hsbt
force-pushed
the
backport/v0.4/security-patches
branch
from
36489fb to
8a238ad
Compare
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.
Ruby 3.3 bundles net-imap from the 0.4.x line, which still has no fixed release for the three advisories published on 2026-06-09 (CVE-2026-47240, CVE-2026-47241, CVE-2026-47242). They are fixed in 0.5.15 and 0.6.4.1, but the latest 0.4 release is 0.4.24.
As the Ruby 3.3 maintainer I would like a release that carries the behavior changes required by the security fixes, while staying free of the other 0.5.x incompatibilities. Moving 3.3 from 0.4 to 0.5 in a patch release would pull in
SequenceSet, theMessageSetdeprecation, the config object, and a higherrequired_ruby_version, none of which belong in a maintenance update.So I went through v0.5.14..v0.5.15 and backported the equivalent commits onto
v0.4-stable, mirroring your0.5.15backport in #703. I kept only the security fixes and the prerequisites they depend on, adapted them to the 0.4 code (which still targets Ruby 2.7.3 and usesMessageSetrather thanSequenceSet), and left out everything 0.5-only. The full suite is green, and there are no breaking public API changes beyond the intended rejection of the malicious inputs.How do you feel about cutting 0.4.25 from this branch? If it helps, I am happy to handle the release work myself.