-
Notifications
You must be signed in to change notification settings - Fork 153
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
base: master
Are you sure you want to change the base?
alpaca mitigations #5061
Conversation
(details subject to change after we get some data about what tags IMAP clients in the wild are sending to our servers) |
9f9067a
to
96e275b
Compare
Needs #5126 to land first |
96e275b
to
bf62745
Compare
bf62745
to
fd4f32d
Compare
fd4f32d
to
1dd94e9
Compare
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': |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
(have not approved, need more time to think/play if you want a full review from me, maybe tomorrow) |
since it's about to get more complicated than just isatom
reduces surface area for cross-protocol reflection attacks against web browsers
It already did, but this early short-circuit was overlooked at the time (This isn't strictly related to this PR, but while I'm in here anyway...)
1dd94e9
to
da2d23b
Compare
Adds the following mitigations that prevent Cyrus IMAP servers being used in Application Layer Protocol Confusion (ALPACA) attacks, particularly against web browsers:
<>
characters (prevents reflecting attacker-supplied HTML content in IMAP responses):
characters (HTTP request header lines now count as basic syntax errors)Fixes #5046