-
Notifications
You must be signed in to change notification settings - Fork 180
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
RStudio Weak Credentials Plugin #362
RStudio Weak Credentials Plugin #362
Conversation
canAcceptByCustomFingerprint = | ||
response.status().isSuccess() | ||
&& response.headers().get(SERVER_HEADER).isPresent() | ||
&& response.headers().get(SERVER_HEADER).get().equals(RSTUDIO_HEADER) | ||
&& response | ||
.bodyString() | ||
.map(RStudioCredentialTester::bodyContainsRStudioElements) | ||
.orElse(false); |
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.
Ideally we can have a dedicated web fingerpirnt for rstudio. The web fingerprinter is triggered on all the http services detected by nmap. Feel free to open a new fingerprint request for rstudio.
This is fine for now, no action needed.
ret.append(B64PAD); | ||
return ret.toString(); | ||
} | ||
|
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.
I haven't done this in Java, did a quick google search, does the one-liner mentioned here work?
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.
During the plugin development i was looking for some shortcut to perform the conversion. Back then this was not working, but right now it is. Maybe it was related to other issues in the code. I've fixed this
Hi @maoning, |
public ImmutableList<TestCredential> testValidCredentials( | ||
NetworkService networkService, List<TestCredential> credentials) { | ||
|
||
return credentials.stream() |
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.
I noticed that this tester takes a long time to complete, can we make it return once it finds a valid credential?
Similar to
Lines 77 to 81 in 4ce380a
return credentials.stream() | |
.filter(cred -> isJenkinsAccessible(networkService, cred)) | |
.findFirst() | |
.map(ImmutableList::of) | |
.orElseGet(ImmutableList::of); |
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.
The test takes a long time since the default configuration of RStudio uses RSA, which takes time to decrypt and verify. I've implemented the required change.
@@ -55,6 +55,12 @@ service_default_credentials { | |||
default_usernames: "admin" | |||
default_passwords: "admin" | |||
} | |||
service_default_credentials { |
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.
Could you add another default credential rstudio/rstudio? Creds declared here are being tested first, hopefully short-circuit the plugin more often.
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.
I've implemented the required change. However, since nmap doesn't recognize the service, the standard flow will be followed. This means that the credentials will be bruteforced in the regular way.
Hi @maoning , |
@leonardo-doyensec could you do a rebase of your branch to resolve the merge conflict? |
Hi @maoning, i've fixed the merge conflict. I've also fixed a non working test after the recent commits |
The following PR adds the weak credential tester for RStudio Server.
Some notes on the current version of the rstudio server credential tester.
canAccept()
method since rstudio is not correctly recognized from nmap