Skip to content
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

scan rules: Clean code tweaks #5541

Merged
merged 2 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public int getWascId() {
return 7;
}

private String randomCharacterString(int length) {
private static String randomCharacterString(int length) {
StringBuilder sb1 = new StringBuilder(length + 1);
int counter = 0;
int character = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ public int getRisk() {
return Alert.RISK_HIGH;
}

private String getOtherInfo(TestType testType, String testValue) {
private static String getOtherInfo(TestType testType, String testValue) {
return Constant.messages.getString(
MESSAGE_PREFIX + "otherinfo." + testType.getNameKey(), testValue);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public String getReference() {
return Constant.messages.getString(MESSAGE_PREFIX + "refs");
}

private void checkIfDirectory(HttpMessage msg) throws URIException {
private static void checkIfDirectory(HttpMessage msg) throws URIException {

URI uri = msg.getRequestHeader().getURI();
uri.setQuery(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ private static boolean isRedirectHost(String value, boolean escaped) throws URIE
* @param msg the current message where reflected redirection should be check into
* @return get back the redirection type if exists
*/
private int isRedirected(String payload, HttpMessage msg) {
private static int isRedirected(String payload, HttpMessage msg) {

// (1) Check if redirection by "Location" header
// http://en.wikipedia.org/wiki/HTTP_location
Expand Down Expand Up @@ -471,7 +471,7 @@ private static boolean isRedirectPresent(Pattern pattern, String value) {
* @param type the redirection type
* @return a string representing the reason of this redirection
*/
private String getRedirectionReason(int type) {
private static String getRedirectionReason(int type) {
switch (type) {
case REDIRECT_LOCATION_HEADER:
return Constant.messages.getString(MESSAGE_PREFIX + "reason.location.header");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public String getReference() {
return Constant.messages.getString(MESSAGE_PREFIX + "refs");
}

private String getError(char c) {
private static String getError(char c) {
return Constant.messages.getString(MESSAGE_PREFIX + "error" + c);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private String getEmptyValueResponse(String paramName) throws IOException {
* @param value the value that need to be checked
* @return true if it seems to be encrypted
*/
private boolean isEncrypted(byte[] value) {
private static boolean isEncrypted(byte[] value) {

// Make sure we have a reasonable sized string
// (encrypted strings tend to be long, and short strings tend to break our numbers)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ private boolean sendAndCheckPayload(
return false;
}

private String getContentsToMatch(HttpMessage message) {
private static String getContentsToMatch(HttpMessage message) {
return message.getResponseHeader().isHtml()
? StringEscapeUtils.unescapeHtml4(message.getResponseBody().toString())
: message.getResponseHeader().toString() + message.getResponseBody().toString();
Expand Down Expand Up @@ -700,7 +700,7 @@ public String match(String contents) {
return matchWinDirectories(contents);
}

private String matchNixDirectories(String contents) {
private static String matchNixDirectories(String contents) {
Pattern procPattern =
Pattern.compile("(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
Pattern etcPattern = Pattern.compile("(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
Expand All @@ -727,7 +727,7 @@ private String matchNixDirectories(String contents) {
return null;
}

private String matchWinDirectories(String contents) {
private static String matchWinDirectories(String contents) {
if (contents.contains("Windows")
&& Pattern.compile("Program\\sFiles").matcher(contents).find()) {
return "Windows";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ private HttpMessage createHttpMessage(URI uri) throws HttpMalformedHeaderExcepti
* @return
* @throws URIException
*/
private URI getClassURI(URI hostURI, String classname) throws URIException {
private static URI getClassURI(URI hostURI, String classname) throws URIException {
return new URI(
hostURI.getScheme()
+ "://"
Expand All @@ -288,7 +288,7 @@ private URI getClassURI(URI hostURI, String classname) throws URIException {
false);
}

private URI getPropsFileURI(URI hostURI, String propsfilename) throws URIException {
private static URI getPropsFileURI(URI hostURI, String propsfilename) throws URIException {
return new URI(
hostURI.getScheme()
+ "://"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ public String getDescription() {
return Constant.messages.getString("ascanrules.spring4shell.desc");
}

private boolean is400Response(HttpMessage msg) {
private static boolean is400Response(HttpMessage msg) {
return !msg.getResponseHeader().isEmpty() && msg.getResponseHeader().getStatusCode() == 400;
}

private void setGetPayload(HttpMessage msg, String payload) throws URIException {
private static void setGetPayload(HttpMessage msg, String payload) throws URIException {
msg.getRequestHeader().setMethod("GET");
URI uri = msg.getRequestHeader().getURI();
String query = uri.getEscapedQuery();
Expand All @@ -92,7 +92,7 @@ private void setGetPayload(HttpMessage msg, String payload) throws URIException
uri.setEscapedQuery(query);
}

private void setPostPayload(HttpMessage msg, String payload) {
private static void setPostPayload(HttpMessage msg, String payload) {
msg.getRequestHeader().setMethod("POST");
String body = msg.getRequestBody().toString();
if (body.isEmpty()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ private enum PayloadHandling {
CONCAT_PATH
};

private NanoServerHandler createHttpRedirectHandler(String path, String header) {
private static NanoServerHandler createHttpRedirectHandler(String path, String header) {
return createHttpRedirectHandler(path, header, PayloadHandling.NEITHER);
}

private NanoServerHandler createHttpRedirectHandler(
private static NanoServerHandler createHttpRedirectHandler(
String path, String header, PayloadHandling payloadHandling) {
return new NanoServerHandler(path) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void checkNoPathsHaveLeadingSlash() {
}
}

private void assertNoLeadingSlash(String message, String path) {
private static void assertNoLeadingSlash(String message, String path) {
assertThat(message.replace(REPLACE_TOKEN, path), !path.startsWith("/"), is(true));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ void shouldAlertOnlyIfCertainTagValuesArePresent()
assertThat(alert.getConfidence(), equalTo(Alert.CONFIDENCE_MEDIUM));
}

private NanoServerHandler createNanoHandler(
private static NanoServerHandler createNanoHandler(
String path, NanoHTTPD.Response.IStatus status, String responseBody) {
return new NanoServerHandler(path) {
@Override
Expand Down
3 changes: 2 additions & 1 deletion addOns/ascanrulesAlpha/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased

### Changed
- Maintenance changes.

## [48] - 2024-09-02
### Changed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
*
* @author psiinon
*/
public class ExampleFileActiveScanRule extends AbstractAppParamPlugin {
public class ExampleFileActiveScanRule extends AbstractAppParamPlugin
implements CommonActiveScanRuleInfo {

/** Prefix for internationalized messages used by this rule */
private static final String MESSAGE_PREFIX = "ascanalpha.examplefile.";
Expand Down Expand Up @@ -80,7 +81,7 @@ public String getDescription() {
return Constant.messages.getString(MESSAGE_PREFIX + "desc");
}

private String getOtherInfo() {
private static String getOtherInfo() {
return Constant.messages.getString(MESSAGE_PREFIX + "other");
}

Expand Down Expand Up @@ -155,14 +156,7 @@ public void scan(HttpMessage msg, String param, String value) {
String evidence;
if ((evidence = doesResponseContainString(msg.getResponseBody(), attack)) != null) {
// Raise an alert
newAlert()
.setConfidence(Alert.CONFIDENCE_MEDIUM)
.setParam(param)
.setAttack(attack)
.setOtherInfo(getOtherInfo())
.setEvidence(evidence)
.setMessage(testMsg)
.raise();
createAlert(param, attack, evidence).setMessage(testMsg).raise();
return;
}
}
Expand Down Expand Up @@ -194,7 +188,16 @@ private String doesResponseContainString(HttpBody body, String str) {
return null;
}

private List<String> loadFile(String file) {
private AlertBuilder createAlert(String param, String attack, String evidence) {
return newAlert()
.setConfidence(Alert.CONFIDENCE_MEDIUM)
.setParam(param)
.setAttack(attack)
.setOtherInfo(getOtherInfo())
.setEvidence(evidence);
}

private static List<String> loadFile(String file) {
/*
* ZAP will have already extracted the file from the add-on and put it underneath the 'ZAP home' directory
*/
Expand Down Expand Up @@ -244,4 +247,9 @@ public int getWascId() {
// The WASC ID
return 0;
}

@Override
public List<Alert> getExampleAlerts() {
return List.of(createAlert("foo", "<SCRIPT>a=/XSS/", "<SCRIPT>a=/XSS/").build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.zaproxy.zap.extension.ascanrulesAlpha;

import java.io.IOException;
import java.util.List;
import java.util.Random;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand All @@ -39,7 +40,8 @@
*
* @author psiinon
*/
public class ExampleSimpleActiveScanRule extends AbstractAppParamPlugin {
public class ExampleSimpleActiveScanRule extends AbstractAppParamPlugin
implements CommonActiveScanRuleInfo {

// wasc_10 is Denial of Service - well, its just an example ;)
private static final Vulnerability VULN = Vulnerabilities.getDefault().get("wasc_10");
Expand All @@ -59,8 +61,7 @@ public int getId() {

@Override
public String getName() {
// Strip off the "Example Active Scan Rule: " part if implementing a real one ;)
return "Example Active Scan Rule: " + VULN.getName();
return Constant.messages.getString("ascanalpha.examplesimple.name");
}

@Override
Expand Down Expand Up @@ -118,12 +119,7 @@ public void scan(HttpMessage msg, String param, String value) {
// For this example we're just going to raise the alert at random!

if (rnd.nextInt(10) == 0) {
newAlert()
.setConfidence(Alert.CONFIDENCE_MEDIUM)
.setParam(param)
.setAttack(value)
.setMessage(testMsg)
.raise();
createAlert(param, attack).setMessage(testMsg).raise();
return;
}

Expand All @@ -132,6 +128,10 @@ public void scan(HttpMessage msg, String param, String value) {
}
}

private AlertBuilder createAlert(String param, String attack) {
return newAlert().setConfidence(Alert.CONFIDENCE_MEDIUM).setParam(param).setAttack(attack);
}

@Override
public int getRisk() {
return Alert.RISK_HIGH;
Expand All @@ -148,4 +148,9 @@ public int getWascId() {
// The WASC ID
return 0;
}

@Override
public List<Alert> getExampleAlerts() {
return List.of(createAlert("foo", "attack").build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
<H1>Active Scan Rules - Alpha</H1>
The following alpha status active scan rules are included in this add-on:

<H2>An example active scan rule which loads data from a file</H2>
<H2 id="id-60101">An example active scan rule which loads data from a file</H2>
This implements an example active scan rule that loads strings from a file that the user can edit.<br>
For more details see:
<a href="https://www.zaproxy.org/blog/2014-04-30-hacking-zap-4-active-scan-rules/">Hacking ZAP Part 4: Active Scan Rules</a>.
<p>
Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/ascanrulesAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/ExampleFileActiveScanRule.java">ExampleFileActiveScanRule.java</a>

<H2>Example Active Scan Rule: Denial of Service</H2>
<H2 id="id-60100">Example Active Scan Rule: Denial of Service</H2>
This implements a very simple example active scan rule.<br>
For more details see:
<a href="https://www.zaproxy.org/blog/2014-04-30-hacking-zap-4-active-scan-rules/">Hacking ZAP Part 4: Active Scan Rules</a>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ ascanalpha.examplefile.other = This is for information that doesnt fit in any of
ascanalpha.examplefile.refs = https://www.zaproxy.org/blog/2014-04-30-hacking-zap-4-active-scan-rules/
ascanalpha.examplefile.soln = A general description of how to solve the problem.

ascanalpha.examplesimple.name = Example Active Scan Rule: Denial of Service

#ascanalpha.ldapinjection.alert.attack=[{0}] field [{1}] set to [{2}]
ascanalpha.ldapinjection.alert.attack = parameter [{0}] set to [{1}]
#ascanalpha.ldapinjection.alert.extrainfo=[{0}] field [{1}] on [{2}] [{3}] may be vulnerable to LDAP injection, using an attack with LDAP meta-characters [{4}], yielding known [{5}] error message [{6}], which was not present in the original response.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Zed Attack Proxy (ZAP) and its related class files.
*
* ZAP is an HTTP/HTTPS proxy for assessing web application security.
*
* Copyright 2024 The ZAP Development Team
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.zaproxy.zap.extension.ascanrulesAlpha;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;

import java.util.List;
import org.junit.jupiter.api.Test;
import org.parosproxy.paros.core.scanner.Alert;

class ExampleFileActiveScanRuleUnitTest extends ActiveScannerTest<ExampleFileActiveScanRule> {

@Override
protected ExampleFileActiveScanRule createScanner() {
return new ExampleFileActiveScanRule();
}

@Test
void shouldHaveExpectedExample() {
// Given / When
List<Alert> alerts = rule.getExampleAlerts();
// Then
assertThat(alerts, hasSize(1));
Alert alert = alerts.get(0);
assertThat(alert.getParam(), is(equalTo("foo")));
}

@Test
@Override
public void shouldHaveValidReferences() {
super.shouldHaveValidReferences();
}
}
Loading