Skip to content

Commit

Permalink
🔒 Limit exponential memory usage to parse uid-set
Browse files Browse the repository at this point in the history
The UID sets in UIDPlusData are stored as arrays of UIDs.  In common
scenarios, copying between one and a few hundred emails at a time, this
is barely noticable.  But the memory use expands _exponentially_.

This should not be an issue for _trusted_ servers, and (I assume)
compromised servers will be more interested in evading detection and
stealing your credentials and your email than in causing client Denial
of Service.  Nevertheless, this is a very simple DoS attack against
clients connecting to untrusted servers (for example, a service that
connects to user-specified servers).

For example, assuming a 64-bit architecture, considering only the data
in the two arrays, assuming the arrays' internal capacity is no more
than needed, and ignoring the fixed cost of the response structs:
* 32 bytes expands to ~160KB (about 5000 times more):
  `"* OK [COPYUID 1 1:9999 1:9999]\r\n"`
* 40 bytes expands to ~1.6GB (about 50 million times more):
  `"* OK [COPYUID 1 1:99999999 1:99999999]\r\n"`
* In the worst scenario (uint32 max), 44 bytes expands to 64GiB in
  memory, using over 1.5 billion times more to store than to send:
  `"* OK [COPYUID 1 1:4294967295 1:4294967295]\r\n"`

----

The preferred fix is to store `uid-set` as a SequenceSet, not an array.
Unfortunately, this is not fully backwards compatible.  For v0.4 and
v0.5, use `Config#parser_use_deprecated_uidplus_data` to false to use
AppendUIDData and CopyUIDData instead of UIDPlusData.  Unless you are
_using_ UIDPLUS, this is completely safe.  v0.6 will drop UIDPlusData.

----

The simplest _partial_ fix (preserving full backward compatibility) is
to raise an error when the number of UIDs goes over some threshold, and
continue using arrays inside UIDPlusData.

For v0.3.x (and in this commit) the maximum count is hard-coded to
10,000.  This is high enough that it should almost never be triggered by
normal usage, and low enough to be a less extreme problem.  For v0.4 and
v0.5, the next commit will make the maximum array size configurable,
with a much lower default: 1000 for 0.4 and 100 for 0.5.  These are low
enough that they are _unlikely_ to cause a problem, but 0.4 and 0.5 can
also use the newer AppendUIDData and CopyUIDData classes.

However, because unhandled responses are stored on the `#responses`
hash, this can still be a problem.  A malicious server could repeatedly
use 160Kb of client memory by sending only 32 bytes in a loop.  To fully
solve this problem, a response handler must be added to prune excessive
APPENDUID/COPYUID responses as they are received.

Because unhandled responses have always been retained, managing
unhandled responses is already documented as necessary for long-lived
connections.
  • Loading branch information
nevans committed Feb 6, 2025
1 parent 60f5776 commit c674700
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 3 deletions.
15 changes: 12 additions & 3 deletions lib/net/imap/response_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ class IMAP < Protocol

# Parses an \IMAP server response.
class ResponseParser
MAX_UID_SET_SIZE = 10_000

include ParserUtils
extend ParserUtils::Generator

Expand Down Expand Up @@ -2023,9 +2025,16 @@ def CopyUID(...) DeprecatedUIDPlus(...) || CopyUIDData.new(...) end
# TODO: remove this code in the v0.6.0 release
def DeprecatedUIDPlus(validity, src_uids = nil, dst_uids)
return unless config.parser_use_deprecated_uidplus_data
src_uids &&= src_uids.each_ordered_number.to_a
dst_uids = dst_uids.each_ordered_number.to_a
UIDPlusData.new(validity, src_uids, dst_uids)
compact_uid_sets = [src_uids, dst_uids].compact
count = compact_uid_sets.map { _1.count_with_duplicates }.max
max = MAX_UID_SET_SIZE
if count <= max
src_uids &&= src_uids.each_ordered_number.to_a
dst_uids = dst_uids.each_ordered_number.to_a
UIDPlusData.new(validity, src_uids, dst_uids)
else
parse_error("uid-set is too large: %d > %d", count, max)
end
end

ADDRESS_REGEXP = /\G
Expand Down
10 changes: 10 additions & 0 deletions test/net/imap/test_imap_response_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,11 @@ def test_fetch_binary_and_binary_size
parser = Net::IMAP::ResponseParser.new(config: {
parser_use_deprecated_uidplus_data: true,
})
assert_raise_with_message Net::IMAP::ResponseParseError, /uid-set is too large/ do
parser.parse(
"A004 OK [APPENDUID 1 10000:20000,1] Done\r\n"
)
end
response = parser.parse("A004 OK [APPENDUID 1 101:200] Done\r\n")
uidplus = response.data.code.data
assert_instance_of Net::IMAP::UIDPlusData, uidplus
Expand Down Expand Up @@ -263,6 +268,11 @@ def test_fetch_binary_and_binary_size
parser = Net::IMAP::ResponseParser.new(config: {
parser_use_deprecated_uidplus_data: true,
})
assert_raise_with_message Net::IMAP::ResponseParseError, /uid-set is too large/ do
parser.parse(
"A004 OK [copyUID 1 10000:20000,1 1:10001] Done\r\n"
)
end
response = parser.parse("A004 OK [copyUID 1 101:200 1:100] Done\r\n")
uidplus = response.data.code.data
assert_instance_of Net::IMAP::UIDPlusData, uidplus
Expand Down

0 comments on commit c674700

Please sign in to comment.