Reject CR/LF/NUL in user-supplied IMAP command text - #119
Conversation
Adds UIDPLUS extension support so callers can recover the UIDs the server assigns on APPEND/COPY and target expunges by UID: - appendFullUID: like appendFull, returns the APPENDUID response code - copyUID / copyUIDs / copyUIDR: UID COPY returning the COPYUID code - uidExpunge / uidExpungeR: UID EXPUNGE over a UID set or range New types AppendUID and CopyUID, the UIDSet alias, and the APPENDUID/COPYUID/UIDNOTSTICKY status codes with their parsers. sendCommandWithResponse exposes the tagged ServerResponse so the response codes can be read. The existing appendFull/copyFull keep their old signatures by discarding the UID result. Covered by new parser cases in baseTest and a dedicated imapUIDPlusTest group exercising the API against scripted server responses.
RFC 3501 keywords, response codes, flag names and status attributes are case-insensitive, but the parser matched them with case-sensitive `string`, so any server replying with non-canonical casing (e.g. `ok`, `* search`, `[uidvalidity ...]`, `\seen`) caused a parse error. Adds `stringCI`/`charCI` and applies them throughout the response parser. Along the way the tagged/fatal response handling is factored into `pWithTaggedOrFatal`, `pDone` is split into `pRespCode`/`pRespText`/`pStatusCode`, and string parsing is unified (`pQuotedString`/`pLiteralString`/`pAString`/`pMailboxName`). This also: - surfaces an untagged `* BYE` as a fatal response instead of failing - accepts an empty `* SEARCH` reply (no matches) - accepts a `NIL` hierarchy separator in LIST/LSUB - stops `atomChar` from running past CR/LF Covered by a new caseInsensitiveTest group.
IMAP SEARCH string keys (BCC/BODY/CC/FROM/HEADER/SUBJECT/TEXT/TO/
X-GM-RAW) were interpolated unquoted, so any value containing a space
or special character produced a malformed command. They are now wrapped
with quoteIMAPString.
LARGER/SMALLER wrongly wrapped their octet count in `{}` (the literal
syntax), e.g. `LARGER {100}`; the size is a plain number (`LARGER 100`).
SEARCH now auto-detects non-ASCII text in a query and prepends
`CHARSET UTF-8`, and command bytes are sent UTF-8 encoded
(sendCommandNoResponse) so the non-ASCII bytes survive instead of being
truncated to Latin-1.
Covered by a new imapSearchApiTest group.
A handful of independent fixes to the IMAP client:
- connectStream accepts a PREAUTH greeting and matches the greeting
status case-insensitively (was: only exact "* OK")
- AUTHENTICATE now loops over '+' challenge continuations via
getAuthResponse, so multi-step SASL mechanisms (e.g. XOAUTH2) that
emit an error continuation before the tagged reply complete instead
of stalling; the challenge prefix is matched as "+" not "+ "
- getResponse understands non-synchronising literals ({n+}) and the
binary literal prefix (~{n}), not just plain {n}
- fetch/fetchPeek/fetchR/fetchRPeek normalize a NIL body to empty
- storeFull reads result flags from the FLAGS key (was "FLAG", which
never matched) and quotes X-GM-LABELS values
- escapeLogin is unified with quoteIMAPString (drops the invalid
backslash-escaping of { and } inside a quoted string)
Covered by a new imapRobustnessTest group.
User-controlled strings were interpolated straight into command lines. A value containing CR, LF or NUL could therefore terminate the current command and smuggle additional IMAP commands onto the connection (command injection). Adds validateCommandText, which fails loudly when such a character is present, and wires it into every user-supplied argument: login credentials, mailbox names (select/examine/create/delete/rename/ subscribe/unsubscribe/status/append/copy/move), SEARCH charset and string query terms, FETCH commands and header field names, STORE flag keywords, and Gmail labels. Covered by a new imapHardeningTest group.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
HaskellNet/src/Network/HaskellNet/IMAP.hs
Lines 697 to 700 in c8f500d
Continue parsing literals inside parenthesized FETCH values
When a parenthesized FETCH value such as ENVELOPE or BODYSTRUCTURE contains a legal literal, the first bsGetLine ends at that literal marker, but this branch only scans the current buffer for the closing parenthesis. scanParenValueEndBS consequently returns Nothing and the request fails with cannot parse FETCH value instead of reading the literal from the stream; the parenthesized-value parser needs to participate in the streaming literal handling.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parseMailbox = do space | ||
| mbox <- pListMailboxName | ||
| mbox <- pMailboxName |
There was a problem hiding this comment.
For an unquoted LIST/LSUB mailbox containing %, *, or ], routing the name through pMailboxName now rejects the response because its pAString fallback uses atomChar, which excludes those characters. They are valid list-char characters, and the previous pListMailboxName accepted any non-space line content, so such a response is now swallowed by pOtherLine and silently omitted from the returned mailbox list.
Useful? React with 👍 / 👎.
| char ')' | ||
| return attrs | ||
| parseSep = space >> char '"' >> anyChar `manyTill` char '"' | ||
| parseSep = space >> ((string "NIL" >> return "") <|> pQuotedString) |
There was a problem hiding this comment.
When a server sends a lowercase or mixed-case hierarchy separator such as * LIST () nil INBOX, this literal string "NIL" fails even though IMAP protocol tokens are case-insensitive. The LIST line then falls through to pOtherLine, causing list/lsub to omit the mailbox rather than returning it with an empty separator.
Useful? React with 👍 / 👎.
| pWithTaggedOrFatal lineParser build = tagged <|> fatal | ||
| where | ||
| tagged = do untagged <- many lineParser | ||
| resp <- Parser pDone | ||
| let (mboxUp, values) = mkMboxUpdate untagged | ||
| return (resp, mboxUp, build values) | ||
| fatal = do resp <- pFatalLine |
There was a problem hiding this comment.
When an ordinary untagged update precedes * BYE (for example, * 23 EXISTS followed by a shutdown notice), the tagged branch consumes both lines through many lineParser because pOtherLine also accepts BYE, and then fails looking for the tagged completion. The fatal alternative can only restart at the first line, where it sees EXISTS rather than BYE, so parsing fails instead of surfacing the fatal response; BYE must be recognized while iterating over untagged lines.
Useful? React with 👍 / 👎.
1 participant
Top of the stack — based on #118 → #117 → #116 → #115. Final PR in the series upstreaming the belege.ai fork's IMAP fixes. The diff shows the earlier commits until they merge; the commit for this PR is "Reject CR/LF/NUL in user-supplied IMAP command text". Merge order: #115 → #116 → #117 → #118 → this.
What
User-controlled strings are interpolated straight into IMAP command lines. A value containing CR, LF or NUL can terminate the current command and smuggle additional commands onto the connection — i.e. IMAP command injection (e.g. a mailbox name
INBOX\r\nA001 DELETE victim).validateCommandTextfails loudly when such a character is present, and is wired into every user-supplied argument:select/examine/create/delete/rename/subscribe/unsubscribe/status/append/copy/moveSEARCHcharset and string query terms (BCC/BODY/CC/FROM/HEADER/SUBJECT/TEXT/TO/X-GM-RAW, recursing throughNOT/OR)FETCHcommands and header field namesSTOREflag keywords and Gmail labels (appendFullflags too)(UID-set arguments are excluded — they're always generated internally from
[UID]/ranges and only ever contain digits,,,:.)Tests
New
imapHardeningTest: injection attempts via login, select, search, fetch and store are all rejected with aCR, LF, or NULerror.🤖 Generated with Claude Code