Prevent DNS reflection attacks - #1653
Closed
NuxRo wants to merge 1 commit into
Closed
Conversation
Contributor
|
LGTM |
Member
|
LGTM, we need to also test for users who connect to VPN on that network (will DNS work for them as well?). @NuxRo can you edit the PR and change the branch to 4.9, we should have this in 4.9 branch as well. |
Contributor
Author
|
I've tested this in Adv zone with Security Groups and it works. Can't vouch for "pure" Adv zones. Perhaps @ustcweizhou can say more. |
Member
|
@NuxRo if you can change the base-branch of the PR to 4.9, I can initiate some tests on VR/trillian. |
Contributor
Author
|
Rohit, done, go right ahead. |
Member
|
Thanks @NuxRo can you rebase against latest 4.9, looks like some more commits came in. |
Contributor
Author
|
Done? |
Member
|
@NuxRo there are still several commits which have come from master; can you export the commit (git format-patch -1) and reset --hard origin/4.9 and re-apply the commit (git am ) and then git push -f. |
Contributor
Author
|
Rohit unfortunately I do not have git setup properly, I am limited to what the github interface offers me. |
Member
|
@NuxRo okay, can you grant me push access on your fork: https://github.com/NuxRo/cloudstack/tree/patch-4 and I can help fix it for you? |
At least in SG zones DNS on the VR is publicly accessible and as such susceptible to DNS amplification/reflection attacks. This fixes it as per https://issues.apache.org/jira/browse/CLOUDSTACK-6432 Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Member
|
@NuxRo thanks, fixed the branch, you may remove my access now :) @blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1653 |
Member
|
@blueorangutan test matrix |
|
@rhtyd a Trillian-Jenkins matrix job (centos6 mgmt + xs56sp1, centos7 mgmt + vmware55u3, ubuntu mgmt + kvmcentos7) has been kicked to run smoke tests against packages at http://packages.shapeblue.com/cloudstack/pr/1653 |
| ["filter", "", "-A INPUT -i %s -p udp -m udp --dport 53 -s %s -j ACCEPT" % (self.dev, self.address['network'])]) | ||
| self.fw.append( | ||
| ["filter", "", "-A INPUT -i %s -p tcp -m tcp --dport 53 -j ACCEPT" % self.dev]) | ||
| ["filter", "", "-A INPUT -i %s -p tcp -m tcp --dport 53 -s %s -j ACCEPT" % (self.dev, self.address['network'])]) |
Contributor
There was a problem hiding this comment.
Minor nit: extract the value of self.address['network'] to a local variable? Not only will change ensure consistency of the value across the four rules, but it will make the code more literate.
Member
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
@rhtyd I agree that keeping it limited to the scope of the PR is the most pragmatic approach. The intention of my comment was to encourage test case improvement in small increments.
Contributor
|
@NuxRo is it possible to create/update a Marvin test case to verify this change? Also, reading through CS-6432, it appears that this issue was fixed in 4.4.0 and 4.5.0. Did the VR refactoring introduce a regression? If so, it seems appropriate to open a new JIRA ticket for this issue since CS-6432 is marked as resolved. |
Contributor
Author
|
@jburwell I am afraid I do not have the coding skills to do what you want re that variable, I am just submitting @ustcweizhou's code from that Jira issue. Right now basic/sg zones are vulnerable to this and need manual patching, so it must be a regression I guess. I've opened a new issue: |
Contributor
|
@NuxRo understood. I am working to get you some assistance. That work for you? |
Member
|
@NuxRo np, I'll help with the refactoring fixes etc. |
Member
|
@NuxRo can you give me push permission on your repository once again to push the refactoring change, thanks. |
|
Trillian test result (trillian-pr1653-34-xenserver-65sp1-cs49):
|
|
Trillian test result (trillian-pr1653-36-vmware-55u3-cs49):
|
Contributor
Author
|
So, you guys kind of lost me, all I want is my VRs not to be used in DNS reflection attacks. |
Contributor
|
@NuxRo this patch does not appear to break things. However, we want to add a Marvin test case to verify that the fix does not regress again. Does that make sense? @rhtyd is planning to write the Marvin test case this week. Once he is done and we retest, we will merge this PR. |
Contributor
Author
|
Understood, thanks. |
Member
asfgit
pushed a commit
that referenced
this pull request
Aug 30, 2016
[LTS/blocker] CLOUDSTACK-6432: Prevent DNS reflection attacksCLOUDSTACK-6432: Prevent DNS reflection attacks
DNS on VR should not be publically accessible as it may be prone to DNS
amplification/reflection attacks. This fixes the issue by only allowing VR
DNS (port 53) to be accessible from guest network cidr, as per the fix in:
https://issues.apache.org/jira/browse/CLOUDSTACK-6432
- Only allows guest network cidrs to query VR DNS on port 53.
- Includes marvin smoke test that checks the VR DNS accessibility checks from
guest and non-guest network.
- Fixes Marvin sshClient to avoid using ssh agent when password is provided,
previous some environments may have seen 'No existing session' exception without
this fix.
- Adds a new dnspython dependency that is used to perform dns resolutions in the
tests.
Due to repository commit issues I've created this PR, based on #1653 .
/cc @jburwell @karuturi @NuxRo @ustcweizhou @wido and others
* pr/1663:
CLOUDSTACK-6432: Prevent DNS reflection attacks
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
lucas-a-martins
pushed a commit
to scclouds/cloudstack
that referenced
this pull request
Jan 29, 2024
Corrigir ordenação de _secondary storages_ com o algoritmo `firstfitleastconsumed` Closes apache#1653 See merge request scclouds/scclouds!728
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At least in SG zones DNS on the VR is publicly accessible and as such susceptible to DNS amplification/reflection attacks. This fixes it as per https://issues.apache.org/jira/browse/CLOUDSTACK-6432