Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alpaca mitigations #5061

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

elliefm
Copy link
Contributor

@elliefm elliefm commented Oct 2, 2024

Adds the following mitigations that prevent Cyrus IMAP servers being used in Application Layer Protocol Confusion (ALPACA) attacks, particularly against web browsers:

  • connection is dropped after excessive consecutive basic syntax errors ("Invalid tag", "Null command", "Unrecognized command")
  • tags cannot contain <> characters (prevents reflecting attacker-supplied HTML content in IMAP responses)
  • tags cannot contain : characters (HTTP request header lines now count as basic syntax errors)
  • the first command's tag cannot be one of the HTTP request methods that accepts a request body (connections from tricked web browsers will look like this)
  • connection is dropped if the first command has an invalid tag, including the first command after STARTTLS is established

Fixes #5046

@elliefm
Copy link
Contributor Author

elliefm commented Oct 2, 2024

(details subject to change after we get some data about what tags IMAP clients in the wild are sending to our servers)

@elliefm elliefm force-pushed the v311/alpaca-mitigations branch from 9f9067a to 96e275b Compare November 11, 2024 02:41
@elliefm
Copy link
Contributor Author

elliefm commented Nov 11, 2024

Needs #5126 to land first

@elliefm elliefm force-pushed the v311/alpaca-mitigations branch from 96e275b to bf62745 Compare November 12, 2024 22:24
@elliefm elliefm force-pushed the v311/alpaca-mitigations branch from bf62745 to fd4f32d Compare November 24, 2024 23:49
@elliefm elliefm force-pushed the v311/alpaca-mitigations branch from fd4f32d to 1dd94e9 Compare December 18, 2024 03:33
@elliefm elliefm marked this pull request as ready for review December 18, 2024 04:01
@elliefm
Copy link
Contributor Author

elliefm commented Dec 18, 2024

I've reviewed the tag data we collected and there's nothing surprising in there that would make me change this implementation, so here it is!

* and test_istag() in imparse.testc
*/
switch (s[0]) {
case 'A':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpd.c only lists methods that it supports. It isn't an exhaustive list. A complete list can be found here:
https://www.iana.org/assignments/http-methods/http-methods.xhtml#methods. I know that all of the methods in RFC 3253 accept bodies.

I wonder if, rather than using a switch statement, we could create a strarray via strarray_split() in service_init() and then just use strarray_find() to check if a tag is an HTTP method.

Copy link
Contributor Author

@elliefm elliefm Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strarray_find would call strcmp for every string in the array until it finds it, or has checked them all -- which would be similar to an if chain, except that strarray also by necessity chases pointers around, so it can't benefit from prefetching if its strings aren't already in cache (which they probably aren't, since we only get in here on the first client command). We could perhaps improve that by pre-sorting the strarray (or the hypothetical if chain) by expected frequency, such that at least we'd be comparing the most likely strings first, but that seems like a lot of work.

The switch approach only calls strcmp 5 times in the worst case (if the tag starts with 'P' and isn't one of the first four of those), and 0 in the likely case (when the tag doesn't start with any of these letters), and when it does call strcmp, the comparators are accessed directly with no pointer chasing.

It might not make much difference in practice, though. If this were unusual or tricky code, I'd be in favour of simplifying it for readability/maintainability, at maybe some performance loss. But switching on the first character is an idiom we already use a bunch in Cyrus, it's not unusual, nor hard to maintain.

cassandane/Cassandane/Cyrus/Alpaca.pm Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/Alpaca.pm Show resolved Hide resolved
imap/imapd.c Show resolved Hide resolved
@wolfsage
Copy link
Contributor

(have not approved, need more time to think/play if you want a full review from me, maybe tomorrow)

@elliefm elliefm force-pushed the v311/alpaca-mitigations branch from 1dd94e9 to da2d23b Compare December 22, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

countermeasures to ALPACA attack?
3 participants