Federation: sanitize paths to avoid path traversal attacks #1646
Conversation
jschaul
changed the title
jschaul
force-pushed
the
federation-sanitize-paths
branch
from
e9905c5 to
e8fc0c3
Compare
jschaul
changed the title
| runTestFederator env $ do | ||
| let o = object ["name" .= ("fakeNewUser" :: Text)] | ||
| err <- asInwardErrorUnsafe <$> inwardBrigCall "federation/../i/users" (encode o) | ||
| expectErr IForbiddenEndpoint err |
There was a problem hiding this comment.
dear reviewers, I'd love ideas on what other things to test. Would any property tests make sense here?
There was a problem hiding this comment.
I think having property tests for sanitizePath would be useful, but of course only insofar as the generator produces realistic URL inputs. As for ad-hoc test cases, you could maybe add some with non-ascii character (both percent-encoded and not), query string and/or fragments.
There was a problem hiding this comment.
I agree, some (unit) tests for sanitizePath would be good to have, including one that has "federation" as the only path segment.
There was a problem hiding this comment.
I added a bunch of unit tests, take a look.
There was a problem hiding this comment.
Looks solid already! Another common vector for attacks would be encoding. I assume that URIB.parseRelativeRef handles the decoding of the byte string, so we could test the normalization function for its handling of percent encoding. I don't have a lot of experience in webapp security, but it seems to be a common enough attack vector to have some tests for it, i.e. to make sure that the normalizer properly handles percent decoding before sanitization.
| runTestFederator env $ do | ||
| let o = object ["name" .= ("fakeNewUser" :: Text)] | ||
| err <- asInwardErrorUnsafe <$> inwardBrigCall "federation/../i/users" (encode o) | ||
| expectErr IForbiddenEndpoint err |
There was a problem hiding this comment.
I think having property tests for sanitizePath would be useful, but of course only insofar as the generator produces realistic URL inputs. As for ad-hoc test cases, you could maybe add some with non-ascii character (both percent-encoded and not), query string and/or fragments.
services/federator/test/integration/Test/Federator/InwardSpec.hs
Outdated
Show resolved
Hide resolved
| runTestFederator env $ do | ||
| let o = object ["name" .= ("fakeNewUser" :: Text)] | ||
| err <- asInwardErrorUnsafe <$> inwardBrigCall "federation/../i/users" (encode o) | ||
| expectErr IForbiddenEndpoint err |
There was a problem hiding this comment.
I agree, some (unit) tests for sanitizePath would be good to have, including one that has "federation" as the only path segment.
* Introduce a few error types in `InwardResponse` * Improve readability of ExternalServer in Federator by use of Polysemy.Error also in callLocal * This also solves an existing TODO whereby federator integration tests didn't work due to InwardResponses, whether an error or an expected return value, were parsed always as InwardResponseBody. This may have been an issue with mu-haskell when parsing (only needed in tests), since the behaviour when using `grpccurl` was correct. This is now sidestepped by using more than a simple string on errors. This PR is in preparation to sanitize request paths against path traversal attacks (separate PR #1646)
At the moment we use the convention that all RPCs between federator -> brig/galley/etc use the "federation" path prefix. This PR adds validation to ensure other paths cannot be called, as that would expose non-federation APIs (including internal, non-authenticated ones) to the world. See also the discussion in https://wearezeta.atlassian.net/wiki/spaces/CORE/pages/224166764/Limiting+access+to+federation+endpoints
jschaul
force-pushed
the
federation-sanitize-paths
branch
from
87a871e to
c858462
Compare
|
I added a range of tests, and also changed the implementation to guard against more cases. Merging this PR now, but in case someone still has other ideas, feel free to comment. |
At the moment we use the convention that all RPCs between federator -> brig/galley/etc use the "federation" path prefix. This PR adds validation to ensure other paths cannot be called, as that would expose non-federation APIs (including internal, non-authenticated ones) to the world.
See also the discussion in https://wearezeta.atlassian.net/wiki/spaces/CORE/pages/224166764/Limiting+access+to+federation+endpoints
Checklist