Bugfix/secure hg hook callback #1411
Closed
Conversation
This file contains 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
The issue javasecurity:S5144 which was claimed by SonarQube is now fixed. But SonarQube will find still find the issue, because we will send the request with user controller data, but we have now a secure check if the host is SCM-Manager. So the issue javasecurity:S5144 is now a false-positive and will be suppressed.
sdorra
force-pushed
the
bugfix/secure_hg_hook_callback
branch
from
3db9fef
to
2ce402f
Compare
sdorra
force-pushed
the
bugfix/secure_hg_hook_callback
branch
from
2ce402f
to
87e1476
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
After internal considerations we came to the conclusion, that the changes from this pull do not really fix the problem of possible "man in the middle" attacks. Therefore this will be replaced by another approach where we want to replace the complete http communication with a simple socket connection with an arbitrary port on localhost. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes
The HgHookManager tries to find the URL of the SCM-Manager instance on the first mercurial write request.
This is necessary because it is not always as easy as http://localhost:8080 due to different environments and configurations.
Among other things, the URL is taken from the request headers
HostorX-Forwarded-Host. Theoretically an attacker could send one of these headers with a request to manipulate the HgHookManager to send the ping request to a server of the attacker. If this server responds as expected by the HgHookManager, the SCM Manager sends all Mercurial Hook data to the attacker's server from that point on.To close the gap the HgHookManager send now a temporary challenge with the ping request and the callback servlets answers with a signature of this challenge. With this answer the HgHookManager is able to verify the signature and can now trust that the instance it SCM-Manager.
Furthermore if the HgHookManager could not find a valid Hook URL, SCM-Manager does not allow any mercurial push anymore. Because otherwise it is possible to bypass security relevant mechanisms e.g: BranchWP).
Your checklist for this pull request
Checklist for branch merge request (not required for forks)